phobos
phobos copied to clipboard
dlang
Add std.traits.isStronglyPure
See relevant NG discussion: https://forum.dlang.org/thread/[email protected]
D currently allows functions to be pure even though they are not strictly pure. To be strictly pure, a function must satisfy the following:
- Each parameter:
- is
immutable, OR - can be converted automatically to
immutable(i.e. has no mutable indirections) AND is passed by value
- is
- The return type:
- is
immutable, OR - can be converted automatically to
immutable
- is
Andrei has said that const will not count for now
I'm also unsure on how to unittest the second overload for isStronglyPure, since overloading of functions within the local scope is not allowed. Please help with this. Thanks!
Thanks for your pull request and interest in making D better, @SirSireesh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:
- My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
- My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
- I have provided a detailed rationale explaining my changes
- New or modified functions have Ddoc comments (with
Params:andReturns:)
Please see CONTRIBUTING.md for more information.
If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.
Bugzilla references
Your PR doesn't reference any Bugzilla issue.
If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub run digger -- build "master + phobos#6403"
I'm not entirely sure what the use case for this trait will be, but for those who don't know and as it's a bit related:
Vibe.d has the concept of isStronglyIsolated and isWeaklyIsolated (combined with makeIsolated and assumeIsolated):
https://github.com/vibe-d/vibe-core/blob/master/source/vibe/core/concurrency.d#L960 https://github.com/vibe-d/vibe-core/blob/master/source/vibe/core/concurrency.d#L310
I've removed the second overload for the following reasons:
- It does not work with local functions
- Also makes it harder to unittest.
- None of the other functions in
std.traitsdo this type of overloading as far as I can tell
Merge branch 'std.traits.isStronglyPure' of github.com:
Please don't use merge commits on your branch - instead rebase is recommended:
https://wiki.dlang.org/Starting_as_a_Contributor#Rebasing https://wiki.dlang.org/Starting_as_a_Contributor#Squashing https://wiki.dlang.org/Starting_as_a_Contributor#A_file_that_I_made_a_change_on_was_modified_by_a_different_merged_PR.2C_and_now_my_PR_can.27t_be_merged.2C_now_what.3F
We also prefer all "fixup" commits to be squashed, but this can wait until the end (i.e. when this function has been approved) and we can also easily do this for you.
BTW also notice the bot's message:
If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
In this case a changelog entry is preferred as it's a new symbol.
Is this the same as the definition of "strong purity" that the compiler uses in deciding whether certain optimizations are legal?
@scarface-one no need to close this PR. It's better to keep it, s.t. the discussion isn't split over multiple places and it's easier for people to follow. I will reopen this and fix (aka "rebase") it for you and squash all commits for you.
Thanks, I panicked a bit after that. I'll add changelog and fix the docs now
no need to close this PR
One reason in favor of recreating this is that now all the code owners that were accidentally assigned are now following this discussion, and would need to manually unsubscribe to avoid getting irrelevant notifications in their inbox.
See https://forum.dlang.org/post/[email protected] Essentially:
int[] f(int x) pure
{
return [x];
}
immutable var = f(1); // works
This was previously incorrectly reported to not be strongly pure. I've fixed this, but I think checking return type is unnecessary. I think that if all of a function's parameters are immutable or implicitly convertible to immutable, then its return type can always be converted to immutable.
Can someone give a case where the return type may not be implicitly converted to immutable, given that the parameters can all be implicitly converted to immutable?
Can someone give a case where the return type may not be implicitly converted to immutable, given that the parameters can all be implicitly converted to immutable?
That's not the interesting case. I believe the counterexample you are looking for is rather:
auto var1 = f(1);
var1[0] = 0;
auto var2 = f(1);
Obviously, rewriting this to
auto var1 = f(1);
var1[0] = 0;
auto var2 = var1;
would be wrong.
Obviously, rewriting this to
auto var1 = f(1); var1[0] = 0; auto var2 = var1;would be wrong.
Makes sense. Then I think isStronglyPure should require that the function's return type is explicitly annotated with immutable
I've changed it to require that the return type is strictly immutable or void. I'll squash the commits later
Then I think isStronglyPure should require that the function's return type is explicitly annotated with immutable
Not quite. The type must be implicitly convertible to immutable, as in your original wording. int[] is not implicitly convertible to immutable(int[]) – it's just the purity of f() that allows this to happen in your above example.
If f was marked strongly pure, then
auto var1 = f(1);
var1[0] = 0;
auto var2 = var1;
becomes correct. On the other hand if f's return type was marked immutable then this would not be allowed. Which is why I think it should strictly be immutable
Ping. Should I revert this to the old behaviour (allow implicit conversion to immutable) or is the new stricter behaviour preferable?
I think the strict behaviour should be preferred, since would be weird if isStronglyPure!F was true but the compiler was not treating F like a strongly pure function. e.g. are there in above discussions
Ping. Should I revert this to the old behaviour (allow implicit conversion to immutable) or is the new stricter behaviour preferable?
Implicit conversion to immutable should be allowed.
immutable(int) foo(int) pure;
behaves the same as
int foo(int) pure;
immutable(int) is implicitly convertible to int, but immutable(int[]) not to int[] – which takes care of the above problem.
Ok I think I've addressed all the comments except Andrei's. He said that ref return types should not be allowed, but I'm not sure how to check if the return type is ref. The problem is that ref is not a storage class outside of function parameters, so there's no way I'm aware of, to check if the return type is ref.
Also, the following code passes:
ref int f(int x)
{
return *(new int(x));
}
immutable a = f(1);
immutable b = f(1);
static assert(a == b);
I think as long as the return type has no mutable indirections, this should be allowed as strongly pure. Am I correct?
Ok I figured out how to check if return type is ref. It applies as a function's attributes, as in the docs. ref return types are now not allowed unless marked immutable. Just a summary of what counts as strongly pure:
- The parameters are all
immutableor implicitly convertible toimmutable - The return type is
immutableor implicitly convertible toimmutable
Types may be implicitly converted to immutable if they are not ref of pseudo-ref types and, if they are aggregate types, the same applies recursively to all subtypes
I just realised that the following code passes:
struct S
{
int x;
int f() pure
{
return x++;
}
}
static assert (isStronglyPure!(S.f));
The correct thing to do would be to ensure that member functions are marked as const, but I'm not sure how one can find out if a function is a member of a struct or a class or a free function. Does anyone know how this can be done?
The correct thing to do would be to ensure that member functions are marked as const
Actually, it would have to be immutable, since strongly pure requires that the parameters be immutable or implicitly convertible to immutable.
Oh dear, I had a whole page-long review written up a few weeks ago, including a simplified implementation and several design comments. It seems like it never made it to the GitHub server, though. Apologies for the silence – I'll try to find some time to collect my thoughts on this again soon.
Actually, it would have to be
immutable, since stronglypurerequires that the parameters beimmutableor implicitly convertible toimmutable.
Yes I meant immutable. I changed it so member functions are now required to be marked immutable.
Does anyone know how to fix the "unreachable code" warnings? I've tried making a few of the ifs static ifs, but that just moves the warning to a different line?
Oh dear, I had a whole page-long review written up a few weeks ago, including a simplified implementation and several design comments. It seems like it never made it to the GitHub server, though. Apologies for the silence – I'll try to find some time to collect my thoughts on this again soon.
No problem