New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Issue 809 - Should be possible to convert lazy argument to delegate #10264
Conversation
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#10264" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this change, but it needs a runnable test, not compilable.
Yeah, makes sense to me. |
} | ||
} | ||
} | ||
|
||
exp.e1 = exp.e1.toLvalue(sc, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok that exp.e1
here and following all of this could be mutated by lines 6247 and 6248 if it is a TOK.variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine because it is only performing semantic analysis, thus enriching the AST. In order to be sure that you have a delegate there you need to do some transformations.
@Geod24 Done. |
Merge after dmd PR: dlang/dmd#10264
Spec change: dlang/dlang.org#2686 |
I'm having second thoughts about this. Suppose we were to some day introduce rvalue references. Would we then want the |
In addition to what JinShil said, I also think there's some inconsistency here. Currently taking the address of a delegate gives you a pointer to that delegate. If lazy is to be viewed as a delegate (as making sense according to the bug report) then there's an asymmetry between |
I think that if we were to introduce rvalue references (which I hope we will not), one solution might be to consider
Yes. Today, whenever you use |
Mhm. Ok. Are you sure? Below when I reference
Is |
@aliak00 I don't see any references to
Not really. |
Hmm. Seems I didn't even finish my typing 😛 I did not mean (the code i posted above uses |
Ok. I understand. Currently, how you've written the example it will not compile because With my patch, this does not change; you will have the same behavior, however, if you write |
Ah true. Sorry, the example does not compile yes. Yeah I understand the proposal. It's just that my understanding of what a
To me, if we say that |
That is why your first example compiles (you can take the address of a I hope this explanation makes things a bit clearer. |
Yep, finally got it! Thanks a lot @RazvanN7 |
Don't mention it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing which is not handled, much more fundamental:
At the moment lazy
are scope delegate
. Meaning they don't perform memory allocation.
If you can extract them, it means the extract delegate must be scope
, and you need to add a test with -dip1000
otherwise it breaks safety.
@Geod24 Nice catch! I added a fail compilation test. |
Autotester failure is unrelated. |
src/dmd/expressionsem.d
Outdated
*/ | ||
if (e1.op == TOK.variable) | ||
if (e1.op == TOK.variable && !((cast(VarExp)e1).delegateWasExtracted)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use isVarExp()
src/dmd/expressionsem.d
Outdated
* to the delegate anymore, but rather, the | ||
* actual symbol. | ||
*/ | ||
if (exp.e1.op == TOK.variable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use isVarExp()
src/dmd/expressionsem.d
Outdated
{ | ||
exp.e1 = exp.e1.expressionSemantic(sc); | ||
exp.e1 = resolveProperties(sc, exp.e1); | ||
if (exp.e1.op == TOK.call) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use isCallExp()
src/dmd/expressionsem.d
Outdated
auto callExp = cast(CallExp)exp.e1; | ||
if (callExp.e1.type.ty == Tdelegate) | ||
{ | ||
VarExp ve2 = cast(VarExp)callExp.e1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use isVarExp()
. Also, how do you know callExp.e1 is a VarExp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because resolvePropertiesX replaces the VarExp with a CallExp for which e1 is the initial VarExp. (look at line 1302 in expressionsem.d)
if (callExp.e1.type.ty == Tdelegate) | ||
{ | ||
VarExp ve2 = cast(VarExp)callExp.e1; | ||
ve2.delegateWasExtracted = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why "extracting a delegate" permanently alters the semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expression semantic on an AddrExp will rewrite the AST so that we have the name of a delegate instead of variable; after that when resolve properties is called on the newly rewritten AST node, when it finds the var name it will try to replace it again as a CallExp, so we need to store the fact that the delegate was already extracted.
src/dmd/expressionsem.d
Outdated
if (exp.e1.op == TOK.call) | ||
{ | ||
auto callExp = cast(CallExp)exp.e1; | ||
if (callExp.e1.type.ty == Tdelegate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use type.toBasetype().ty, not type.ty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments.
I'm unsure as well that this won't simply produce other inconsistencies. Is this a problem that actually needs solving? Note that the bugzilla issue gives a simple workaround. |
@WalterBright I made the changes. Thanks for your feedback.
It seems that folks want this as it is more expressive than the workaround. |
Spec change: dlang/dlang.org#2686 I think this is worth moving forward with. cc @atilaneves |
Let's see if @WalterBright is okay with the changes following the tech review. On the semantics side, |
I like it but I'll defer to @WalterBright. |
Ping. |
Another respectful ping on this. |
Adding 72h -> merge |
&dg
will extract the delegate, ifdg
is an lazy argument. I'm not sure if this needs a spec PR or not; if it does, where would it be appropriate to add it? Maybe the lazy parameters section [1]?[1] https://dlang.org/spec/function.html#lazy-params