[SR-628][Sema] Improved handling of mixed lvalues & rvalues in tuple exprs #1150
Conversation
gregomni
changed the title
|
I haven't had a chance to look at this, but FWIW, I'd love to get to a model where LValueType was never embedded inside of a tuple. It doesn't make sense for a tuple to be partially mutable or not, because the tuple either occurs on the LHS of an assignment or not. If all the tuple members are lvalues, then the tuple should be an lvalue. If any are rvalues, then the whole thing should be rvalues. |
|
@swift-ci Please test |
|
@lattner I'm guessing it used to be such that calling a function with a mix of inout and non-inout arguments would create such a tuple, but this is no longer the case? |
|
We still represent arguments as tuples, but those will contain |
|
At the moment (before & after this commit), tuple expressions get created with individual element lvalue or rvalue types - just whatever the result type is of each expr inside. It should be the case in valid argument tuples that "&" converts LValueType to InOutType, so I don't think we lose anything from making the suggested restriction (except maybe slightly worse diagnostics or harder to make good diagnostics when missing "&"s), but it does effect a lot of places. I can look into it. |
|
This looks like it might be unrelated? Hmm. @jrose-apple ideas? |
|
Yeah, I ran swiftpm's tests and get no failures locally, so I'm not sure what's going on. I'm not that familiar with Jenkins or its output, though, so I'm not sure what's up beside it saying that that test fails. |
|
@mxcl Does that failure look familiar? Is swiftpm currently busted? (It seems unlikely that it's this change's fault.) |
|
@swift-ci Please test |
|
Apparently this is an unstable test in swiftpm? https://bugs.swift.org/browse/SR-712 |
|
@swift-ci Please test |
|
@gregomni It looks like this is still failing, can you see if maybe there's a regression that wasn't caught by the tests? |
|
@slavapestov I'll get everything up to date on this branch and take a look again, but that Build #22 that failed is still the one that ran back on Feb 1st. Maybe I need to push another change before it'll run that set of tests again... |
My previous commit here didn’t work correctly for nested tuples, both because it didn’t recurse into them to propagate access kind correctly and because an outer TupleIndex overload (when indexing into the nested tuple) could still be expecting an lvalue type. This fix is much better. ConstraintSystem::resolveOverload now correctly always expects rvalue types from rvalue tuples. And during applyMemberRefExpr, if the overload expects an rvalue but the tuple contains lvalues, coerceToType() correctly does any recursive munging of the tuple expr required.
|
We could just merge it and revert it if something breaks. :) |
|
Please just go ahead and rebase this on master and push it again. I'll merge it and keep an eye on the bots. |
gregomni
force-pushed
the
tuple-element-lvalue
branch
from
b30157e to
37d05ea
Compare
|
Oh good, that's what I just did. :) |
|
@swift-ci Please test |
[SR-628][Sema] Improved handling of mixed lvalues & rvalues in tuple exprs
|
@slavapestov Cool, thanks. |
My previous commit here didn’t work correctly for nested tuples, both because it didn’t recurse into them to propagate access kind correctly and because an outer TupleIndex overload (when indexing into the nested tuple) could still be expecting an lvalue type.
This fix is much better. ConstraintSystem::resolveOverload now correctly always expects rvalue types from rvalue tuples. And during applyMemberRefExpr, if the overload expects an rvalue but the tuple contains lvalues, coerceToType() can correctly do any recursive munging of the tuple expr required. Note the small change to coerceToType: it knows how to handle lvalue to rvalue tuple conversions correctly, but it was performing it incorrectly (too simplistically) for tuples if the solver recorded a knownRestriction.
Test added, tests pass. Could still use a better diagnosis for
(3,x).1 = 7- will look at that next.