Skip to content

Sema: Don't coerce subexpression of 'try' to rvalue #85183

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 29, 2025
Merged

Sema: Don't coerce subexpression of 'try' to rvalue #85183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97f7efd
Sema: rdar://r78102266 was a SILGen crash so fix the test to pass -em…
slavapestov Oct 28, 2025
f014a9a
Sema: Don't coerce subexpression of 'try' to rvalue
slavapestov Oct 28, 2025
5c6a526
Sema: Replace early return with ASSERT
slavapestov Oct 28, 2025
8fd7a1d
Sema: Fix CSGen for ForceTryExpr
slavapestov Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3752,7 +3752,11 @@ namespace {
return expr;
}

Expr *visitAnyTryExpr(AnyTryExpr *expr) {
Expr *visitTryExpr(TryExpr *expr) {
return simplifyExprType(expr);
}

Expr *visitForceTryExpr(ForceTryExpr *expr) {
auto *subExpr = expr->getSubExpr();
auto type = simplifyType(cs.getType(subExpr));

Expand Down
12 changes: 11 additions & 1 deletion lib/Sema/CSGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2060,10 +2060,20 @@ namespace {
return valueTy;
}

Type visitAnyTryExpr(AnyTryExpr *expr) {
Type visitTryExpr(AnyTryExpr *expr) {
return CS.getType(expr->getSubExpr());
}

Type visitForceTryExpr(AnyTryExpr *expr) {
auto valueTy = CS.createTypeVariable(CS.getConstraintLocator(expr),
TVO_PrefersSubtypeBinding |
TVO_CanBindToNoEscape);
CS.addConstraint(ConstraintKind::Equal, valueTy,
CS.getType(expr->getSubExpr()),
CS.getConstraintLocator(expr));
Copy link
Contributor

@xedin xedin Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Copy link
Contributor Author

@slavapestov slavapestov Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably disallow tuple types that contain lvalues, or at least a mix of rvalues and lvalues, but that's a bigger change...

Copy link
Contributor Author

@slavapestov slavapestov Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {}

Copy link
Contributor Author

@slavapestov slavapestov Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@xedin xedin Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

@hamishknight hamishknight Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

@xedin xedin Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return valueTy;
}

Type visitOptionalTryExpr(OptionalTryExpr *expr) {
auto valueTy = CS.createTypeVariable(CS.getConstraintLocator(expr),
TVO_PrefersSubtypeBinding |
Expand Down
5 changes: 1 addition & 4 deletions lib/Sema/TypeCheckConstraints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,10 +1192,7 @@ TypeChecker::coerceToRValue(ASTContext &Context, Expr *expr,
llvm::function_ref<Type(Expr *)> getType,
llvm::function_ref<void(Expr *, Type)> setType) {
Type exprTy = getType(expr);

// If expr has no type, just assume it's the right expr.
if (!exprTy)
return expr;
ASSERT(exprTy);

// If the type is already materializable, then we're already done.
if (!exprTy->hasLValueType())
Expand Down
7 changes: 4 additions & 3 deletions lib/Sema/TypeCheckStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1702,10 +1702,11 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
// Type-check the subject expression.
Expr *subjectExpr = switchStmt->getSubjectExpr();
auto resultTy = TypeChecker::typeCheckExpression(subjectExpr, DC);

auto limitExhaustivityChecks = !resultTy;
if (Expr *newSubjectExpr =
TypeChecker::coerceToRValue(getASTContext(), subjectExpr))
subjectExpr = newSubjectExpr;
if (resultTy)
subjectExpr = TypeChecker::coerceToRValue(getASTContext(), subjectExpr);

switchStmt->setSubjectExpr(subjectExpr);
Type subjectType = switchStmt->getSubjectExpr()->getType();

Expand Down
2 changes: 1 addition & 1 deletion validation-test/Sema/type_checker_crashers_fixed/rdar78102266.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-swift-frontend %s -typecheck
// RUN: %target-swift-emit-silgen %s

struct Info {
}
Expand Down
12 changes: 12 additions & 0 deletions validation-test/compiler_crashers_fixed/issue-85034.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-swift-emit-silgen %s

class C {
var x = 0
}

do {
let x = C()
let _ = (0, try x.x) // expected-warning {{no calls to throwing functions occur within 'try' expression}}
let _ = (0, try! x.x) // expected-warning {{no calls to throwing functions occur within 'try' expression}}
}