Sema: Don't coerce subexpression of 'try' to rvalue #85183
Conversation
There was a problem hiding this comment.
I think this also needs CSGen changes to work correctly, e.g we don't want to produce an lvalue here:
var x = 0
(try! x) += 1If we see that the sub-expression can bind to an lvalue I think we need to produce a fresh type variable that isn't TVO_CanBindToLValue and add an Equal constraint
The change in 8ab8b2e, was too broad. We want to coerce the subexpression of `try!` to an rvalue, but not the subexpression of a `try`. If the subexpression of a `try` becomes an rvalue even though the type of the parent expression is an lvalue, we can end up with infinite recursion in coerceToType(), as demonstrated by the test case. Fixes swiftlang#85034.
slavapestov
force-pushed
the
fix-issue-85034
branch
from
366a0ba to
d1c4d78
Compare
|
@hamishknight Actually it looks like the Indeed, the original test case only passed |
It seems that we end up with a null Type in one place in StmtChecker::visitSwitchStmt(), so add a null check there, and a FIXME to investigate further.
|
@slavapestov I suspect the updated code will still crash for the newly added test case if you change |
|
Indeed you're right, I'll see about fixing CSGen for |
slavapestov
force-pushed
the
fix-issue-85034
branch
from
d1c4d78 to
8fd7a1d
Compare
|
@hamishknight Thank you for the detailed review, that was really helpful! |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@hamishknight It looks like for |
| TVO_CanBindToNoEscape); | ||
| CS.addConstraint(ConstraintKind::Equal, valueTy, | ||
| CS.getType(expr->getSubExpr()), | ||
| CS.getConstraintLocator(expr)); |
There was a problem hiding this comment.
@hamishknight this makes me wonder - instead of trying to load a sub-expr of a try, maybe we should be instead make sure that each element of the tuple expression is r-value which would in turn make sure that try is loaded as well...
There was a problem hiding this comment.
we should probably disallow tuple types that contain lvalues, or at least a mix of rvalues and lvalues, but that's a bigger change...
There was a problem hiding this comment.
I see the following is rejected, so I'm wondering if its possible for lvalue to appear inside a tuple type in any case where the entire tuple isn't an lvalue:
func f() {
var x = 0
g(&(x, "hi").0)
}
func g(_: inout Int) {}
There was a problem hiding this comment.
Certainly (x, y) = (y, x) and so on is fine, but that can be modeled as @lvalue (Int, Int) instead of (@lvalue Int, @lvalue Int).
There was a problem hiding this comment.
Yeah, if I try (0, x.x) I see that the member reference is loaded. I haven't actually tested, but based on the discussion, it sounds like the "type mismatch" in the example is related to the fact that the type of a tuple element is l-value but in tuple expression when the actual expression has been loaded already by the logic in visitAnyTryExpr, in this case I'd say that CSGen should make sure that the elements are r-values and that would allow us to remove some of the loading for CSApply that handles this kind of transform post-factum...
There was a problem hiding this comment.
this makes me wonder - instead of trying to load a sub-expr of a try, maybe we should be instead make sure that each element of the tuple expression is r-value which would in turn make sure that try is loaded as well...
I don't think this is specific to tuples, I think you'd run into similar issues for things like (try! x), if x is an lvalue and we don't enforce that the type of the try! is an rvalue in CSGen then the ParenExpr is going to have an lvalue type in the solution which CSApply will happily assign and we'll crash even if we have coerced x to an rvalue.
It agree it would be nice if we could stop modeling tuple elements as lvalues though, maybe at some point in the future we could even stop modeling lvalues at all in the constraint system :)
There was a problem hiding this comment.
I don't think this is specific to tuples
@hamishknight Isn't the problem with infinite coerceToType is specific to tuple elements? I brought it up because I think we should be enforcing this higher up if the assessment in my previous comment is correct. It seems like the fundamental issue here is that we end up with a type that contains l-value(s) i.e. (Int, @lvalue Int) coming out of CSGen but then later we coerce each element type to r-value (that's what I noticed with (0, x.x)) in CSApply somewhere, regardless of whether r-value is necessary for a try! or not this shouldn't have produced a solution in my opinion and we don't actually know in what other cases we might be in the same situation were we'd need to use a non-lvalue type variable for some other expression.
3 participants
Undo the change in 8ab8b2e, which was an incorrect fix for some other bug which has since been fixed properly.
If this becomes an rvalue even though the type of the surrounding expression is an lvalue, we can end up with infinite recursion in coerceToType(), as demonstrated by the test case.
Fixes #85034.