From 1e3dd5c50c415fa976de18d5b47a2473a7d713bb Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Tue, 8 Aug 2023 18:08:51 +0100 Subject: [PATCH 1/2] [AST] Improve source range info for TapExpr Previously we would only base the start loc on the `SubExpr`, but that isn't set until CSApply. Change it to take both `SubExpr` and `Body`'s source range into account. Also tighten up the invariant that a TapExpr must be created with a non-null BraceStmt. --- include/swift/AST/Expr.h | 6 +----- lib/AST/Expr.cpp | 38 +++++++++++++++++++++----------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h index c37dc40cb38f..ed0f2b93cfab 100644 --- a/include/swift/AST/Expr.h +++ b/include/swift/AST/Expr.h @@ -880,12 +880,8 @@ class TapExpr : public Expr { void setBody(BraceStmt * b) { Body = b; } SourceLoc getLoc() const { return SubExpr ? SubExpr->getLoc() : SourceLoc(); } - - SourceLoc getStartLoc() const { - return SubExpr ? SubExpr->getStartLoc() : SourceLoc(); - } - SourceLoc getEndLoc() const; + SourceRange getSourceRange() const; static bool classof(const Expr *E) { return E->getKind() == ExprKind::Tap; diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index 3f16c42a1b98..417320aed42c 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -2611,29 +2611,33 @@ void InterpolatedStringLiteralExpr::forEachSegment(ASTContext &Ctx, TapExpr::TapExpr(Expr * SubExpr, BraceStmt *Body) : Expr(ExprKind::Tap, /*Implicit=*/true), SubExpr(SubExpr), Body(Body) { - if (Body) { - assert(!Body->empty() && - Body->getFirstElement().isDecl(DeclKind::Var) && - "First element of Body should be a variable to init with the subExpr"); - } + assert(Body); + assert(!Body->empty() && + Body->getFirstElement().isDecl(DeclKind::Var) && + "First element of Body should be a variable to init with the subExpr"); } VarDecl * TapExpr::getVar() const { return dyn_cast(Body->getFirstElement().dyn_cast()); } -SourceLoc TapExpr::getEndLoc() const { - // Include the body in the range, assuming the body follows the SubExpr. - // Also, be (perhaps overly) defensive about null pointers & invalid - // locations. - if (auto *const b = getBody()) { - const auto be = b->getEndLoc(); - if (be.isValid()) - return be; - } - if (auto *const se = getSubExpr()) - return se->getEndLoc(); - return SourceLoc(); +SourceRange TapExpr::getSourceRange() const { + if (!SubExpr) + return Body->getSourceRange(); + + SourceLoc start = SubExpr->getStartLoc(); + if (!start.isValid()) + start = Body->getStartLoc(); + if (!start.isValid()) + return SourceRange(); + + SourceLoc end = Body->getEndLoc(); + if (!end.isValid()) + end = SubExpr->getEndLoc(); + if (!end.isValid()) + return SourceRange(); + + return SourceRange(start, end); } RegexLiteralExpr * From bc31eb1595f8b6dc6616fe7fae7506467366fd0d Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Tue, 8 Aug 2023 18:08:53 +0100 Subject: [PATCH 2/2] [CS] Solve all conjunctions in source order Previously we would only do source ordering for ClosureExprs, but other conjunctions need to have their source location taken into account too, in order to make sure we don't try and type-check e.g a TapExpr in a second closure before we type-check the first closure. Also while here, switch to `std::min_element` instead of sorting, and treat invalid source locations as incomparable. rdar://113326835 --- lib/Sema/CSSolver.cpp | 30 ++++++++++-------- test/Constraints/rdar113326835.swift | 46 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 test/Constraints/rdar113326835.swift diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp index bd6360f31d20..37931db76303 100644 --- a/lib/Sema/CSSolver.cpp +++ b/lib/Sema/CSSolver.cpp @@ -2438,25 +2438,31 @@ Constraint *ConstraintSystem::selectConjunction() { auto &SM = getASTContext().SourceMgr; - // All of the multi-statement closures should be solved in order of their - // apperance in the source. - llvm::sort( - conjunctions, [&](Constraint *conjunctionA, Constraint *conjunctionB) { + // Conjunctions should be solved in order of their apperance in the source. + // This is important because once a conjunction is solved, we don't re-visit + // it, so we need to make sure we don't solve it before another conjuntion + // that could provide it with necessary type information. Source order + // provides an easy to reason about and quick way of establishing this. + return *std::min_element( + conjunctions.begin(), conjunctions.end(), + [&](Constraint *conjunctionA, Constraint *conjunctionB) { auto *locA = conjunctionA->getLocator(); auto *locB = conjunctionB->getLocator(); - if (!(locA && locB)) return false; - auto *closureA = getAsExpr(locA->getAnchor()); - auto *closureB = getAsExpr(locB->getAnchor()); + auto anchorA = locA->getAnchor(); + auto anchorB = locB->getAnchor(); + if (!(anchorA && anchorB)) + return false; - return closureA && closureB - ? SM.isBeforeInBuffer(closureA->getLoc(), closureB->getLoc()) - : false; - }); + auto slocA = anchorA.getStartLoc(); + auto slocB = anchorB.getStartLoc(); + if (!(slocA.isValid() && slocB.isValid())) + return false; - return conjunctions.front(); + return SM.isBeforeInBuffer(slocA, slocB); + }); } bool DisjunctionChoice::attempt(ConstraintSystem &cs) const { diff --git a/test/Constraints/rdar113326835.swift b/test/Constraints/rdar113326835.swift new file mode 100644 index 000000000000..95ae44692202 --- /dev/null +++ b/test/Constraints/rdar113326835.swift @@ -0,0 +1,46 @@ +// RUN: %target-typecheck-verify-swift + +// rdar://113326835 - Make sure we type-check the conjunctions in source order, +// the first closure should be type-checked before we attempt the +// TapExpr/SingleValueExpr conjunctions, since they rely on 'k' being resolved. + +func global(_ x: T) -> String { "" } +func global(_ x: Any.Type) -> String { "" } + +protocol P { + associatedtype X +} + +struct Q: P { + init() {} + func bar(_: String) -> Self { fatalError() } + func qux(_: (X) -> U) -> Q { fatalError() } +} + +struct J: P { + init(_: X) {} + func baz(_ transform: (X) -> T) -> Q { fatalError() } +} + +func foo(a: Int) -> Q { + J(a) + .baz { x in + () + return a + } + .qux { k in + Q().bar("\(k)") + } +} + +func bar(a: Int) -> Q { + J(a) + .baz { x in + () + return a + } + .qux { k in + Q().bar(if .random() { global(k) } else { global(k) }) + // expected-error@-1 {{'if' may only be used as expression in return, throw, or as the source of an assignment}} + } +}