From 2c0ac6943332177bf26592aa9a49385f86cb876d Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Mon, 27 Oct 2025 20:59:10 +0000 Subject: [PATCH 1/5] [CS] Add conversion for ErrorExpr's original expr Make sure we don't produce unnecessary diagnostics while still allowing things like cursor info to work. No test change since it's covered by the next commit. --- lib/Sema/CSGen.cpp | 13 ++++++++++++- lib/Sema/CSSimplify.cpp | 4 ++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp index 30abd5403f3c..7bfd90f593c9 100644 --- a/lib/Sema/CSGen.cpp +++ b/lib/Sema/CSGen.cpp @@ -1322,7 +1322,18 @@ namespace { } virtual Type visitErrorExpr(ErrorExpr *E) { - return recordInvalidNode(E); + auto ty = recordInvalidNode(E); + // If we have an original expression, introduce a conversion to the hole + // type of the ErrorExpr. This avoids unnecessary diagnostics for the + // original expression in cases where the ErrorExpr could have provided + // contextual info, while also still allowing the original expression to + // be solved without holes being introduced prematurely, allowing e.g + // cursor info to work correctly. + if (auto *orig = E->getOriginalExpr()) { + CS.addConstraint(ConstraintKind::Conversion, CS.getType(orig), ty, + CS.getConstraintLocator(E)); + } + return ty; } virtual Type visitCodeCompletionExpr(CodeCompletionExpr *E) { diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp index eb00690d08d7..9e41c972bdab 100644 --- a/lib/Sema/CSSimplify.cpp +++ b/lib/Sema/CSSimplify.cpp @@ -5582,6 +5582,10 @@ bool ConstraintSystem::repairFailures( if (!anchor) return false; + // If we have an ErrorExpr anchor, we don't need to do any more fixes. + if (isExpr(anchor)) + return true; + // This could be: // - `InOutExpr` used with r-value e.g. `foo(&x)` where `x` is a `let`. // - `ForceValueExpr` e.g. `foo.bar! = 42` where `bar` or `foo` are From ea79f675ff7fa1d52f4d6fef41419ad57792f6c1 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sat, 15 Nov 2025 11:03:36 +0000 Subject: [PATCH 2/5] [test] Convert `complete_return` to `batch-code-completion` --- test/IDE/complete_return.swift | 41 ++++++++++------------------------ 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/test/IDE/complete_return.swift b/test/IDE/complete_return.swift index 466b173deedb..cd01e6added4 100644 --- a/test/IDE/complete_return.swift +++ b/test/IDE/complete_return.swift @@ -1,21 +1,4 @@ -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_VOID_1 | %FileCheck %s -check-prefix=RETURN_VOID_1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_INT_1 | %FileCheck %s -check-prefix=RETURN_INT_1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_INT_2 | %FileCheck %s -check-prefix=RETURN_INT_2 - -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TRY_RETURN_INT | %FileCheck %s -check-prefix=RETURN_INT_1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TRY_RETURN_VOID | %FileCheck %s -check-prefix=RETURN_VOID_1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1 | %FileCheck %s -check-prefix=RETURN_TR1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2 | %FileCheck %s -check-prefix=RETURN_TR2 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3 | %FileCheck %s -check-prefix=RETURN_TR3 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1_METHOD | %FileCheck %s -check-prefix=RETURN_TR1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2_METHOD | %FileCheck %s -check-prefix=RETURN_TR2 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3_METHOD | %FileCheck %s -check-prefix=RETURN_TR3 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1_STATICMETHOD | %FileCheck %s -check-prefix=RETURN_TR1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2_STATICMETHOD | %FileCheck %s -check-prefix=RETURN_TR2 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3_STATICMETHOD | %FileCheck %s -check-prefix=RETURN_TR3 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR1_CLOSURE | %FileCheck %s -check-prefix=RETURN_TR1 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR2_CLOSURE | %FileCheck %s -check-prefix=RETURN_TR2 -// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=RETURN_TR3_CLOSURE | %FileCheck %s -check-prefix=RETURN_TR3 +// RUN: %batch-code-completion struct FooStruct { var instanceVar : Int @@ -64,11 +47,11 @@ func testReturnInt2(_ fooObject: FooStruct) { } func testMisplacedTry() throws -> Int { - try return #^TRY_RETURN_INT^# + try return #^TRY_RETURN_INT?check=RETURN_INT_1^# } func testMisplacedTryVoid() throws { - try return #^TRY_RETURN_VOID^# + try return #^TRY_RETURN_VOID?check=RETURN_VOID_1^# } func testTR1() -> Int? { @@ -111,26 +94,26 @@ struct TestStruct { var i : Int var oi : Int? var fs : FooStruct - return #^RETURN_TR1_METHOD^# + return #^RETURN_TR1_METHOD?check=RETURN_TR1^# } func testTR2_method(_ g : Gen) -> Int? { - return g.#^RETURN_TR2_METHOD^# + return g.#^RETURN_TR2_METHOD?check=RETURN_TR2^# } func testTR3_method(_ g : Gen) -> Int? { - return g.IG.#^RETURN_TR3_METHOD^# + return g.IG.#^RETURN_TR3_METHOD?check=RETURN_TR3^# } static func testTR1_static() -> Int? { var i : Int var oi : Int? var fs : FooStruct - return #^RETURN_TR1_STATICMETHOD^# + return #^RETURN_TR1_STATICMETHOD?check=RETURN_TR1^# } static func testTR2_static(_ g : Gen) -> Int? { - return g.#^RETURN_TR2_STATICMETHOD^# + return g.#^RETURN_TR2_STATICMETHOD?check=RETURN_TR2^# } static func testTR3_static(_ g : Gen) -> Int? { - return g.IG.#^RETURN_TR3_STATICMETHOD^# + return g.IG.#^RETURN_TR3_STATICMETHOD?check=RETURN_TR3^# } } @@ -140,12 +123,12 @@ func testClosures(_ g: Gen) { var fs : FooStruct _ = { () -> Int? in - return #^RETURN_TR1_CLOSURE^# + return #^RETURN_TR1_CLOSURE?check=RETURN_TR1^# } _ = { () -> Int? in - return g.#^RETURN_TR2_CLOSURE^# + return g.#^RETURN_TR2_CLOSURE?check=RETURN_TR2^# } _ = { () -> Int? in - return g.IG.#^RETURN_TR3_CLOSURE^# + return g.IG.#^RETURN_TR3_CLOSURE?check=RETURN_TR3^# } } From e7f5ca954bc5ce32e5bfc959f7b290cfab4b8d8c Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Wed, 5 Nov 2025 19:33:19 +0000 Subject: [PATCH 3/5] [Sema] Better handle recovery for structurally invalid ReturnStmts Make sure we preserve the result expression for an out-of-place `return`, or a non-`nil` result in an initializer. This ensures we can still provide semantic functionality from them and fixes a crash where we would fail to type-check a binding. --- lib/Sema/TypeCheckStmt.cpp | 35 +++++++++++++------ test/IDE/complete_return.swift | 14 ++++++++ test/stmt/statements.swift | 2 ++ ...nstraintSystem-getClosureType-aac79a.swift | 2 +- 4 files changed, 42 insertions(+), 11 deletions(-) rename validation-test/{compiler_crashers => compiler_crashers_fixed}/ConstraintSystem-getClosureType-aac79a.swift (81%) diff --git a/lib/Sema/TypeCheckStmt.cpp b/lib/Sema/TypeCheckStmt.cpp index ddea97a1087c..9584ec27111e 100644 --- a/lib/Sema/TypeCheckStmt.cpp +++ b/lib/Sema/TypeCheckStmt.cpp @@ -1091,12 +1091,14 @@ class StmtChecker : public StmtVisitor { auto &eval = getASTContext().evaluator; auto *S = evaluateOrDefault(eval, PreCheckReturnStmtRequest{RS, DC}, nullptr); + if (!S) + return nullptr; - // We do a cast here as it may have been turned into a FailStmt. We should - // return that without doing anything else. - RS = dyn_cast_or_null(S); + // We do a cast here as we may now have a different stmt (e.g a FailStmt, or + // a BraceStmt for error cases). If so, recurse into the visitor. + RS = dyn_cast(S); if (!RS) - return S; + return visit(S); auto TheFunc = AnyFunctionRef::fromDeclContext(DC); assert(TheFunc && "Should have bailed from pre-check if this is None"); @@ -1785,16 +1787,31 @@ Stmt *PreCheckReturnStmtRequest::evaluate(Evaluator &evaluator, ReturnStmt *RS, auto &ctx = DC->getASTContext(); auto fn = AnyFunctionRef::fromDeclContext(DC); + auto errorResult = [&]() -> Stmt * { + // We don't need any recovery if there's no result, just bail. + if (!RS->hasResult()) + return nullptr; + + // If we have a result, make sure it's preserved, insert an implicit brace + // with a wrapping error expression. This ensures we can still do semantic + // functionality, and avoids downstream crashes where we expect the + // expression to have been type-checked. + auto *result = RS->getResult(); + RS->setResult(nullptr); + auto *err = new (ctx) ErrorExpr(result->getSourceRange(), Type(), result); + return BraceStmt::createImplicit(ctx, {err}); + }; + // Not valid outside of a function. if (!fn) { ctx.Diags.diagnose(RS->getReturnLoc(), diag::return_invalid_outside_func); - return nullptr; + return errorResult(); } // If the return is in a defer, then it isn't valid either. if (isDefer(DC)) { ctx.Diags.diagnose(RS->getReturnLoc(), diag::jump_out_of_defer, "return"); - return nullptr; + return errorResult(); } // The rest of the checks only concern return statements with results. @@ -1815,8 +1832,7 @@ Stmt *PreCheckReturnStmtRequest::evaluate(Evaluator &evaluator, ReturnStmt *RS, if (!nilExpr) { ctx.Diags.diagnose(RS->getReturnLoc(), diag::return_init_non_nil) .highlight(E->getSourceRange()); - RS->setResult(nullptr); - return RS; + return errorResult(); } // "return nil" is only permitted in a failable initializer. @@ -1826,8 +1842,7 @@ Stmt *PreCheckReturnStmtRequest::evaluate(Evaluator &evaluator, ReturnStmt *RS, ctx.Diags .diagnose(ctor->getLoc(), diag::make_init_failable, ctor) .fixItInsertAfter(ctor->getLoc(), "?"); - RS->setResult(nullptr); - return RS; + return errorResult(); } // Replace the "return nil" with a new 'fail' statement. diff --git a/test/IDE/complete_return.swift b/test/IDE/complete_return.swift index cd01e6added4..5e28a85b9ab2 100644 --- a/test/IDE/complete_return.swift +++ b/test/IDE/complete_return.swift @@ -132,3 +132,17 @@ func testClosures(_ g: Gen) { return g.IG.#^RETURN_TR3_CLOSURE?check=RETURN_TR3^# } } + +// Make sure we can do a completion in an out-of-place return +do { + return TestStruct.#^COMPLETE_IN_INVALID_RETURN^# + // COMPLETE_IN_INVALID_RETURN: Decl[StaticMethod]/CurrNominal: testTR1_static()[#Int?#]; name=testTR1_static() + // COMPLETE_IN_INVALID_RETURN: Decl[StaticMethod]/CurrNominal: testTR2_static({#(g): Gen#})[#Int?#]; name=testTR2_static(:) + // COMPLETE_IN_INVALID_RETURN: Decl[StaticMethod]/CurrNominal: testTR3_static({#(g): Gen#})[#Int?#]; name=testTR3_static(:) +} + +struct TestReturnInInit { + init() { + return TestStruct.#^COMPLETE_IN_INVALID_INIT_RETURN?check=COMPLETE_IN_INVALID_RETURN^# + } +} diff --git a/test/stmt/statements.swift b/test/stmt/statements.swift index 566599474bc9..4c604127b790 100644 --- a/test/stmt/statements.swift +++ b/test/stmt/statements.swift @@ -179,6 +179,8 @@ return 42 // expected-error {{return invalid outside of a func}} return // expected-error {{return invalid outside of a func}} +return VoidReturn1() // expected-error {{return invalid outside of a func}} + func NonVoidReturn1() -> Int { _ = 0 return // expected-error {{non-void function should return a value}} diff --git a/validation-test/compiler_crashers/ConstraintSystem-getClosureType-aac79a.swift b/validation-test/compiler_crashers_fixed/ConstraintSystem-getClosureType-aac79a.swift similarity index 81% rename from validation-test/compiler_crashers/ConstraintSystem-getClosureType-aac79a.swift rename to validation-test/compiler_crashers_fixed/ConstraintSystem-getClosureType-aac79a.swift index 313b604bae0c..806102beb1aa 100644 --- a/validation-test/compiler_crashers/ConstraintSystem-getClosureType-aac79a.swift +++ b/validation-test/compiler_crashers_fixed/ConstraintSystem-getClosureType-aac79a.swift @@ -1,5 +1,5 @@ // {"kind":"typecheck","signature":"swift::constraints::ConstraintSystem::getClosureType(swift::ClosureExpr const*) const","signatureAssert":"Assertion failed: (result), function getClosureType"} -// RUN: not --crash %target-swift-frontend -typecheck %s +// RUN: not %target-swift-frontend -typecheck %s return { lazy var a = if .random() { return } } From 192303750220d9dcd51ab6260914ac6452052ed5 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Mon, 10 Nov 2025 10:40:35 +0000 Subject: [PATCH 4/5] [CS] Invalidate unresolved variables in ExprPatterns early Make sure we invalidate when we initially visit the ExprPattern to ensure that we don't run into issues when generating constraints for a `where` clause before the constraints for the ExprPattern. We ought to change the constraint generation there to use a conjunction to ensure that the pattern is solved before the `where` clause, but I want to keep this as a quick low risk fix that we can cherry-pick. I'll switch it to a conjunction in a follow-up. --- lib/Sema/CSGen.cpp | 10 ++++++++++ ...ConstraintSystem-getTypeOfReferencePre-60045c.swift | 9 +++++++++ .../TypeVarRefCollector-walkToStmtPre-e56ccf.swift | 10 ++++++++++ 3 files changed, 29 insertions(+) create mode 100644 validation-test/compiler_crashers_fixed/ConstraintSystem-getTypeOfReferencePre-60045c.swift create mode 100644 validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-e56ccf.swift diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp index 7bfd90f593c9..48ceec289c4a 100644 --- a/lib/Sema/CSGen.cpp +++ b/lib/Sema/CSGen.cpp @@ -3167,6 +3167,16 @@ namespace { } case PatternKind::Expr: { + // Make sure we invalidate any nested VarDecls early since generating + // constraints for a `where` clause may happen before we've generated + // constraints for the ExprPattern. We'll record a fix when visiting + // the UnresolvedPatternExpr. + // FIXME: We ought to use a conjunction for switch cases, then we + // wouldn't need this logic. + auto *EP = cast(pattern); + EP->getSubExpr()->forEachUnresolvedVariable([&](VarDecl *VD) { + CS.setType(VD, ErrorType::get(CS.getASTContext())); + }); // We generate constraints for ExprPatterns in a separate pass. For // now, just create a type variable. return setType(CS.createTypeVariable(CS.getConstraintLocator(locator), diff --git a/validation-test/compiler_crashers_fixed/ConstraintSystem-getTypeOfReferencePre-60045c.swift b/validation-test/compiler_crashers_fixed/ConstraintSystem-getTypeOfReferencePre-60045c.swift new file mode 100644 index 000000000000..aece4ac77ecd --- /dev/null +++ b/validation-test/compiler_crashers_fixed/ConstraintSystem-getTypeOfReferencePre-60045c.swift @@ -0,0 +1,9 @@ +// {"kind":"typecheck","signature":"swift::constraints::ConstraintSystem::getTypeOfReferencePre(swift::constraints::OverloadChoice, swift::DeclContext*, swift::constraints::ConstraintLocatorBuilder, swift::constraints::PreparedOverloadBuilder*)","signatureAssert":"Assertion failed: (func->isOperator() && \"Lookup should only find operators\"), function getTypeOfReferencePre"} +// RUN: not %target-swift-frontend -typecheck %s +{ + switch if .random() else { + } + { + case let !a where a: + } +} diff --git a/validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-e56ccf.swift b/validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-e56ccf.swift new file mode 100644 index 000000000000..2f88c27677f6 --- /dev/null +++ b/validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-e56ccf.swift @@ -0,0 +1,10 @@ +// {"kind":"typecheck","signature":"swift::constraints::TypeVarRefCollector::walkToStmtPre(swift::Stmt*)","signatureAssert":"Assertion failed: (result), function getClosureType"} +// RUN: not %target-swift-frontend -typecheck %s +{ + switch if <#expression#> { + return + } + { + case let !a where a: + } +} From ff833daab4a058e84ba782f6685de90b5e35984e Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Wed, 5 Nov 2025 19:33:19 +0000 Subject: [PATCH 5/5] [ASTScopes] Remove ErrorExpr check from condition scope expansion This appears unnecessary and incorrectly meant the ErrorExpr would be placed outside the condition, meaning we could find the binding in its own initializer. --- lib/AST/ASTScopeCreation.cpp | 7 ++----- .../TypeVarRefCollector-walkToStmtPre-5c070f.swift | 7 +++++++ 2 files changed, 9 insertions(+), 5 deletions(-) create mode 100644 validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-5c070f.swift diff --git a/lib/AST/ASTScopeCreation.cpp b/lib/AST/ASTScopeCreation.cpp index d95675b40b24..b947c44dcc8d 100644 --- a/lib/AST/ASTScopeCreation.cpp +++ b/lib/AST/ASTScopeCreation.cpp @@ -959,11 +959,8 @@ AnnotatedInsertionPoint ConditionalClausePatternUseScope::expandAScopeThatCreatesANewInsertionPoint( ScopeCreator &scopeCreator) { auto *initializer = sec.getInitializer(); - if (!isa(initializer)) { - scopeCreator - .constructExpandAndInsert( - this, initializer); - } + scopeCreator.constructExpandAndInsert( + this, initializer); return {this, "Succeeding code must be in scope of conditional clause pattern bindings"}; diff --git a/validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-5c070f.swift b/validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-5c070f.swift new file mode 100644 index 000000000000..a714f8547e0b --- /dev/null +++ b/validation-test/compiler_crashers_fixed/TypeVarRefCollector-walkToStmtPre-5c070f.swift @@ -0,0 +1,7 @@ +// {"kind":"typecheck","signature":"swift::constraints::TypeVarRefCollector::walkToStmtPre(swift::Stmt*)","signatureAssert":"Assertion failed: (result), function getClosureType"} +// RUN: not %target-swift-frontend -typecheck %s +{ + guard let a = (a if{ + return + } +) "