Merge pull request #22219 from xedin/rdar-38885760-5.0

AnnaZaks · web-flow · commit c19dfb500878 · 2019-01-30T16:44:09.000-08:00
[5.0][TypeChecker] Always emit a fallback error if type-check failed witho
diff --git a/include/swift/AST/ASTContext.h b/include/swift/AST/ASTContext.h
@@ -765,6 +765,10 @@ class ASTContext final {
     bool IsError;
   };
 
+  /// Check whether current context has any errors associated with
+  /// ill-formed protocol conformances which haven't been produced yet.
+  bool hasDelayedConformanceErrors() const;
+
   /// Add a delayed diagnostic produced while type-checking a
   /// particular protocol conformance.
   void addDelayedConformanceDiag(NormalProtocolConformance *conformance,
diff --git a/include/swift/AST/Builtins.def b/include/swift/AST/Builtins.def
@@ -580,6 +580,7 @@ BUILTIN_SANITIZER_OPERATION(TSanInoutAccess, "tsanInoutAccess", "")
 BUILTIN_TYPE_CHECKER_OPERATION(TypeJoin, type_join)
 BUILTIN_TYPE_CHECKER_OPERATION(TypeJoinInout, type_join_inout)
 BUILTIN_TYPE_CHECKER_OPERATION(TypeJoinMeta, type_join_meta)
+BUILTIN_TYPE_CHECKER_OPERATION(TriggerFallbackDiagnostic, trigger_fallback_diagnostic)
 
 #undef BUILTIN_TYPE_CHECKER_OPERATION
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2822,6 +2822,10 @@ ERROR(type_of_expression_is_ambiguous,none,
 ERROR(specific_type_of_expression_is_ambiguous,none,
       "expression type %0 is ambiguous without more context", (Type))
 
+ERROR(failed_to_produce_diagnostic,Fatal,
+      "failed to produce diagnostic for expression; "
+      "please file a bug report with your project", ())
+
 
 ERROR(missing_protocol,none,
       "missing protocol %0", (Identifier))
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -2047,6 +2047,19 @@ LazyGenericContextData *ASTContext::getOrCreateLazyGenericContextData(
                                                               lazyLoader);
 }
 
+bool ASTContext::hasDelayedConformanceErrors() const {
+  for (const auto &entry : getImpl().DelayedConformanceDiags) {
+    auto &diagnostics = entry.getSecond();
+    if (std::any_of(diagnostics.begin(), diagnostics.end(),
+                    [](const ASTContext::DelayedConformanceDiag &diag) {
+                      return diag.IsError;
+                    }))
+      return true;
+  }
+
+  return false;
+}
+
 void ASTContext::addDelayedConformanceDiag(
        NormalProtocolConformance *conformance,
        DelayedConformanceDiag fn) {
diff --git a/lib/AST/Builtins.cpp b/lib/AST/Builtins.cpp
@@ -1010,6 +1010,12 @@ static ValueDecl *getTypeJoinMetaOperation(ASTContext &Context, Identifier Id) {
   return builder.build(Id);
 }
 
+static ValueDecl *getTriggerFallbackDiagnosticOperation(ASTContext &Context,
+                                                        Identifier Id) {
+  // () -> Void
+  return getBuiltinFunction(Id, {}, Context.TheEmptyTupleType);
+}
+
 static ValueDecl *getCanBeObjCClassOperation(ASTContext &Context,
                                              Identifier Id) {
   // <T> T.Type -> Builtin.Int8
@@ -1899,7 +1905,10 @@ ValueDecl *swift::getBuiltinValueDecl(ASTContext &Context, Identifier Id) {
     return getTypeJoinInoutOperation(Context, Id);
 
   case BuiltinValueKind::TypeJoinMeta:
-    return getTypeJoinMetaOperation(Context, Id);      
+    return getTypeJoinMetaOperation(Context, Id);
+
+  case BuiltinValueKind::TriggerFallbackDiagnostic:
+    return getTriggerFallbackDiagnosticOperation(Context, Id);
   }
 
   llvm_unreachable("bad builtin value!");
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -3912,8 +3912,9 @@ namespace {
     }
 
     Expr *visitLazyInitializerExpr(LazyInitializerExpr *expr) {
-      simplifyExprType(expr);
-      assert(expr->getType()->isEqual(expr->getSubExpr()->getType()));
+      // Since `LazyInitializerExpr` should always have a type set,
+      // there is no need to do anything here.
+      assert(cs.getType(expr)->isEqual(expr->getSubExpr()->getType()));
       return expr;
     }
     
diff --git a/lib/Sema/CSDiag.cpp b/lib/Sema/CSDiag.cpp
@@ -1893,6 +1893,10 @@ Expr *FailureDiagnosis::typeCheckChildIndependently(
   // the context is missing).
   TypeCheckExprOptions TCEOptions = TypeCheckExprFlags::DisableStructuralChecks;
 
+  // Make sure that typechecker knows that this is an attempt
+  // to diagnose a problem.
+  TCEOptions |= TypeCheckExprFlags::SubExpressionDiagnostics;
+
   // Don't walk into non-single expression closure bodies, because
   // ExprTypeSaver and TypeNullifier skip them too.
   TCEOptions |= TypeCheckExprFlags::SkipMultiStmtClosures;
diff --git a/lib/Sema/CSGen.cpp b/lib/Sema/CSGen.cpp
@@ -1579,6 +1579,11 @@ namespace {
       if (typeOperation != TypeOperation::None)
         return CS.getASTContext().TheAnyType;
 
+      // If this is `Builtin.trigger_fallback_diagnostic()`, fail
+      // without producing any diagnostics, in order to test fallback error.
+      if (isTriggerFallbackDiagnosticBuiltin(expr, CS.getASTContext()))
+        return Type();
+
       // Open a member constraint for constructor delegations on the
       // subexpr type.
       if (CS.TC.getSelfForInitDelegationInConstructor(CS.DC, expr)) {
@@ -2919,7 +2924,9 @@ namespace {
     }
 
     Type visitLazyInitializerExpr(LazyInitializerExpr *expr) {
-      return expr->getType();
+      auto type = expr->getType();
+      assert(type && "LazyInitializerExpr should always have type set");
+      return type;
     }
 
     Type visitEditorPlaceholderExpr(EditorPlaceholderExpr *E) {
@@ -3128,6 +3135,19 @@ namespace {
       return tv;
     }
 
+    static bool isTriggerFallbackDiagnosticBuiltin(UnresolvedDotExpr *UDE,
+                                                   ASTContext &Context) {
+      auto *DRE = dyn_cast<DeclRefExpr>(UDE->getBase());
+      if (!DRE)
+        return false;
+
+      if (DRE->getDecl() != Context.TheBuiltinModule)
+        return false;
+
+      auto member = UDE->getName().getBaseName().userFacingName();
+      return member.equals("trigger_fallback_diagnostic");
+    }
+
     enum class TypeOperation { None,
                                Join,
                                JoinInout,
diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp
@@ -1175,6 +1175,8 @@ ConstraintSystem::solveImpl(Expr *&expr,
   if (auto generatedExpr = generateConstraints(expr))
     expr = generatedExpr;
   else {
+    if (listener)
+      listener->constraintGenerationFailed(expr);
     return SolutionKind::Error;
   }
 
diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp
@@ -1323,7 +1323,9 @@ static void synthesizeLazyGetterBody(TypeChecker &TC, AccessorDecl *Get,
   // FIXME: we should really have stronger invariants than this.  Leaving it
   // unwrapped may expose both expressions to naive walkers
   if (wasInitializerChecked) {
+    auto initType = InitValue->getType();
     InitValue = new (Ctx) LazyInitializerExpr(InitValue);
+    InitValue->setType(initType);
   }
 
   Pattern *Tmp2PBDPattern = new (Ctx) NamedPattern(Tmp2VD, /*implicit*/true);
diff --git a/lib/Sema/TypeCheckConstraints.cpp b/lib/Sema/TypeCheckConstraints.cpp
@@ -29,6 +29,7 @@
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/PrettyStackTrace.h"
+#include "swift/AST/ProtocolConformance.h"
 #include "swift/AST/SubstitutionMap.h"
 #include "swift/AST/TypeCheckerDebugConsumer.h"
 #include "swift/Basic/Statistic.h"
@@ -1937,6 +1938,11 @@ Expr *ExprTypeCheckListener::appliedSolution(Solution &solution, Expr *expr) {
   return expr;
 }
 
+void ExprTypeCheckListener::preCheckFailed(Expr *expr) {}
+void ExprTypeCheckListener::constraintGenerationFailed(Expr *expr) {}
+void ExprTypeCheckListener::applySolutionFailed(Solution &solution,
+                                                Expr *expr) {}
+
 void ParentConditionalConformance::diagnoseConformanceStack(
     DiagnosticEngine &diags, SourceLoc loc,
     ArrayRef<ParentConditionalConformance> conformances) {
@@ -1964,20 +1970,115 @@ bool GenericRequirementsCheckListener::diagnoseUnsatisfiedRequirement(
   return false;
 }
 
+/// Sometimes constraint solver fails without producing any diagnostics,
+/// that leads to crashes down the line in AST Verifier or SILGen
+/// which, as a result, are much harder to figure out.
+///
+/// This class is intended to guard against situations like that by
+/// keeping track of failures of different type-check phases, and
+/// emitting fallback fatal error if any of them fail without producing
+/// error diagnostic, and there were no errors emitted or scheduled to be
+/// emitted previously.
+class FallbackDiagnosticListener : public ExprTypeCheckListener {
+  TypeChecker &TC;
+  TypeCheckExprOptions Options;
+  ExprTypeCheckListener *BaseListener;
+
+public:
+  FallbackDiagnosticListener(TypeChecker &TC, TypeCheckExprOptions options,
+                             ExprTypeCheckListener *base)
+      : TC(TC), Options(options), BaseListener(base) {}
+
+  bool builtConstraints(ConstraintSystem &cs, Expr *expr) override {
+    return BaseListener ? BaseListener->builtConstraints(cs, expr) : false;
+  }
+
+  Expr *foundSolution(Solution &solution, Expr *expr) override {
+    return BaseListener ? BaseListener->foundSolution(solution, expr) : expr;
+  }
+
+  Expr *appliedSolution(Solution &solution, Expr *expr) override {
+    return BaseListener ? BaseListener->appliedSolution(solution, expr) : expr;
+  }
+
+  void preCheckFailed(Expr *expr) override {
+    if (BaseListener)
+      BaseListener->preCheckFailed(expr);
+    maybeProduceFallbackDiagnostic(expr);
+  }
+
+  void constraintGenerationFailed(Expr *expr) override {
+    if (BaseListener)
+      BaseListener->constraintGenerationFailed(expr);
+    maybeProduceFallbackDiagnostic(expr);
+  }
+
+  void applySolutionFailed(Solution &solution, Expr *expr) override {
+    if (BaseListener)
+      BaseListener->applySolutionFailed(solution, expr);
+
+    if (hadAnyErrors())
+      return;
+
+    // If solution involves invalid or incomplete conformances that's
+    // a probable cause of failure to apply it without producing an error,
+    // which is going to be diagnosed later, so let's not produce
+    // fallback diagnostic in this case.
+    if (llvm::any_of(
+            solution.Conformances,
+            [](const std::pair<ConstraintLocator *, ProtocolConformanceRef>
+                   &conformance) -> bool {
+              auto &ref = conformance.second;
+              return ref.isConcrete() && ref.getConcrete()->isInvalid();
+            }))
+      return;
+
+    maybeProduceFallbackDiagnostic(expr);
+  }
+
+private:
+  bool hadAnyErrors() const { return TC.Context.Diags.hadAnyError(); }
+
+  void maybeProduceFallbackDiagnostic(Expr *expr) const {
+    if (Options.contains(TypeCheckExprFlags::SubExpressionDiagnostics) ||
+        Options.contains(TypeCheckExprFlags::SuppressDiagnostics))
+      return;
+
+    // Before producing fatal error here, let's check if there are any "error"
+    // diagnostics already emitted or waiting to be emitted. Because they are
+    // a better indication of the problem.
+    if (!(hadAnyErrors() || TC.Context.hasDelayedConformanceErrors()))
+      TC.diagnose(expr->getLoc(), diag::failed_to_produce_diagnostic);
+  }
+};
+
 #pragma mark High-level entry points
 Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
                                       TypeLoc convertType,
                                       ContextualTypePurpose convertTypePurpose,
                                       TypeCheckExprOptions options,
                                       ExprTypeCheckListener *listener,
                                       ConstraintSystem *baseCS) {
+  FallbackDiagnosticListener diagListener(*this, options, listener);
+  return typeCheckExpressionImpl(expr, dc, convertType, convertTypePurpose,
+                                 options, diagListener, baseCS);
+}
+
+Type TypeChecker::typeCheckExpressionImpl(Expr *&expr, DeclContext *dc,
+                                          TypeLoc convertType,
+                                          ContextualTypePurpose convertTypePurpose,
+                                          TypeCheckExprOptions options,
+                                          ExprTypeCheckListener &listener,
+                                          ConstraintSystem *baseCS) {
   FrontendStatsTracer StatsTracer(Context.Stats, "typecheck-expr", expr);
   PrettyStackTraceExpr stackTrace(Context, "type-checking", expr);
 
   // First, pre-check the expression, validating any types that occur in the
   // expression and folding sequence expressions.
-  if (preCheckExpression(expr, dc))
+  if (preCheckExpression(expr, dc)) {
+    listener.preCheckFailed(expr);
     return Type();
+  }
 
   // Construct a constraint system from this expression.
   ConstraintSystemOptions csOptions = ConstraintSystemFlags::AllowFixes;
@@ -2032,10 +2133,9 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
     convertTo = getOptionalType(expr->getLoc(), var);
   }
 
-  // Attempt to solve the constraint system.
   SmallVector<Solution, 4> viable;
-  if (cs.solve(expr, convertTo, listener, viable,
-               allowFreeTypeVariables))
+  // Attempt to solve the constraint system.
+  if (cs.solve(expr, convertTo, &listener, viable, allowFreeTypeVariables))
     return Type();
 
   // If the client allows the solution to have unresolved type expressions,
@@ -2049,11 +2149,9 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
 
   auto result = expr;
   auto &solution = viable[0];
-  if (listener) {
-    result = listener->foundSolution(solution, result);
-    if (!result)
-      return Type();
-  }
+  result = listener.foundSolution(solution, result);
+  if (!result)
+    return Type();
 
   if (options.contains(TypeCheckExprFlags::SkipApplyingSolution))
     return solution.simplifyType(cs.getType(expr));
@@ -2063,18 +2161,17 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
       solution, result, convertType.getType(),
       options.contains(TypeCheckExprFlags::IsDiscarded),
       options.contains(TypeCheckExprFlags::SkipMultiStmtClosures));
+
   if (!result) {
+    listener.applySolutionFailed(solution, expr);
     // Failure already diagnosed, above, as part of applying the solution.
     return Type();
   }
 
-  // If there's a listener, notify it that we've applied the solution.
-  if (listener) {
-    result = listener->appliedSolution(solution, result);
-    if (!result) {
-      return Type();
-    }
-  }
+  // Notify listener that we've applied the solution.
+  result = listener.appliedSolution(solution, result);
+  if (!result)
+    return Type();
 
   if (getLangOpts().DebugConstraintSolver) {
     auto &log = Context.TypeCheckerDebug->getStream();
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
@@ -267,6 +267,14 @@ enum class TypeCheckExprFlags {
   /// If set, a conversion constraint should be specfied so that the result of
   /// the expression is an optional type.
   ExpressionTypeMustBeOptional = 0x400,
+
+  /// FIXME(diagnostics): Once diagnostics are completely switched to new
+  /// framework, this flag could be removed as obsolete.
+  ///
+  /// If set, this is a sub-expression, and it is being re-typechecked
+  /// as part of the expression diagnostics, which is attempting to narrow
+  /// down failure location.
+  SubExpressionDiagnostics = 0x800,
 };
 
 using TypeCheckExprOptions = OptionSet<TypeCheckExprFlags>;
@@ -385,6 +393,18 @@ class ExprTypeCheckListener {
   /// failure.
   virtual Expr *appliedSolution(constraints::Solution &solution,
                                 Expr *expr);
+
+  /// Callback invoked if expression is structurally unsound and can't
+  /// be correctly processed by the constraint solver.
+  virtual void preCheckFailed(Expr *expr);
+
+  /// Callback invoked if constraint system failed to generate
+  /// constraints for a given expression.
+  virtual void constraintGenerationFailed(Expr *expr);
+
+  /// Callback invoked if application of chosen solution to
+  /// expression has failed.
+  virtual void applySolutionFailed(constraints::Solution &solution, Expr *expr);
 };
 
 /// A conditional conformance that implied some other requirements. That is, \c
@@ -1369,7 +1389,15 @@ class TypeChecker final : public LazyResolver {
                                TypeCheckExprOptions(), listener);
   }
 
+private:
+  Type typeCheckExpressionImpl(Expr *&expr, DeclContext *dc,
+                               TypeLoc convertType,
+                               ContextualTypePurpose convertTypePurpose,
+                               TypeCheckExprOptions options,
+                               ExprTypeCheckListener &listener,
+                               constraints::ConstraintSystem *baseCS);
 
+public:
   /// \brief Type check the given expression and return its type without
   /// applying the solution.
   ///
diff --git a/test/Sema/rdar38885760.swift b/test/Sema/rdar38885760.swift

Original file line number	Diff line number	Diff line change
`@@ -3912,8 +3912,9 @@ namespace {`
`3912`	`3912`	`}`
`3913`	`3913`
`3914`	`3914`	`Expr visitLazyInitializerExpr(LazyInitializerExpr expr) {`
`3915`		`- simplifyExprType(expr);`
`3916`		`- assert(expr->getType()->isEqual(expr->getSubExpr()->getType()));`
	`3915`	+ // Since `LazyInitializerExpr` should always have a type set,
	`3916`	`+ // there is no need to do anything here.`
	`3917`	`+ assert(cs.getType(expr)->isEqual(expr->getSubExpr()->getType()));`
`3917`	`3918`	`return expr;`
`3918`	`3919`	`}`
`3919`	`3920`
Original file line number	Diff line number	Diff line change
`@@ -1175,6 +1175,8 @@ ConstraintSystem::solveImpl(Expr *&expr,`
`1175`	`1175`	`if (auto generatedExpr = generateConstraints(expr))`
`1176`	`1176`	`expr = generatedExpr;`
`1177`	`1177`	`else {`
	`1178`	`+ if (listener)`
	`1179`	`+ listener->constraintGenerationFailed(expr);`
`1178`	`1180`	`return SolutionKind::Error;`
`1179`	`1181`	`}`
`1180`	`1182`
Original file line number	Diff line number	Diff line change
`@@ -1323,7 +1323,9 @@ static void synthesizeLazyGetterBody(TypeChecker &TC, AccessorDecl *Get,`
`1323`	`1323`	`// FIXME: we should really have stronger invariants than this. Leaving it`
`1324`	`1324`	`// unwrapped may expose both expressions to naive walkers`
`1325`	`1325`	`if (wasInitializerChecked) {`
	`1326`	`+ auto initType = InitValue->getType();`
`1326`	`1327`	`InitValue = new (Ctx) LazyInitializerExpr(InitValue);`
	`1328`	`+ InitValue->setType(initType);`
`1327`	`1329`	`}`
`1328`	`1330`
`1329`	`1331`	`Pattern Tmp2PBDPattern = new (Ctx) NamedPattern(Tmp2VD, /implicit*/true);`