Skip to content

Commit 2b8d3ea

Browse files
tkremenektkremenek
authored
Merge pull request #33614 from xedin/rdar-65320500-5.3
[5.3][FunctionBuilders] Implement graceful handling of pre-check failures
2 parents 9f28f80 + 4de7ea1 commit 2b8d3ea

14 files changed

+232
-45
lines changed

‎include/swift/AST/DiagnosticEngine.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,6 +1025,12 @@ namespace swift {
10251025
}
10261026
}
10271027

1028+
bool hasDiagnostics() const {
1029+
return std::distance(Engine.TentativeDiagnostics.begin() +
1030+
PrevDiagnostics,
1031+
Engine.TentativeDiagnostics.end()) > 0;
1032+
}
1033+
10281034
/// Abort and close this transaction and erase all diagnostics
10291035
/// record while it was open.
10301036
void abort() {

‎include/swift/AST/TypeCheckRequests.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1822,7 +1822,8 @@ enum class FunctionBuilderBodyPreCheck : uint8_t {
18221822
class PreCheckFunctionBuilderRequest
18231823
: public SimpleRequest<PreCheckFunctionBuilderRequest,
18241824
FunctionBuilderBodyPreCheck(AnyFunctionRef,
1825-
BraceStmt *),
1825+
BraceStmt *,
1826+
bool),
18261827
RequestFlags::Cached> {
18271828
public:
18281829
using SimpleRequest::SimpleRequest;
@@ -1832,7 +1833,8 @@ class PreCheckFunctionBuilderRequest
18321833

18331834
// Evaluation.
18341835
FunctionBuilderBodyPreCheck evaluate(Evaluator &evaluator, AnyFunctionRef fn,
1835-
BraceStmt *body) const;
1836+
BraceStmt *body,
1837+
bool suppressDiagnostics) const;
18361838

18371839
public:
18381840
// Separate caching.

‎lib/Sema/BuilderTransform.cpp

Lines changed: 46 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1374,9 +1374,10 @@ Optional<BraceStmt *> TypeChecker::applyFunctionBuilderBodyTransform(
13741374
// If we encountered an error or there was an explicit result type,
13751375
// bail out and report that to the caller.
13761376
auto &ctx = func->getASTContext();
1377-
auto request = PreCheckFunctionBuilderRequest{func, func->getBody()};
1378-
switch (evaluateOrDefault(
1379-
ctx.evaluator, request, FunctionBuilderBodyPreCheck::Error)) {
1377+
auto request = PreCheckFunctionBuilderRequest{func, func->getBody(),
1378+
/*suppressDiagnostics=*/false};
1379+
switch (evaluateOrDefault(ctx.evaluator, request,
1380+
FunctionBuilderBodyPreCheck::Error)) {
13801381
case FunctionBuilderBodyPreCheck::Okay:
13811382
// If the pre-check was okay, apply the function-builder transform.
13821383
break;
@@ -1524,16 +1525,27 @@ ConstraintSystem::matchFunctionBuilder(
15241525

15251526
// Pre-check the body: pre-check any expressions in it and look
15261527
// for return statements.
1527-
auto request = PreCheckFunctionBuilderRequest{fn, fn.getBody()};
1528+
auto request = PreCheckFunctionBuilderRequest{fn, fn.getBody(),
1529+
/*SuppressDiagnostics=*/true};
15281530
switch (evaluateOrDefault(getASTContext().evaluator, request,
15291531
FunctionBuilderBodyPreCheck::Error)) {
15301532
case FunctionBuilderBodyPreCheck::Okay:
15311533
// If the pre-check was okay, apply the function-builder transform.
15321534
break;
15331535

1534-
case FunctionBuilderBodyPreCheck::Error:
1535-
// If the pre-check had an error, flag that.
1536+
case FunctionBuilderBodyPreCheck::Error: {
1537+
if (!shouldAttemptFixes())
1538+
return getTypeMatchFailure(locator);
1539+
1540+
if (auto *closure =
1541+
dyn_cast_or_null<ClosureExpr>(fn.getAbstractClosureExpr())) {
1542+
auto failed = recordFix(IgnoreInvalidFunctionBuilderBody::create(
1543+
*this, getConstraintLocator(closure)));
1544+
return failed ? getTypeMatchFailure(locator) : getTypeMatchSuccess();
1545+
}
1546+
15361547
return getTypeMatchFailure(locator);
1548+
}
15371549

15381550
case FunctionBuilderBodyPreCheck::HasReturnStmt:
15391551
// If the body has a return statement, suppress the transform but
@@ -1626,14 +1638,17 @@ namespace {
16261638
class PreCheckFunctionBuilderApplication : public ASTWalker {
16271639
AnyFunctionRef Fn;
16281640
bool SkipPrecheck = false;
1641+
bool SuppressDiagnostics = false;
16291642
std::vector<ReturnStmt *> ReturnStmts;
16301643
bool HasError = false;
16311644

16321645
bool hasReturnStmt() const { return !ReturnStmts.empty(); }
16331646

16341647
public:
1635-
PreCheckFunctionBuilderApplication(AnyFunctionRef fn, bool skipPrecheck)
1636-
: Fn(fn), SkipPrecheck(skipPrecheck) {}
1648+
PreCheckFunctionBuilderApplication(AnyFunctionRef fn, bool skipPrecheck,
1649+
bool suppressDiagnostics)
1650+
: Fn(fn), SkipPrecheck(skipPrecheck),
1651+
SuppressDiagnostics(suppressDiagnostics) {}
16371652

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

@@ -1657,16 +1672,28 @@ class PreCheckFunctionBuilderApplication : public ASTWalker {
16571672
}
16581673

16591674
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
1675+
if (SkipPrecheck)
1676+
return std::make_pair(false, E);
1677+
16601678
// Pre-check the expression. If this fails, abort the walk immediately.
16611679
// Otherwise, replace the expression with the result of pre-checking.
16621680
// In either case, don't recurse into the expression.
1663-
if (!SkipPrecheck &&
1664-
ConstraintSystem::preCheckExpression(E, /*DC*/ Fn.getAsDeclContext())) {
1665-
HasError = true;
1666-
return std::make_pair(false, nullptr);
1667-
}
1681+
{
1682+
auto *DC = Fn.getAsDeclContext();
1683+
auto &diagEngine = DC->getASTContext().Diags;
1684+
1685+
// Suppress any diangostics which could be produced by this expression.
1686+
DiagnosticTransaction transaction(diagEngine);
16681687

1669-
return std::make_pair(false, E);
1688+
HasError |= ConstraintSystem::preCheckExpression(
1689+
E, DC, /*replaceInvalidRefsWithErrors=*/false);
1690+
HasError |= transaction.hasDiagnostics();
1691+
1692+
if (SuppressDiagnostics)
1693+
transaction.abort();
1694+
1695+
return std::make_pair(false, HasError ? nullptr : E);
1696+
}
16701697
}
16711698

16721699
std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
@@ -1692,18 +1719,20 @@ class PreCheckFunctionBuilderApplication : public ASTWalker {
16921719

16931720
FunctionBuilderBodyPreCheck
16941721
PreCheckFunctionBuilderRequest::evaluate(Evaluator &eval, AnyFunctionRef fn,
1695-
BraceStmt *body) const {
1722+
BraceStmt *body,
1723+
bool suppressDiagnostics) const {
16961724
// NOTE: 'body' is passed only for the request evaluater caching key.
16971725
// Since source tooling (e.g. code completion) might replace the body,
16981726
// the function alone is not sufficient for the key.
16991727
assert(fn.getBody() == body &&
17001728
"body must be the current body of the function");
17011729

1702-
return PreCheckFunctionBuilderApplication(fn, false).run();
1730+
return PreCheckFunctionBuilderApplication(fn, false, suppressDiagnostics).run();
17031731
}
17041732

17051733
std::vector<ReturnStmt *> TypeChecker::findReturnStatements(AnyFunctionRef fn) {
1706-
PreCheckFunctionBuilderApplication precheck(fn, true);
1734+
PreCheckFunctionBuilderApplication precheck(fn, /*skipPreCheck=*/true,
1735+
/*SuppressDiagnostics=*/true);
17071736
(void)precheck.run();
17081737
return precheck.getReturnStmts();
17091738
}

‎lib/Sema/CSBindings.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1106,7 +1106,14 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
11061106
} else if (TypeVar->getImpl().isClosureParameterType()) {
11071107
fix = SpecifyClosureParameterType::create(cs, dstLocator);
11081108
} else if (TypeVar->getImpl().isClosureResultType()) {
1109-
fix = SpecifyClosureReturnType::create(cs, dstLocator);
1109+
auto *locator = TypeVar->getImpl().getLocator();
1110+
auto *closure = cast<ClosureExpr>(locator->getAnchor());
1111+
// If the whole body is being ignored due to a pre-check failure,
1112+
// let's not record a fix about result type since there is
1113+
// just not enough context to infer it without a body.
1114+
if (!cs.hasFixFor(cs.getConstraintLocator(closure),
1115+
FixKind::IgnoreInvalidFunctionBuilderBody))
1116+
fix = SpecifyClosureReturnType::create(cs, dstLocator);
11101117
} else if (srcLocator->getAnchor() &&
11111118
isa<ObjectLiteralExpr>(srcLocator->getAnchor())) {
11121119
fix = SpecifyObjectLiteralTypeImport::create(cs, dstLocator);

‎lib/Sema/CSFix.cpp

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,3 +1363,48 @@ bool SpecifyKeyPathRootType::diagnose(const Solution &solution,
13631363

13641364
return failure.diagnose(asNote);
13651365
}
1366+
1367+
bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution,
1368+
bool asNote) const {
1369+
auto *S = cast<ClosureExpr>(getAnchor())->getBody();
1370+
1371+
class PreCheckWalker : public ASTWalker {
1372+
DeclContext *DC;
1373+
DiagnosticTransaction Transaction;
1374+
1375+
public:
1376+
PreCheckWalker(DeclContext *dc)
1377+
: DC(dc), Transaction(dc->getASTContext().Diags) {}
1378+
1379+
std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
1380+
auto hasError = ConstraintSystem::preCheckExpression(
1381+
E, DC, /*replaceInvalidRefsWithErrors=*/true);
1382+
return std::make_pair(false, hasError ? nullptr : E);
1383+
}
1384+
1385+
std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
1386+
return std::make_pair(true, S);
1387+
}
1388+
1389+
// Ignore patterns because function builder pre-check does so as well.
1390+
std::pair<bool, Pattern *> walkToPatternPre(Pattern *P) override {
1391+
return std::make_pair(false, P);
1392+
}
1393+
1394+
bool diagnosed() const {
1395+
return Transaction.hasDiagnostics();
1396+
}
1397+
};
1398+
1399+
auto &cs = getConstraintSystem();
1400+
PreCheckWalker walker(cs.DC);
1401+
S->walk(walker);
1402+
1403+
return walker.diagnosed();
1404+
}
1405+
1406+
IgnoreInvalidFunctionBuilderBody *
1407+
IgnoreInvalidFunctionBuilderBody::create(ConstraintSystem &cs,
1408+
ConstraintLocator *locator) {
1409+
return new (cs.getAllocator()) IgnoreInvalidFunctionBuilderBody(cs, locator);
1410+
}

‎lib/Sema/CSFix.h

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,8 @@ enum class FixKind : uint8_t {
264264
/// Specify key path root type when it cannot be infered from context.
265265
SpecifyKeyPathRootType,
266266

267+
/// Ignore function builder body which fails `pre-check` call.
268+
IgnoreInvalidFunctionBuilderBody,
267269
};
268270

269271
class ConstraintFix {
@@ -1841,6 +1843,26 @@ class SpecifyKeyPathRootType final : public ConstraintFix {
18411843
ConstraintLocator *locator);
18421844
};
18431845

1846+
class IgnoreInvalidFunctionBuilderBody final : public ConstraintFix {
1847+
IgnoreInvalidFunctionBuilderBody(ConstraintSystem &cs,
1848+
ConstraintLocator *locator)
1849+
: ConstraintFix(cs, FixKind::IgnoreInvalidFunctionBuilderBody, locator) {}
1850+
1851+
public:
1852+
std::string getName() const override {
1853+
return "ignore invalid function builder body";
1854+
}
1855+
1856+
bool diagnose(const Solution &solution, bool asNote = false) const override;
1857+
1858+
bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
1859+
return diagnose(*commonFixes.front().first);
1860+
}
1861+
1862+
static IgnoreInvalidFunctionBuilderBody *create(ConstraintSystem &cs,
1863+
ConstraintLocator *locator);
1864+
};
1865+
18441866
} // end namespace constraints
18451867
} // end namespace swift
18461868

‎lib/Sema/CSSimplify.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9712,7 +9712,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
97129712
case FixKind::SpecifyObjectLiteralTypeImport:
97139713
case FixKind::AllowKeyPathRootTypeMismatch:
97149714
case FixKind::AllowCoercionToForceCast:
9715-
case FixKind::SpecifyKeyPathRootType: {
9715+
case FixKind::SpecifyKeyPathRootType:
9716+
case FixKind::IgnoreInvalidFunctionBuilderBody: {
97169717
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
97179718
}
97189719

‎lib/Sema/ConstraintSystem.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4606,8 +4606,12 @@ class ConstraintSystem {
46064606
public:
46074607
/// Pre-check the expression, validating any types that occur in the
46084608
/// expression and folding sequence expressions.
4609-
static bool preCheckExpression(Expr *&expr, DeclContext *dc);
4610-
4609+
///
4610+
/// \param replaceInvalidRefsWithErrors Indicates whether it's allowed
4611+
/// to replace any discovered invalid member references with `ErrorExpr`.
4612+
static bool preCheckExpression(Expr *&expr, DeclContext *dc,
4613+
bool replaceInvalidRefsWithErrors);
4614+
46114615
/// Solve the system of constraints generated from provided target.
46124616
///
46134617
/// \param target The target that we'll generate constraints from, which

0 commit comments

Comments
 (0)