Skip to content

Commit a12ac41

Browse files
xedinxedin
authored
Merge pull request swiftlang#33509 from xedin/rdar-65320500
[FunctionBuilders] Implement graceful handling of pre-check failures
2 parents 17e70c8 + 56ef379 commit a12ac41

15 files changed

+233
-47
lines changed

‎include/swift/AST/DiagnosticEngine.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,12 @@ namespace swift {
10581058
}
10591059
}
10601060

1061+
bool hasDiagnostics() const {
1062+
return std::distance(Engine.TentativeDiagnostics.begin() +
1063+
PrevDiagnostics,
1064+
Engine.TentativeDiagnostics.end()) > 0;
1065+
}
1066+
10611067
/// Abort and close this transaction and erase all diagnostics
10621068
/// record while it was open.
10631069
void abort() {

‎include/swift/AST/TypeCheckRequests.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1843,6 +1843,7 @@ class ValueWitnessRequest
18431843

18441844
struct PreCheckFunctionBuilderDescriptor {
18451845
AnyFunctionRef Fn;
1846+
bool SuppressDiagnostics;
18461847

18471848
private:
18481849
// NOTE: Since source tooling (e.g. code completion) might replace the body,
@@ -1852,8 +1853,8 @@ struct PreCheckFunctionBuilderDescriptor {
18521853
BraceStmt *Body;
18531854

18541855
public:
1855-
PreCheckFunctionBuilderDescriptor(AnyFunctionRef Fn)
1856-
: Fn(Fn), Body(Fn.getBody()) {}
1856+
PreCheckFunctionBuilderDescriptor(AnyFunctionRef Fn, bool suppressDiagnostics)
1857+
: Fn(Fn), SuppressDiagnostics(suppressDiagnostics), Body(Fn.getBody()) {}
18571858

18581859
friend llvm::hash_code
18591860
hash_value(const PreCheckFunctionBuilderDescriptor &owner) {

‎lib/Sema/BuilderTransform.cpp

Lines changed: 46 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,9 +1489,11 @@ Optional<BraceStmt *> TypeChecker::applyFunctionBuilderBodyTransform(
14891489
// If we encountered an error or there was an explicit result type,
14901490
// bail out and report that to the caller.
14911491
auto &ctx = func->getASTContext();
1492-
auto request = PreCheckFunctionBuilderRequest{AnyFunctionRef(func)};
1493-
switch (evaluateOrDefault(
1494-
ctx.evaluator, request, FunctionBuilderBodyPreCheck::Error)) {
1492+
auto request =
1493+
PreCheckFunctionBuilderRequest{{AnyFunctionRef(func),
1494+
/*SuppressDiagnostics=*/false}};
1495+
switch (evaluateOrDefault(ctx.evaluator, request,
1496+
FunctionBuilderBodyPreCheck::Error)) {
14951497
case FunctionBuilderBodyPreCheck::Okay:
14961498
// If the pre-check was okay, apply the function-builder transform.
14971499
break;
@@ -1623,16 +1625,24 @@ ConstraintSystem::matchFunctionBuilder(
16231625

16241626
// Pre-check the body: pre-check any expressions in it and look
16251627
// for return statements.
1626-
auto request = PreCheckFunctionBuilderRequest{fn};
1628+
auto request =
1629+
PreCheckFunctionBuilderRequest{{fn, /*SuppressDiagnostics=*/true}};
16271630
switch (evaluateOrDefault(getASTContext().evaluator, request,
16281631
FunctionBuilderBodyPreCheck::Error)) {
16291632
case FunctionBuilderBodyPreCheck::Okay:
16301633
// If the pre-check was okay, apply the function-builder transform.
16311634
break;
16321635

1633-
case FunctionBuilderBodyPreCheck::Error:
1634-
// If the pre-check had an error, flag that.
1635-
return getTypeMatchFailure(locator);
1636+
case FunctionBuilderBodyPreCheck::Error: {
1637+
if (!shouldAttemptFixes())
1638+
return getTypeMatchFailure(locator);
1639+
1640+
if (recordFix(IgnoreInvalidFunctionBuilderBody::create(
1641+
*this, getConstraintLocator(fn.getBody()))))
1642+
return getTypeMatchFailure(locator);
1643+
1644+
return getTypeMatchSuccess();
1645+
}
16361646

16371647
case FunctionBuilderBodyPreCheck::HasReturnStmt:
16381648
// If the body has a return statement, suppress the transform but
@@ -1709,14 +1719,17 @@ namespace {
17091719
class PreCheckFunctionBuilderApplication : public ASTWalker {
17101720
AnyFunctionRef Fn;
17111721
bool SkipPrecheck = false;
1722+
bool SuppressDiagnostics = false;
17121723
std::vector<ReturnStmt *> ReturnStmts;
17131724
bool HasError = false;
17141725

17151726
bool hasReturnStmt() const { return !ReturnStmts.empty(); }
17161727

17171728
public:
1718-
PreCheckFunctionBuilderApplication(AnyFunctionRef fn, bool skipPrecheck)
1719-
: Fn(fn), SkipPrecheck(skipPrecheck) {}
1729+
PreCheckFunctionBuilderApplication(AnyFunctionRef fn, bool skipPrecheck,
1730+
bool suppressDiagnostics)
1731+
: Fn(fn), SkipPrecheck(skipPrecheck),
1732+
SuppressDiagnostics(suppressDiagnostics) {}
17201733

17211734
const std::vector<ReturnStmt *> getReturnStmts() const { return ReturnStmts; }
17221735

@@ -1740,16 +1753,28 @@ class PreCheckFunctionBuilderApplication : public ASTWalker {
17401753
}
17411754

17421755
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
1756+
if (SkipPrecheck)
1757+
return std::make_pair(false, E);
1758+
17431759
// Pre-check the expression. If this fails, abort the walk immediately.
17441760
// Otherwise, replace the expression with the result of pre-checking.
17451761
// In either case, don't recurse into the expression.
1746-
if (!SkipPrecheck &&
1747-
ConstraintSystem::preCheckExpression(E, /*DC*/ Fn.getAsDeclContext())) {
1748-
HasError = true;
1749-
return std::make_pair(false, nullptr);
1750-
}
1762+
{
1763+
auto *DC = Fn.getAsDeclContext();
1764+
auto &diagEngine = DC->getASTContext().Diags;
1765+
1766+
// Suppress any diangostics which could be produced by this expression.
1767+
DiagnosticTransaction transaction(diagEngine);
17511768

1752-
return std::make_pair(false, E);
1769+
HasError |= ConstraintSystem::preCheckExpression(
1770+
E, DC, /*replaceInvalidRefsWithErrors=*/false);
1771+
HasError |= transaction.hasDiagnostics();
1772+
1773+
if (SuppressDiagnostics)
1774+
transaction.abort();
1775+
1776+
return std::make_pair(false, HasError ? nullptr : E);
1777+
}
17531778
}
17541779

17551780
std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
@@ -1782,11 +1807,15 @@ FunctionBuilderBodyPreCheck PreCheckFunctionBuilderRequest::evaluate(
17821807
owner.Fn.getAbstractClosureExpr()))
17831808
skipPrecheck = shouldTypeCheckInEnclosingExpression(closure);
17841809

1785-
return PreCheckFunctionBuilderApplication(owner.Fn, false).run();
1810+
return PreCheckFunctionBuilderApplication(
1811+
owner.Fn, /*skipPrecheck=*/false,
1812+
/*suppressDiagnostics=*/owner.SuppressDiagnostics)
1813+
.run();
17861814
}
17871815

17881816
std::vector<ReturnStmt *> TypeChecker::findReturnStatements(AnyFunctionRef fn) {
1789-
PreCheckFunctionBuilderApplication precheck(fn, true);
1817+
PreCheckFunctionBuilderApplication precheck(fn, /*skipPreCheck=*/true,
1818+
/*SuppressDiagnostics=*/true);
17901819
(void)precheck.run();
17911820
return precheck.getReturnStmts();
17921821
}

‎lib/Sema/CSBindings.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1206,7 +1206,14 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
12061206
} else if (TypeVar->getImpl().isClosureParameterType()) {
12071207
fix = SpecifyClosureParameterType::create(cs, dstLocator);
12081208
} else if (TypeVar->getImpl().isClosureResultType()) {
1209-
fix = SpecifyClosureReturnType::create(cs, dstLocator);
1209+
auto *locator = TypeVar->getImpl().getLocator();
1210+
auto *closure = castToExpr<ClosureExpr>(locator->getAnchor());
1211+
// If the whole body is being ignored due to a pre-check failure,
1212+
// let's not record a fix about result type since there is
1213+
// just not enough context to infer it without a body.
1214+
if (!cs.hasFixFor(cs.getConstraintLocator(closure->getBody()),
1215+
FixKind::IgnoreInvalidFunctionBuilderBody))
1216+
fix = SpecifyClosureReturnType::create(cs, dstLocator);
12101217
} else if (srcLocator->directlyAt<ObjectLiteralExpr>()) {
12111218
fix = SpecifyObjectLiteralTypeImport::create(cs, dstLocator);
12121219
} else if (srcLocator->isKeyPathRoot()) {

‎lib/Sema/CSFix.cpp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1551,3 +1551,47 @@ AllowKeyPathWithoutComponents::create(ConstraintSystem &cs,
15511551
ConstraintLocator *locator) {
15521552
return new (cs.getAllocator()) AllowKeyPathWithoutComponents(cs, locator);
15531553
}
1554+
1555+
bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution,
1556+
bool asNote) const {
1557+
auto *S = getAnchor().get<Stmt *>();
1558+
1559+
class PreCheckWalker : public ASTWalker {
1560+
DeclContext *DC;
1561+
DiagnosticTransaction Transaction;
1562+
1563+
public:
1564+
PreCheckWalker(DeclContext *dc)
1565+
: DC(dc), Transaction(dc->getASTContext().Diags) {}
1566+
1567+
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
1568+
auto hasError = ConstraintSystem::preCheckExpression(
1569+
E, DC, /*replaceInvalidRefsWithErrors=*/true);
1570+
return std::make_pair(false, hasError ? nullptr : E);
1571+
}
1572+
1573+
std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
1574+
return std::make_pair(true, S);
1575+
}
1576+
1577+
// Ignore patterns because function builder pre-check does so as well.
1578+
std::pair<bool, Pattern *> walkToPatternPre(Pattern *P) override {
1579+
return std::make_pair(false, P);
1580+
}
1581+
1582+
bool diagnosed() const {
1583+
return Transaction.hasDiagnostics();
1584+
}
1585+
};
1586+
1587+
PreCheckWalker walker(solution.getDC());
1588+
S->walk(walker);
1589+
1590+
return walker.diagnosed();
1591+
}
1592+
1593+
IgnoreInvalidFunctionBuilderBody *
1594+
IgnoreInvalidFunctionBuilderBody::create(ConstraintSystem &cs,
1595+
ConstraintLocator *locator) {
1596+
return new (cs.getAllocator()) IgnoreInvalidFunctionBuilderBody(cs, locator);
1597+
}

‎lib/Sema/CSFix.h

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,9 @@ enum class FixKind : uint8_t {
276276

277277
/// Allow key path expressions with no components.
278278
AllowKeyPathWithoutComponents,
279+
280+
/// Ignore function builder body which fails `pre-check` call.
281+
IgnoreInvalidFunctionBuilderBody,
279282
};
280283

281284
class ConstraintFix {
@@ -1980,6 +1983,26 @@ class AllowKeyPathWithoutComponents final : public ConstraintFix {
19801983
ConstraintLocator *locator);
19811984
};
19821985

1986+
class IgnoreInvalidFunctionBuilderBody final : public ConstraintFix {
1987+
IgnoreInvalidFunctionBuilderBody(ConstraintSystem &cs,
1988+
ConstraintLocator *locator)
1989+
: ConstraintFix(cs, FixKind::IgnoreInvalidFunctionBuilderBody, locator) {}
1990+
1991+
public:
1992+
std::string getName() const override {
1993+
return "ignore invalid function builder body";
1994+
}
1995+
1996+
bool diagnose(const Solution &solution, bool asNote = false) const override;
1997+
1998+
bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
1999+
return diagnose(*commonFixes.front().first);
2000+
}
2001+
2002+
static IgnoreInvalidFunctionBuilderBody *create(ConstraintSystem &cs,
2003+
ConstraintLocator *locator);
2004+
};
2005+
19832006
} // end namespace constraints
19842007
} // end namespace swift
19852008

‎lib/Sema/CSSimplify.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9997,7 +9997,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
99979997
case FixKind::AllowCoercionToForceCast:
99989998
case FixKind::SpecifyKeyPathRootType:
99999999
case FixKind::SpecifyLabelToAssociateTrailingClosure:
10000-
case FixKind::AllowKeyPathWithoutComponents: {
10000+
case FixKind::AllowKeyPathWithoutComponents:
10001+
case FixKind::IgnoreInvalidFunctionBuilderBody: {
1000110002
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
1000210003
}
1000310004

‎lib/Sema/CSSolver.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1411,7 +1411,8 @@ void ConstraintSystem::solveForCodeCompletion(
14111411
llvm::function_ref<void(const Solution &)> callback) {
14121412
// First, pre-check the expression, validating any types that occur in the
14131413
// expression and folding sequence expressions.
1414-
if (ConstraintSystem::preCheckExpression(expr, DC))
1414+
if (ConstraintSystem::preCheckExpression(
1415+
expr, DC, /*replaceInvalidRefsWithErrors=*/true))
14151416
return;
14161417

14171418
ConstraintSystemOptions options;

‎lib/Sema/ConstraintSystem.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4955,8 +4955,12 @@ class ConstraintSystem {
49554955
public:
49564956
/// Pre-check the expression, validating any types that occur in the
49574957
/// expression and folding sequence expressions.
4958-
static bool preCheckExpression(Expr *&expr, DeclContext *dc);
4959-
4958+
///
4959+
/// \param replaceInvalidRefsWithErrors Indicates whether it's allowed
4960+
/// to replace any discovered invalid member references with `ErrorExpr`.
4961+
static bool preCheckExpression(Expr *&expr, DeclContext *dc,
4962+
bool replaceInvalidRefsWithErrors);
4963+
49604964
/// Solve the system of constraints generated from provided target.
49614965
///
49624966
/// \param target The target that we'll generate constraints from, which

‎lib/Sema/TypeCheckCodeCompletion.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -553,10 +553,10 @@ TypeChecker::getTypeOfCompletionOperator(DeclContext *DC, Expr *LHS,
553553
// We allocate these expressions on the stack because we know they can't
554554
// escape and there isn't a better way to allocate scratch Expr nodes.
555555
UnresolvedDeclRefExpr UDRE(DeclNameRef(opName), refKind, DeclNameLoc(Loc));
556-
auto *opExpr = TypeChecker::resolveDeclRefExpr(&UDRE, DC);
556+
auto *opExpr = TypeChecker::resolveDeclRefExpr(
557+
&UDRE, DC, /*replaceInvalidRefsWithErrors=*/true);
557558

558559
switch (refKind) {
559-
560560
case DeclRefKind::PostfixOperator: {
561561
// (postfix_unary_expr
562562
// (declref_expr name=<opName>)
@@ -611,7 +611,8 @@ static Optional<Type> getTypeOfCompletionContextExpr(
611611
CompletionTypeCheckKind kind,
612612
Expr *&parsedExpr,
613613
ConcreteDeclRef &referencedDecl) {
614-
if (constraints::ConstraintSystem::preCheckExpression(parsedExpr, DC))
614+
if (constraints::ConstraintSystem::preCheckExpression(
615+
parsedExpr, DC, /*replaceInvalidRefsWithErrors=*/true))
615616
return None;
616617

617618
switch (kind) {

0 commit comments

Comments
 (0)