From 3e1715ec4c80af91395c4af3b87b19a76009ad04 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Tue, 1 Sep 2020 12:23:47 -0700 Subject: [PATCH] [ConstraintSystem] Extend invalid function body fix to cover constraint generation failures Ignore function builder body if it emits at least one diagnostic during constraint generation. Resolves: rdar://problem/65983237 --- include/swift/AST/DiagnosticEngine.h | 17 ++++++--- lib/Sema/BuilderTransform.cpp | 36 ++++++++++++++++--- lib/Sema/CSFix.cpp | 18 +++++++--- lib/Sema/CSFix.h | 27 +++++++++++--- lib/Sema/ConstraintSystem.h | 7 ++++ test/Constraints/function_builder_diags.swift | 8 +++++ 6 files changed, 95 insertions(+), 18 deletions(-) diff --git a/include/swift/AST/DiagnosticEngine.h b/include/swift/AST/DiagnosticEngine.h index 6513a5df51ae3..50bbe2037a1e6 100644 --- a/include/swift/AST/DiagnosticEngine.h +++ b/include/swift/AST/DiagnosticEngine.h @@ -1025,10 +1025,19 @@ namespace swift { } } - bool hasDiagnostics() const { - return std::distance(Engine.TentativeDiagnostics.begin() + - PrevDiagnostics, - Engine.TentativeDiagnostics.end()) > 0; + bool hasErrors() const { + ArrayRef diagnostics(Engine.TentativeDiagnostics.begin() + + PrevDiagnostics, + Engine.TentativeDiagnostics.end()); + + for (auto &diagnostic : diagnostics) { + auto behavior = Engine.state.determineBehavior(diagnostic.getID()); + if (behavior == DiagnosticState::Behavior::Fatal || + behavior == DiagnosticState::Behavior::Error) + return true; + } + + return false; } /// Abort and close this transaction and erase all diagnostics diff --git a/lib/Sema/BuilderTransform.cpp b/lib/Sema/BuilderTransform.cpp index df7f4f87d76ce..ae1892d0e1312 100644 --- a/lib/Sema/BuilderTransform.cpp +++ b/lib/Sema/BuilderTransform.cpp @@ -1523,6 +1523,14 @@ ConstraintSystem::matchFunctionBuilder( assert(builder && "Bad function builder type"); assert(builder->getAttrs().hasAttribute()); + if (InvalidFunctionBuilderBodies.count(fn)) { + auto *closure = cast(fn.getAbstractClosureExpr()); + (void)recordFix( + IgnoreInvalidFunctionBuilderBody::duringConstraintGeneration( + *this, getConstraintLocator(closure))); + return getTypeMatchSuccess(); + } + // Pre-check the body: pre-check any expressions in it and look // for return statements. auto request = PreCheckFunctionBuilderRequest{fn, fn.getBody(), @@ -1539,7 +1547,7 @@ ConstraintSystem::matchFunctionBuilder( if (auto *closure = dyn_cast_or_null(fn.getAbstractClosureExpr())) { - auto failed = recordFix(IgnoreInvalidFunctionBuilderBody::create( + auto failed = recordFix(IgnoreInvalidFunctionBuilderBody::duringPreCheck( *this, getConstraintLocator(closure))); return failed ? getTypeMatchFailure(locator) : getTypeMatchSuccess(); } @@ -1598,9 +1606,27 @@ ConstraintSystem::matchFunctionBuilder( BuilderClosureVisitor visitor(getASTContext(), this, dc, builderType, bodyResultType); - auto applied = visitor.apply(fn.getBody()); - if (!applied) - return getTypeMatchFailure(locator); + Optional applied = None; + { + DiagnosticTransaction transaction(dc->getASTContext().Diags); + + applied = visitor.apply(fn.getBody()); + if (!applied) + return getTypeMatchFailure(locator); + + if (transaction.hasErrors()) { + if (auto *closure = + dyn_cast_or_null(fn.getAbstractClosureExpr())) { + InvalidFunctionBuilderBodies.insert(fn); + auto failed = recordFix( + IgnoreInvalidFunctionBuilderBody::duringConstraintGeneration( + *this, getConstraintLocator(closure))); + return failed ? getTypeMatchFailure(locator) : getTypeMatchSuccess(); + } + + return getTypeMatchFailure(locator); + } + } Type transformedType = getType(applied->returnExpr); assert(transformedType && "Missing type"); @@ -1687,7 +1713,7 @@ class PreCheckFunctionBuilderApplication : public ASTWalker { HasError |= ConstraintSystem::preCheckExpression( E, DC, /*replaceInvalidRefsWithErrors=*/false); - HasError |= transaction.hasDiagnostics(); + HasError |= transaction.hasErrors(); if (SuppressDiagnostics) transaction.abort(); diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp index e97b37626f967..e2091e73e7c29 100644 --- a/lib/Sema/CSFix.cpp +++ b/lib/Sema/CSFix.cpp @@ -1366,6 +1366,14 @@ bool SpecifyKeyPathRootType::diagnose(const Solution &solution, bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution, bool asNote) const { + switch (Phase) { + // Handled below + case ErrorInPhase::PreCheck: + break; + case ErrorInPhase::ConstraintGeneration: + return true; // Already diagnosed by `matchFunctionBuilder`. + } + auto *S = cast(getAnchor())->getBody(); class PreCheckWalker : public ASTWalker { @@ -1392,7 +1400,7 @@ bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution, } bool diagnosed() const { - return Transaction.hasDiagnostics(); + return Transaction.hasErrors(); } }; @@ -1403,8 +1411,8 @@ bool IgnoreInvalidFunctionBuilderBody::diagnose(const Solution &solution, return walker.diagnosed(); } -IgnoreInvalidFunctionBuilderBody * -IgnoreInvalidFunctionBuilderBody::create(ConstraintSystem &cs, - ConstraintLocator *locator) { - return new (cs.getAllocator()) IgnoreInvalidFunctionBuilderBody(cs, locator); +IgnoreInvalidFunctionBuilderBody *IgnoreInvalidFunctionBuilderBody::create( + ConstraintSystem &cs, ErrorInPhase phase, ConstraintLocator *locator) { + return new (cs.getAllocator()) + IgnoreInvalidFunctionBuilderBody(cs, phase, locator); } diff --git a/lib/Sema/CSFix.h b/lib/Sema/CSFix.h index 08a582de6a311..727b05a0e62f2 100644 --- a/lib/Sema/CSFix.h +++ b/lib/Sema/CSFix.h @@ -1844,9 +1844,17 @@ class SpecifyKeyPathRootType final : public ConstraintFix { }; class IgnoreInvalidFunctionBuilderBody final : public ConstraintFix { - IgnoreInvalidFunctionBuilderBody(ConstraintSystem &cs, + enum class ErrorInPhase { + PreCheck, + ConstraintGeneration, + }; + + ErrorInPhase Phase; + + IgnoreInvalidFunctionBuilderBody(ConstraintSystem &cs, ErrorInPhase phase, ConstraintLocator *locator) - : ConstraintFix(cs, FixKind::IgnoreInvalidFunctionBuilderBody, locator) {} + : ConstraintFix(cs, FixKind::IgnoreInvalidFunctionBuilderBody, locator), + Phase(phase) {} public: std::string getName() const override { @@ -1859,8 +1867,19 @@ class IgnoreInvalidFunctionBuilderBody final : public ConstraintFix { return diagnose(*commonFixes.front().first); } - static IgnoreInvalidFunctionBuilderBody *create(ConstraintSystem &cs, - ConstraintLocator *locator); + static IgnoreInvalidFunctionBuilderBody * + duringPreCheck(ConstraintSystem &cs, ConstraintLocator *locator) { + return create(cs, ErrorInPhase::PreCheck, locator); + } + + static IgnoreInvalidFunctionBuilderBody * + duringConstraintGeneration(ConstraintSystem &cs, ConstraintLocator *locator) { + return create(cs, ErrorInPhase::ConstraintGeneration, locator); + } + +private: + static IgnoreInvalidFunctionBuilderBody * + create(ConstraintSystem &cs, ErrorInPhase phase, ConstraintLocator *locator); }; } // end namespace constraints diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h index 0428d6570283d..105234a04fca2 100644 --- a/lib/Sema/ConstraintSystem.h +++ b/lib/Sema/ConstraintSystem.h @@ -1727,6 +1727,13 @@ class ConstraintSystem { /// from declared parameters/result and body. llvm::MapVector ClosureTypes; + /// This is a *global* list of all function builder bodies that have + /// been determined to be incorrect by failing constraint generation. + /// + /// Tracking this information is useful to avoid producing duplicate + /// diagnostics when function builder has multiple overloads. + llvm::SmallDenseSet InvalidFunctionBuilderBodies; + /// Maps node types used within all portions of the constraint /// system, instead of directly using the types on the /// nodes themselves. This allows us to typecheck and diff --git a/test/Constraints/function_builder_diags.swift b/test/Constraints/function_builder_diags.swift index fae9cf22a3971..211108bf70ef6 100644 --- a/test/Constraints/function_builder_diags.swift +++ b/test/Constraints/function_builder_diags.swift @@ -606,6 +606,14 @@ struct MyView { } // expected-error {{expected identifier after '.' expression}} } + @TupleBuilder var invalidCaseWithoutDot: some P { + switch Optional.some(1) { + case none: 42 // expected-error {{cannot find 'none' in scope}} + case .some(let x): + 0 + } + } + @TupleBuilder var invalidConversion: Int { "" // expected-error {{cannot convert value of type 'String' to specified type 'Int'}} }