Fix Issue 8006: Property binary assignment operators#7079
Fix Issue 8006: Property binary assignment operators#7079JinShil wants to merge 1 commit intodlang:masterfrom JinShil:master
Conversation
|
Appears to also fix Issue 16187 |
JinShil
changed the title
src/ddmd/expressionsem.d
Outdated
| assert(false, "Binary assignment operator was not handled."); | ||
| } | ||
|
|
||
| if (Expression ex = resolvePropertiesX(sc, e1, e2)) |
There was a problem hiding this comment.
Hmm, the backend should already handle lowering a += b into a = a + b.
Maybe all you need is the call to resolvePropertiesX() ?
There was a problem hiding this comment.
resolvePropertiesX lowers e1 = e2 into e1(e2) if e1 is a property, so I need e2 to be e1 + e2 for that to work.
Currently DMD errors with e1() is not an lvalue, so, if e1 += e2 is lowered to e1 = e1 + e2 in the backend, maybe I just need to figure out how to get past that error so the expression makes it to the backend. But are you sure the backed is doing the lowering?
There was a problem hiding this comment.
Actually, yes, I see the reasoning behind this. e1 = e1 + e2 won't cut the muster, as it will assign the result to a temporary. You need it to be e1(e1 + e2)
There was a problem hiding this comment.
Giving this a quick test with gdc, it looks like resolvePropertiesX is changing the return type of ti.x() from int to void.
You may need to store the rhs in a temporary here.
There was a problem hiding this comment.
Thanks @ibuclaw. I've copied the rhs to a temporary and refactored in an attempt to consolidate code.
src/ddmd/expressionsem.d
Outdated
| { | ||
| // Consider s.x += 1; | ||
| // We want to rewrite that expression to s.x(s.x() + 1); | ||
| // However, resolvePropertiesX will change the type of s.x() to void, |
There was a problem hiding this comment.
Thanks. Looking at this after sleeping on it, I sense that what I am see is infact memory that is being shared between both s.x() and s.x(1) calls. When the latter gets rewritten, it changes the type of the former also (oops!).
I'm just having a look now. I think this is close to being where it should be anyway, just a few nits.
src/ddmd/expressionsem.d
Outdated
| * the rewritten expression if the procedure succeeds, an `ErrorExp` if the | ||
| * and error is encountered, or `null` if `e.e1` is not a `DotIdExp`. | ||
| */ | ||
| Expression binAssignRewriteProp(BinAssignExp e, Scope* sc) |
There was a problem hiding this comment.
This could be renamed to binSemanticProp, then you don't have to call this explicitly.
There was a problem hiding this comment.
Nice call! That made the code just that much more concise.
src/ddmd/expressionsem.d
Outdated
| { | ||
| // https://issues.dlang.org/show_bug.cgi?id=8006 | ||
|
|
||
| if (e.e1.op == TOKdotid) |
There was a problem hiding this comment.
Maybe its better to complete semantic on e1 first, then check that e1.op == TOKcall and ce.e1.type.isproperty.
There was a problem hiding this comment.
I'm not sure what you mean by ce. isproperty is only a field in TypeFunction, and I'm not sure how to get that. I tried a number of different things (e.g. casting to TypeFunction), but they all resulted in a segfault.
There was a problem hiding this comment.
After playing around, this is what I ended up with.
https://pastebin.com/iUXCpeju
Whether it is more correct than what you are doing here, I may have to leave someone else to be the judge of.
src/ddmd/expressionsem.d
Outdated
| auto v = copyToTemp(0, "__tmp", e.e1); | ||
| auto de = new DeclarationExp(e.e1.loc, v); | ||
| auto ve = new VarExp(e.e1.loc, v); | ||
| Expression ea = Expression.combine(de, ve); |
There was a problem hiding this comment.
If you pass a copy of e1 to resolvePropertiesX, then this shouldn't be needed. Codegen looks better at least without this.
There was a problem hiding this comment.
What do you mean by "this"? Do you mean I don't need to copy to a temporary?
|
It appears a recent merge may have broken this :( |
I was about to say, I couldn't see anything wrong with the diff, and running it locally passes for me. Then again, maybe you updated it just before I looked. :-) |
src/ddmd/expressionsem.d
Outdated
|
|
||
| // Consider s.x() += 1; | ||
| // We want to rewrite that expression to s.x(s.x() + 1); | ||
| if (ce.e1.type.ty == Tfunction && (cast(TypeFunction)ce.e1.type).isproperty) |
There was a problem hiding this comment.
Actually, I think it's best to have this check as:
auto tf = (ce.e1.type.ty) == Tfunction ? cast(TypeFunction)ce.e1.type : null;
if (tf && tf.isproperty && !tf.isref)
So that ref @property functions are not rewritten.
There was a problem hiding this comment.
Ok, that seems to work. I'll try to formulate a test case for that as well. Thanks!
|
I think I need the return type of |
Though that is consistent with |
Added tests to ensure that if a setter |
|
Added tests for |
|
Thanks to @ibuclaw, this turned out much better than I could have hoped. I thought I was going to have to split this into a couple of different pull requests, and handle a lot of special casing, but this turned out highly cohesive, comprehensive, and surprisingly simple. I think this is good to go. |
src/ddmd/expressionsem.d
Outdated
| } | ||
|
|
||
| /******************************************************************************** | ||
| * Helper function to consolidate common code for `BinAssingExp`s. |
There was a problem hiding this comment.
Maybe:
Helper function to resolve
@propertyfunctions in aBinAssignExp.
Also, spelling mistake in BinAssignExp
src/ddmd/expressionsem.d
Outdated
| } | ||
|
|
||
| // s.x(e2x) | ||
| return resolvePropertiesX(sc, e.e1.copy(), e2x); |
There was a problem hiding this comment.
Perhaps the result should be checked here, and only return if not null.
test/fail_compilation/test8006.d
Outdated
| /* | ||
| * TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/test8006.d(48): Error: function test8006.TInt.x () is not callable using argument types (int) |
There was a problem hiding this comment.
It might be worth investigating whether we could improve the fail message here.
There was a problem hiding this comment.
Agreed. I looked into this and found that the error messages really need to be improved for all usages of properties, not just for the binary assignments in this pull request.
The error message situation is broader than the scope of this pull request. Simple property assignments are currently implemented and produce the same error message as above. So this pull request maintains the status quo.
The following scenarios all produce different error messages, none of which read specifically to syntax of property usage.
- Attempting to set without a setter
- Attempting to get without a getter
- Attempting to set without a setter through
alias this - Attempting to get without a getter through
alias this - Attempting to set without a setter through
static alias this - Attempting to get without a getter through
static alias this - Attempting to set a property to the wrong type
- Attempting to set a property to the wrong type through
alias this - Attempting to set a property to the wrong type through
static alias this - etc...
So, instead of improving the error messages in this pull request, I suggest accepting this pull request (maintaining the status quo) and improve the error messages for properties more comprehensively in a separate issue and pull request.
There was a problem hiding this comment.
... improve the error messages for properties more comprehensively in a separate issue and pull request.
|
I'd like to have the last say made by @WalterBright, incase there's something I might have missed. |
|
Are we really sure we want to do this? This is a significant language change with essentially no discussion about its merits. |
src/ddmd/expressionsem.d
Outdated
|
|
||
| /******************************************************************************** | ||
| * Helper function to resolve `@property` functions in a `BinAssignExp`. | ||
| * It rewrites expressions of the form, for example, `s.x += 1` to `s.x(s.x() += 1)` |
There was a problem hiding this comment.
I think you mean s.x(s.x() + 1)
src/ddmd/expressionsem.d
Outdated
| // Consider s.x() += 1; | ||
| // We want to rewrite that expression to s.x(s.x() + 1); | ||
|
|
||
| // Only rewrite @property functions. Don't rewrite @ref @property functions. |
There was a problem hiding this comment.
First of all, this already works today:
struct TInt
{
int mX;
ref @property int x()
{
return mX;
}
alias x this;
}
void main()
{
TInt t;
t.x += 4;
assert(t.mX == 4);
t.x -= 2;
assert(t.mX == 2);
t.x *= 4;
assert(t.mX == 8);
t.x /= 2;
assert(t.mX == 4);
t.x %= 3;
assert(t.mX == 1);
t.x <<= 3;
assert(t.mX == 8);
t.x >>= 1;
assert(t.mX == 4);
t.x >>>= 1;
assert(t.mX == 2);
t.x &= 0xF;
assert(t.mX == 0x2);
t.x |= 0x8;
assert(t.mX == 0xA);
t.x ^= 0xF;
assert(t.mX == 0x5);
t.x ^^= 2;
assert(t.mX == 25);
// same as test above, but through the `alias this`
t = 0;
t += 4;
assert(t.mX == 4);
t -= 2;
assert(t.mX == 2);
t *= 4;
assert(t.mX == 8);
t /= 2;
assert(t.mX == 4);
t %= 3;
assert(t.mX == 1);
t <<= 3;
assert(t.mX == 8);
t >>= 1;
assert(t.mX == 4);
t >>>= 1;
assert(t.mX == 2);
t &= 0xF;
assert(t.mX == 0x2);
t |= 0x8;
assert(t.mX == 0xA);
t ^= 0xF;
assert(t.mX == 0x5);
t ^^= 2;
assert(t.mX == 25);
}But more importantly, we found that we broke code in DUB when not excluding ref @property functions. The specific idiom that broke was added to the test suite here.
So ref @property functions were excluded in an effort to be conservative with this change, and avoid code breakage.
There was a problem hiding this comment.
ok. Please add a summary of that explanation as a comment in the code.
There was a problem hiding this comment.
Please add a summary of that explanation as a comment in the code.
Done.
There was a problem hiding this comment.
If there's a getter annotated with ref and a setter. If there's no rewrite for ref functions the setter will be bypassed.
There was a problem hiding this comment.
You should add tests that specifically test which functions are and are not called (i.e. besides testing that the calculation is performed correctly, also test that the getter/setter is/isn't called), both for the non-ref and ref case.
src/ddmd/expressionsem.d
Outdated
| e2x = new UshrExp(e.loc, e1x, e2x); | ||
| break; | ||
| default: | ||
| assert(false, "Binary assignment operator was not handled."); |
There was a problem hiding this comment.
Some people like assert messages, I don't. They bloat up the compiler. Leaving the message as a comment in the source code is plenty good enough.
There was a problem hiding this comment.
In fact, you can just make the switch final and do away with the assert entirely!
There was a problem hiding this comment.
In fact, you can just make the switch
finaland do away with the assert entirely!
I can't do that because the switch doesn't cover all possible Tokens.
Leaving the message as a comment in the source code is plenty good enough.
Done.
|
The implementation itself does look nice. |
src/ddmd/expressionsem.d
Outdated
| e2x = new ShrExp(e.loc, e1x, e2x); | ||
| break; | ||
| case TOKushrass: | ||
| e2x = new UshrExp(e.loc, e1x, e2x); |
There was a problem hiding this comment.
Another thing that sprung to mind, you'll have to assign the rewrite to another variable. If resolvePropertiesX doesn't return a value for whatever reason, you don't want to assign it to the original binassignexp.
Think s.x += 1 being accidentally turned into s.x += s.x + 1
There was a problem hiding this comment.
It took me a little while to see what you meant. but I got it. Nice catch! Thanks.
Honestly, it never occured to me that this would not be desired. Like the other users who commented in Issue 8006 and Issue 16187, I was a bit dismayed to find out this wasn't implemented. I was operating under the assumption that the only reason binary assignments didn't work with I'm willing to follow whatever process you'd like to reach a resolution. Would you like me to write a justification in this pull request, make a forum post, write a DIP? |
|
What would be best is a forum post with links to this PR and the bugzilla issues. Also, a spec PR would be needed. BTW, I do think this is looking good. |
Thanks! I see the utility of those functions, but what is @MartinNowak suggesting the lowering be changed to? |
After reviewing the DIPs and their discussions, it appears that the property debacle with UFCS, optional parens, etc... has indeed been discussed extensively, but this specific issue of binary assignment operators for properties has not. The only serious mentions seem to be the prowiki article, and DIP23. Nevertheless, a DIP PR has been submitted at dlang/DIPs#97. At present it's still a work-in-progress, but it's coming together. |
|
Thanks @JinShil. This is great stuff. |
|
Should that really be a |
|
I need some help. I want to ensure that the getter and setter both contain an |
|
If there's only a getter, wouldn't that be a readonly property? |
Yes, but that's not what mean. Consider the expression
I just realized you may have been referring to the question above. The problem with that function is it has the signature of a setter (i.e. takes one argument), but is actually a getter. That is causing the algorithm in this PR to use it as a setter. The function can never be called with property syntax (e.g. no parens) so I question whether it should be marked with the |
I came up with this. I doubt it'll pass review, but it works. The meat and potatoes of the DIP is now implemented and tests are passing. I just need to redo the tests and finalize the DIP. |
Turned out to be moot anyway as |
|
The implementation now matches that which is proposed in the DIP. |
|
@WalterBright @andralex - are we progressing this? Is there a card for this in trello? |
According to a comment from @mdparker on Nov 14th, 2017 we are waiting on changes to the DIP process. @WalterBright and @andralex Can you please give @mdparker whatever support he needs to unplug the DIP queue?
I tried to add one, but I guess I don't have permission on the Trello board. I added a card here, though that probably won't do much if we don't all agree to use that GitHub feature. |
Sent out an invitation to you, but the Trello board isn't very active and only used by Martin, so it's probably better to give the GitHub board a try. |
|
@JinShil The hold up right now is me. I haven't finished revising the documentation yet. I'm almost there, though. |
|
FYI. I am abandoning this implementation and the DIP. I'm afraid my future with D is uncertain at best. If anyone wants to pick of the torch, it's yours. I'll keep the PRs open for now unless someone believes they should be closed. |
|
cc @thewilsonator |
14 participants
FYI: This does not address unary assignment operators (e.g.
s.x++,++s.x). I intend to implement that in a future pull request.DIP PR: dlang/DIPs#97
Spec change: dlang/dlang.org#1865