Merge pull request #60062 from ahoppen/pr/placeholder-for-errors

[CS] Don’t fail constraint generation for ErrorExpr or if type fails to resolve

Author: ahoppen

include/swift/Sema/CSFix.h

@@ -295,6 +295,13 @@ enum class FixKind : uint8_t {
   /// Ignore result builder body if it has `return` statements.
   IgnoreResultBuilderWithReturnStmts,
 
+  /// Ignore `ErrorExpr` or `ErrorType` during pre-check.
+  IgnoreInvalidASTNode,
+
+  /// Ignore a named pattern whose type we couldn't infer. This issue should
+  /// already have been diagnosed elsewhere.
+  IgnoreInvalidNamedPattern,
+
   /// Resolve type of `nil` by providing a contextual type.
   SpecifyContextualTypeForNil,
 
@@ -707,6 +714,26 @@ class ContextualMismatch : public ConstraintFix {
 
   bool diagnose(const Solution &solution, bool asNote = false) const override;
 
+  bool coalesceAndDiagnose(const Solution &solution,
+                           ArrayRef<ConstraintFix *> secondaryFixes,
+                           bool asNote = false) const override {
+    // If the from type or to type is a placeholer type that corresponds to an
+    // ErrorExpr, the issue has already been diagnosed. There's no need to
+    // produce another diagnostic for the contextual mismatch complainting that
+    // a type is not convertible to a placeholder type.
+    if (auto fromPlaceholder = getFromType()->getAs<PlaceholderType>()) {
+      if (fromPlaceholder->getOriginator().is<ErrorExpr *>()) {
+        return true;
+      }
+    }
+    if (auto toPlaceholder = getToType()->getAs<PlaceholderType>()) {
+      if (toPlaceholder->getOriginator().is<ErrorExpr *>()) {
+        return true;
+      }
+    }
+    return ConstraintFix::coalesceAndDiagnose(solution, secondaryFixes, asNote);
+  }
+
   bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override;
 
   static ContextualMismatch *create(ConstraintSystem &cs, Type lhs, Type rhs,
@@ -2693,6 +2720,57 @@ class IgnoreResultBuilderWithReturnStmts final
   }
 };
 
+class IgnoreInvalidASTNode final : public ConstraintFix {
+  IgnoreInvalidASTNode(ConstraintSystem &cs, ConstraintLocator *locator)
+      : IgnoreInvalidASTNode(cs, FixKind::IgnoreInvalidASTNode, locator) {}
+
+protected:
+  IgnoreInvalidASTNode(ConstraintSystem &cs, FixKind kind,
+                       ConstraintLocator *locator)
+      : ConstraintFix(cs, kind, locator) {}
+
+public:
+  std::string getName() const override { return "ignore invalid AST node"; }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
+    return diagnose(*commonFixes.front().first);
+  }
+
+  static IgnoreInvalidASTNode *create(ConstraintSystem &cs,
+                                      ConstraintLocator *locator);
+
+  static bool classof(ConstraintFix *fix) {
+    return fix->getKind() == FixKind::IgnoreInvalidASTNode;
+  }
+};
+
+class IgnoreInvalidNamedPattern final : public ConstraintFix {
+  IgnoreInvalidNamedPattern(ConstraintSystem &cs, NamedPattern *pattern,
+                            ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::IgnoreInvalidNamedPattern, locator) {}
+
+public:
+  std::string getName() const override {
+    return "specify type for pattern match";
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
+    return diagnose(*commonFixes.front().first);
+  }
+
+  static IgnoreInvalidNamedPattern *create(ConstraintSystem &cs,
+                                           NamedPattern *pattern,
+                                           ConstraintLocator *locator);
+
+  static bool classof(ConstraintFix *fix) {
+    return fix->getKind() == FixKind::IgnoreInvalidNamedPattern;
+  }
+};
+
 class SpecifyContextualTypeForNil final : public ConstraintFix {
   SpecifyContextualTypeForNil(ConstraintSystem &cs,
                               ConstraintLocator *locator)

include/swift/Sema/ConstraintLocatorPathElts.def

@@ -239,6 +239,9 @@ CUSTOM_LOCATOR_PATH_ELT(SyntacticElement)
 /// The element of the pattern binding declaration.
 CUSTOM_LOCATOR_PATH_ELT(PatternBindingElement)
 
+/// The variable declared by a named pattern.
+SIMPLE_LOCATOR_PATH_ELT(NamedPatternDecl)
+
 #undef LOCATOR_PATH_ELT
 #undef CUSTOM_LOCATOR_PATH_ELT
 #undef SIMPLE_LOCATOR_PATH_ELT

include/swift/Sema/ConstraintSystem.h

@@ -640,6 +640,13 @@ T *getAsStmt(ASTNode node) {
   return nullptr;
 }
 
+template <typename T = Pattern>
+T *getAsPattern(ASTNode node) {
+  if (auto *P = node.dyn_cast<Pattern *>())
+    return dyn_cast_or_null<T>(P);
+  return nullptr;
+}
+
 SourceLoc getLoc(ASTNode node);
 SourceRange getSourceRange(ASTNode node);
 

lib/Sema/CSBindings.cpp

@@ -109,6 +109,16 @@ bool BindingSet::isDelayed() const {
     if (locator->directlyAt<NilLiteralExpr>())
       return true;
 
+    // When inferring the type of a variable in a pattern, delay its resolution
+    // so that we resolve type variables inside the expression as placeholders
+    // instead of marking the type of the variable itself as a placeholder. This
+    // allows us to produce more specific errors because the type variable in
+    // the expression that introduced the placeholder might be diagnosable using
+    // fixForHole.
+    if (locator->isLastElement<LocatorPathElt::NamedPatternDecl>()) {
+      return true;
+    }
+
     // It's possible that type of member couldn't be determined,
     // and if so it would be beneficial to bind member to a hole
     // early to propagate that information down to arguments,
@@ -1979,6 +1989,20 @@ TypeVariableBinding::fixForHole(ConstraintSystem &cs) const {
     return std::make_pair(fix, /*impact=*/(unsigned)10);
   }
 
+  if (auto pattern = getAsPattern<NamedPattern>(dstLocator->getAnchor())) {
+    if (dstLocator->getPath().size() == 1 &&
+        dstLocator->isLastElement<LocatorPathElt::NamedPatternDecl>()) {
+      // Not being able to infer the type of a variable in a pattern binding
+      // decl is more dramatic than anything that could happen inside the
+      // expression because we want to preferrably point the diagnostic to a
+      // part of the expression that caused us to be unable to infer the
+      // variable's type.
+      ConstraintFix *fix =
+          IgnoreInvalidNamedPattern::create(cs, pattern, dstLocator);
+      return std::make_pair(fix, /*impact=*/(unsigned)100);
+    }
+  }
+
   return None;
 }
 

lib/Sema/CSFix.cpp

@@ -1945,6 +1945,16 @@ IgnoreInvalidResultBuilderBody::create(ConstraintSystem &cs,
   return new (cs.getAllocator()) IgnoreInvalidResultBuilderBody(cs, locator);
 }
 
+bool IgnoreInvalidASTNode::diagnose(const Solution &solution,
+                                    bool asNote) const {
+  return true; // Already diagnosed by the producer of ErrorExpr or ErrorType.
+}
+
+IgnoreInvalidASTNode *IgnoreInvalidASTNode::create(ConstraintSystem &cs,
+                                                   ConstraintLocator *locator) {
+  return new (cs.getAllocator()) IgnoreInvalidASTNode(cs, locator);
+}
+
 bool SpecifyContextualTypeForNil::diagnose(const Solution &solution,
                                            bool asNote) const {
   MissingContextualTypeForNil failure(solution, getLocator());
@@ -1994,6 +2004,20 @@ IgnoreResultBuilderWithReturnStmts::create(ConstraintSystem &cs, Type builderTy,
       IgnoreResultBuilderWithReturnStmts(cs, builderTy, locator);
 }
 
+bool IgnoreInvalidNamedPattern::diagnose(const Solution &solution,
+                                         bool asNote) const {
+  // Not being able to infer the type of a pattern should already have been
+  // diagnosed on the pattern's initializer or as a structural issue of the AST.
+  return true;
+}
+
+IgnoreInvalidNamedPattern *
+IgnoreInvalidNamedPattern::create(ConstraintSystem &cs, NamedPattern *pattern,
+                                  ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      IgnoreInvalidNamedPattern(cs, pattern, locator);
+}
+
 bool SpecifyBaseTypeForOptionalUnresolvedMember::diagnose(
     const Solution &solution, bool asNote) const {
   MemberMissingExplicitBaseTypeFailure failure(solution, MemberName,

lib/Sema/CSGen.cpp

@@ -890,6 +890,11 @@ namespace {
       // that member through the base returns a value convertible to the type
       // of this expression.
       auto baseTy = CS.getType(base);
+      if (isa<ErrorExpr>(base)) {
+        return CS.createTypeVariable(
+            CS.getConstraintLocator(expr, ConstraintLocator::Member),
+            TVO_CanBindToHole);
+      }
       auto tv = CS.createTypeVariable(
                   CS.getConstraintLocator(expr, ConstraintLocator::Member),
                   TVO_CanBindToLValue | TVO_CanBindToNoEscape);
@@ -1068,19 +1073,11 @@ namespace {
     ConstraintSystem &getConstraintSystem() const { return CS; }
 
     virtual Type visitErrorExpr(ErrorExpr *E) {
-      if (!CS.isForCodeCompletion())
-        return nullptr;
-
-      // For code completion, treat error expressions that don't contain
-      // the completion location itself as holes. If an ErrorExpr contains the
-      // code completion location, a fallback typecheck is called on the
-      // ErrorExpr's OriginalExpr (valid sub-expression) if it had one,
-      // independent of the wider expression containing the ErrorExpr, so
-      // there's no point attempting to produce a solution for it.
-      if (CS.containsCodeCompletionLoc(E))
-        return nullptr;
+      CS.recordFix(
+          IgnoreInvalidASTNode::create(CS, CS.getConstraintLocator(E)));
 
-      return PlaceholderType::get(CS.getASTContext(), E);
+      return CS.createTypeVariable(CS.getConstraintLocator(E),
+                                   TVO_CanBindToHole);
     }
 
     virtual Type visitCodeCompletionExpr(CodeCompletionExpr *E) {
@@ -1374,7 +1371,11 @@ namespace {
       const auto result = TypeResolution::resolveContextualType(
           repr, CS.DC, resCtx, genericOpener, placeholderHandler);
       if (result->hasError()) {
-        return Type();
+        CS.recordFix(
+            IgnoreInvalidASTNode::create(CS, CS.getConstraintLocator(locator)));
+
+        return CS.createTypeVariable(CS.getConstraintLocator(repr),
+                                     TVO_CanBindToHole);
       }
       // Diagnose top-level usages of placeholder types.
       if (isa<PlaceholderTypeRepr>(repr->getWithoutParens())) {
@@ -1600,7 +1601,11 @@ namespace {
 
     Type visitUnresolvedSpecializeExpr(UnresolvedSpecializeExpr *expr) {
       auto baseTy = CS.getType(expr->getSubExpr());
-      
+
+      if (baseTy->isTypeVariableOrMember()) {
+        return baseTy;
+      }
+
       // We currently only support explicit specialization of generic types.
       // FIXME: We could support explicit function specialization.
       auto &de = CS.getASTContext().Diags;
@@ -2322,8 +2327,10 @@ namespace {
         }
 
         if (!varType) {
-          varType = CS.createTypeVariable(CS.getConstraintLocator(locator),
-                                          TVO_CanBindToNoEscape);
+          varType = CS.createTypeVariable(
+              CS.getConstraintLocator(pattern,
+                                      LocatorPathElt::NamedPatternDecl()),
+              TVO_CanBindToNoEscape | TVO_CanBindToHole);
 
           // If this is either a `weak` declaration or capture e.g.
           // `weak var ...` or `[weak self]`. Let's wrap type variable

lib/Sema/CSSimplify.cpp

@@ -11231,18 +11231,6 @@ ConstraintSystem::simplifyApplicableFnConstraint(
   // following: $T1 -> $T2.
   auto func1 = type1->castTo<FunctionType>();
 
-  // If a type variable representing "function type" is a hole
-  // or it could be bound to some concrete type with a help of
-  // a fix, let's propagate holes to the "input" type. Doing so
-  // provides more information to upcoming argument and result matching.
-  if (shouldAttemptFixes()) {
-    if (auto *typeVar = type2->getAs<TypeVariableType>()) {
-      auto *locator = typeVar->getImpl().getLocator();
-      if (typeVar->isPlaceholder() || hasFixFor(locator))
-        recordAnyTypeVarAsPotentialHole(func1);
-    }
-  }
-
   // Before stripping lvalue-ness and optional types, save the original second
   // type for handling `func callAsFunction` and `@dynamicCallable`
   // applications. This supports the following cases:
@@ -11257,6 +11245,29 @@ ConstraintSystem::simplifyApplicableFnConstraint(
   type2 = getFixedTypeRecursive(type2, flags, /*wantRValue=*/true);
   auto desugar2 = type2->getDesugaredType();
 
+  // If a type variable representing "function type" is a hole
+  // or it could be bound to some concrete type with a help of
+  // a fix, let's propagate holes to the "input" type. Doing so
+  // provides more information to upcoming argument and result matching.
+  if (shouldAttemptFixes()) {
+    if (auto *typeVar = type2->getAs<TypeVariableType>()) {
+      auto *locator = typeVar->getImpl().getLocator();
+      if (hasFixFor(locator)) {
+        recordAnyTypeVarAsPotentialHole(func1);
+      }
+    }
+    Type underlyingType = desugar2;
+    while (auto *MT = underlyingType->getAs<AnyMetatypeType>()) {
+      underlyingType = MT->getInstanceType();
+    }
+    underlyingType =
+        getFixedTypeRecursive(underlyingType, flags, /*wantRValue=*/true);
+    if (underlyingType->isPlaceholder()) {
+      recordAnyTypeVarAsPotentialHole(func1);
+      return SolutionKind::Solved;
+    }
+  }
+
   TypeMatchOptions subflags = getDefaultDecompositionOptions(flags);
 
   SmallVector<LocatorPathElt, 2> parts;
@@ -12947,6 +12958,12 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::RenameConflictingPatternVariables: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
+  case FixKind::IgnoreInvalidASTNode: {
+    return recordFix(fix, 10) ? SolutionKind::Error : SolutionKind::Solved;
+  }
+  case FixKind::IgnoreInvalidNamedPattern: {
+    return recordFix(fix, 100) ? SolutionKind::Error : SolutionKind::Solved;
+  }
 
   case FixKind::ExplicitlyConstructRawRepresentable: {
     // Let's increase impact of this fix for binary operators because

lib/Sema/ConstraintLocator.cpp

@@ -97,6 +97,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
   case ConstraintLocator::PackType:
   case ConstraintLocator::PackElement:
   case ConstraintLocator::PatternBindingElement:
+  case ConstraintLocator::NamedPatternDecl:
     return 0;
 
   case ConstraintLocator::FunctionArgument:
@@ -441,6 +442,11 @@ void LocatorPathElt::dump(raw_ostream &out) const {
         << llvm::utostr(patternBindingElt.getIndex());
     break;
   }
+
+  case ConstraintLocator::NamedPatternDecl: {
+    out << "named pattern decl";
+    break;
+  }
   }
 }
 
@@ -592,6 +598,12 @@ void ConstraintLocator::dump(SourceManager *sm, raw_ostream &out) const {
       out << '@';
       expr->getLoc().print(out, *sm);
     }
+  } else if (auto *pattern = anchor.dyn_cast<Pattern *>()) {
+    out << Pattern::getKindName(pattern->getKind()) << "Pattern";
+    if (sm) {
+      out << '@';
+      pattern->getLoc().print(out, *sm);
+    }
   }
 
   for (auto elt : getPath()) {

lib/Sema/ConstraintSystem.cpp

@@ -5256,6 +5256,13 @@ void constraints::simplifyLocator(ASTNode &anchor,
       continue;
     }
 
+    case ConstraintLocator::NamedPatternDecl: {
+      auto pattern = cast<NamedPattern>(anchor.get<Pattern *>());
+      anchor = pattern->getDecl();
+      path = path.slice(1);
+      break;
+    }
+
     case ConstraintLocator::ImplicitConversion:
       break;
 

test/ClangImporter/MixedSource/can_import_objc_idempotent.swift

@@ -30,7 +30,7 @@
   // expected-error@-1 {{cannot find 'CGRect' in scope}}
 
   let (r, s) = square.divided(atDistance: 50, from: .minXEdge)
-  // expected-error@-1 {{type of expression is ambiguous without more context}}
+  // expected-error@-1 {{cannot infer contextual base in reference to member 'minXEdge'}}
 #endif
 
 #if canImport(MixedWithHeader)