From 5d93006d4caa77ca3371a27bb2d68df0e468b59e Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Mon, 14 Jul 2025 17:07:08 +0100 Subject: [PATCH 1/2] [Sema] Avoid folding sequences multiple times for completion Completion can end up calling into pre-checking multiple times in certain cases, make sure we don't attempt to fold a SequenceExpr multiple times since its original AST is in a broken state post-folding. Instead, just return the already-folded expression. rdar://133717866 --- lib/Sema/TypeCheckExpr.cpp | 37 +++++++------------ .../IDE/issues_fixed/issue-75845.swift | 17 +++++++++ 2 files changed, 30 insertions(+), 24 deletions(-) create mode 100644 validation-test/IDE/issues_fixed/issue-75845.swift diff --git a/lib/Sema/TypeCheckExpr.cpp b/lib/Sema/TypeCheckExpr.cpp index 9a5bb2e0fbbd4..62f2c91f8133f 100644 --- a/lib/Sema/TypeCheckExpr.cpp +++ b/lib/Sema/TypeCheckExpr.cpp @@ -288,12 +288,7 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *ternary = dyn_cast(Op)) { // Resolve the ternary expression. - if (!Ctx.CompletionCallback) { - // In code completion we might call preCheckTarget twice - once for - // the first pass and once for the second pass. This is fine since - // preCheckTarget is idempotent. - assert(!ternary->isFolded() && "already folded if expr in sequence?!"); - } + ASSERT(!ternary->isFolded() && "already folded if expr in sequence?!"); ternary->setCondExpr(LHS); ternary->setElseExpr(RHS); return ternary; @@ -301,12 +296,7 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *assign = dyn_cast(Op)) { // Resolve the assignment expression. - if (!Ctx.CompletionCallback) { - // In code completion we might call preCheckTarget twice - once for - // the first pass and once for the second pass. This is fine since - // preCheckTarget is idempotent. - assert(!assign->isFolded() && "already folded assign expr in sequence?!"); - } + ASSERT(!assign->isFolded() && "already folded assign expr in sequence?!"); assign->setDest(LHS); assign->setSrc(RHS); return assign; @@ -314,12 +304,7 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *as = dyn_cast(Op)) { // Resolve the 'as' or 'is' expression. - if (!Ctx.CompletionCallback) { - // In code completion we might call preCheckTarget twice - once for - // the first pass and once for the second pass. This is fine since - // preCheckTarget is idempotent. - assert(!as->isFolded() && "already folded 'as' expr in sequence?!"); - } + ASSERT(!as->isFolded() && "already folded 'as' expr in sequence?!"); assert(RHS == as && "'as' with non-type RHS?!"); as->setSubExpr(LHS); return as; @@ -327,12 +312,7 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *arrow = dyn_cast(Op)) { // Resolve the '->' expression. - if (!Ctx.CompletionCallback) { - // In code completion we might call preCheckTarget twice - once for - // the first pass and once for the second pass. This is fine since - // preCheckTarget is idempotent. - assert(!arrow->isFolded() && "already folded '->' expr in sequence?!"); - } + ASSERT(!arrow->isFolded() && "already folded '->' expr in sequence?!"); arrow->setArgsExpr(LHS); arrow->setResultExpr(RHS); return arrow; @@ -633,6 +613,15 @@ swift::DefaultTypeRequest::evaluate(Evaluator &evaluator, } Expr *TypeChecker::foldSequence(SequenceExpr *expr, DeclContext *dc) { + // We may end up running pre-checking multiple times for completion, just use + // the folded expression if we've already folded the sequence. + // FIXME: We ought to fix completion to not pre-check multiple times, strictly + // speaking it isn't idempotent (e.g for things like `markDirectCallee`). + if (auto *folded = expr->getFoldedExpr()) { + ASSERT(dc->getASTContext().CompletionCallback && + "Attempting to fold sequence twice?"); + return folded; + } // First resolve any unresolved decl references in operator positions. for (auto i : indices(expr->getElements())) { if (i % 2 == 0) diff --git a/validation-test/IDE/issues_fixed/issue-75845.swift b/validation-test/IDE/issues_fixed/issue-75845.swift new file mode 100644 index 0000000000000..41b0c529b0660 --- /dev/null +++ b/validation-test/IDE/issues_fixed/issue-75845.swift @@ -0,0 +1,17 @@ +// RUN: %batch-code-completion + +// https://github.com/apple/swift/issues/75845 +// Make sure we don't crash. + +struct Foo { + init() { + do { + } catch { + #^A^#self#^B^# = #^C^#error#^D^# + } + } +} +// A: Decl[LocalVar]/Local: error[#any Error#]; name=error +// B: Begin completions +// C: Decl[LocalVar]/Local: error[#any Error#]; name=error +// D: Begin completions From e653a22431011c381882d4e4dfff768aa1707b31 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Mon, 14 Jul 2025 17:07:08 +0100 Subject: [PATCH 2/2] [Sema] Make change less risky for 6.2 Revert assertion changes and flip the condition to only apply when doing completion. --- lib/Sema/TypeCheckExpr.cpp | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/lib/Sema/TypeCheckExpr.cpp b/lib/Sema/TypeCheckExpr.cpp index 62f2c91f8133f..8de6d67f708ff 100644 --- a/lib/Sema/TypeCheckExpr.cpp +++ b/lib/Sema/TypeCheckExpr.cpp @@ -288,7 +288,12 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *ternary = dyn_cast(Op)) { // Resolve the ternary expression. - ASSERT(!ternary->isFolded() && "already folded if expr in sequence?!"); + if (!Ctx.CompletionCallback) { + // In code completion we might call preCheckTarget twice - once for + // the first pass and once for the second pass. This is fine since + // preCheckTarget is idempotent. + assert(!ternary->isFolded() && "already folded if expr in sequence?!"); + } ternary->setCondExpr(LHS); ternary->setElseExpr(RHS); return ternary; @@ -296,7 +301,12 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *assign = dyn_cast(Op)) { // Resolve the assignment expression. - ASSERT(!assign->isFolded() && "already folded assign expr in sequence?!"); + if (!Ctx.CompletionCallback) { + // In code completion we might call preCheckTarget twice - once for + // the first pass and once for the second pass. This is fine since + // preCheckTarget is idempotent. + assert(!assign->isFolded() && "already folded assign expr in sequence?!"); + } assign->setDest(LHS); assign->setSrc(RHS); return assign; @@ -304,7 +314,12 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *as = dyn_cast(Op)) { // Resolve the 'as' or 'is' expression. - ASSERT(!as->isFolded() && "already folded 'as' expr in sequence?!"); + if (!Ctx.CompletionCallback) { + // In code completion we might call preCheckTarget twice - once for + // the first pass and once for the second pass. This is fine since + // preCheckTarget is idempotent. + assert(!as->isFolded() && "already folded 'as' expr in sequence?!"); + } assert(RHS == as && "'as' with non-type RHS?!"); as->setSubExpr(LHS); return as; @@ -312,7 +327,12 @@ static Expr *makeBinOp(ASTContext &Ctx, Expr *Op, Expr *LHS, Expr *RHS, if (auto *arrow = dyn_cast(Op)) { // Resolve the '->' expression. - ASSERT(!arrow->isFolded() && "already folded '->' expr in sequence?!"); + if (!Ctx.CompletionCallback) { + // In code completion we might call preCheckTarget twice - once for + // the first pass and once for the second pass. This is fine since + // preCheckTarget is idempotent. + assert(!arrow->isFolded() && "already folded '->' expr in sequence?!"); + } arrow->setArgsExpr(LHS); arrow->setResultExpr(RHS); return arrow; @@ -617,10 +637,9 @@ Expr *TypeChecker::foldSequence(SequenceExpr *expr, DeclContext *dc) { // the folded expression if we've already folded the sequence. // FIXME: We ought to fix completion to not pre-check multiple times, strictly // speaking it isn't idempotent (e.g for things like `markDirectCallee`). - if (auto *folded = expr->getFoldedExpr()) { - ASSERT(dc->getASTContext().CompletionCallback && - "Attempting to fold sequence twice?"); - return folded; + if (dc->getASTContext().CompletionCallback) { + if (auto *folded = expr->getFoldedExpr()) + return folded; } // First resolve any unresolved decl references in operator positions. for (auto i : indices(expr->getElements())) {