Merge pull request #59210 from xedin/conflicting-patterns-in-case

[Diagnostics] Diagnose conflicting pattern variables

Author: xedin

include/swift/Sema/CSFix.h

@@ -398,6 +398,10 @@ enum class FixKind : uint8_t {
   /// Coerce a result type of a call to a particular existential type
   /// by adding `as any <#Type#>`.
   AddExplicitExistentialCoercion,
+
+  /// For example `.a(let x), .b(let x)` where `x` gets bound to different
+  /// types.
+  RenameConflictingPatternVariables,
 };
 
 class ConstraintFix {
@@ -3019,6 +3023,50 @@ class AddExplicitExistentialCoercion final : public ConstraintFix {
                                                 ConstraintLocator *locator);
 };
 
+class RenameConflictingPatternVariables final
+    : public ConstraintFix,
+      private llvm::TrailingObjects<RenameConflictingPatternVariables,
+                                    VarDecl *> {
+  friend TrailingObjects;
+
+  Type ExpectedType;
+  unsigned NumConflicts;
+
+  RenameConflictingPatternVariables(ConstraintSystem &cs, Type expectedTy,
+                                    ArrayRef<VarDecl *> conflicts,
+                                    ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::RenameConflictingPatternVariables, locator),
+        ExpectedType(expectedTy), NumConflicts(conflicts.size()) {
+    std::uninitialized_copy(conflicts.begin(), conflicts.end(),
+                            getConflictingBuffer().begin());
+  }
+
+  MutableArrayRef<VarDecl *> getConflictingBuffer() {
+    return {getTrailingObjects<VarDecl *>(), NumConflicts};
+  }
+
+public:
+  std::string getName() const override { return "rename pattern variables"; }
+
+  ArrayRef<VarDecl *> getConflictingVars() const {
+    return {getTrailingObjects<VarDecl *>(), NumConflicts};
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override {
+    return diagnose(*commonFixes.front().first);
+  }
+
+  static RenameConflictingPatternVariables *
+  create(ConstraintSystem &cs, Type expectedTy, ArrayRef<VarDecl *> conflicts,
+         ConstraintLocator *locator);
+
+  static bool classof(ConstraintFix *fix) {
+    return fix->getKind() == FixKind::RenameConflictingPatternVariables;
+  }
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSClosure.cpp

@@ -338,6 +338,8 @@ class SyntacticElementConstraintGenerator
         hadError = true;
         return;
       }
+
+      caseItem->setPattern(pattern, /*resolved=*/true);
     }
 
     // Let's generate constraints for pattern + where clause.
@@ -780,8 +782,6 @@ class SyntacticElementConstraintGenerator
       }
     }
 
-    bindSwitchCasePatternVars(context.getAsDeclContext(), caseStmt);
-
     auto *caseLoc = cs.getConstraintLocator(
         locator, LocatorPathElt::SyntacticElement(caseStmt));
 
@@ -805,10 +805,8 @@ class SyntacticElementConstraintGenerator
             locator->castLastElementTo<LocatorPathElt::SyntacticElement>()
                 .asStmt());
 
-        for (auto caseBodyVar : caseStmt->getCaseBodyVariablesOrEmptyArray()) {
-          auto parentVar = caseBodyVar->getParentVarDecl();
-          assert(parentVar && "Case body variables always have parents");
-          cs.setType(caseBodyVar, cs.getType(parentVar));
+        if (recordInferredSwitchCasePatternVars(caseStmt)) {
+          hadError = true;
         }
       }
 
@@ -935,6 +933,75 @@ class SyntacticElementConstraintGenerator
         locator->getLastElementAs<LocatorPathElt::SyntacticElement>();
     return parentElt ? parentElt->getElement().isStmt(kind) : false;
   }
+
+  bool recordInferredSwitchCasePatternVars(CaseStmt *caseStmt) {
+    llvm::SmallDenseMap<Identifier, SmallVector<VarDecl *, 2>, 4> patternVars;
+
+    auto recordVar = [&](VarDecl *var) {
+      if (!var->hasName())
+        return;
+      patternVars[var->getName()].push_back(var);
+    };
+
+    for (auto &caseItem : caseStmt->getMutableCaseLabelItems()) {
+      assert(caseItem.isPatternResolved());
+
+      auto *pattern = caseItem.getPattern();
+      pattern->forEachVariable([&](VarDecl *var) { recordVar(var); });
+    }
+
+    for (auto bodyVar : caseStmt->getCaseBodyVariablesOrEmptyArray()) {
+      if (!bodyVar->hasName())
+        continue;
+
+      const auto &variants = patternVars[bodyVar->getName()];
+
+      auto getType = [&](VarDecl *var) {
+        auto type = cs.simplifyType(cs.getType(var));
+        assert(!type->hasTypeVariable());
+        return type;
+      };
+
+      switch (variants.size()) {
+      case 0:
+        break;
+
+      case 1:
+        // If there is only one choice here, let's use it directly.
+        cs.setType(bodyVar, getType(variants.front()));
+        break;
+
+      default: {
+        // If there are multiple choices it could only mean multiple
+        // patterns e.g. `.a(let x), .b(let x), ...:`. Let's join them.
+        Type joinType = getType(variants.front());
+
+        SmallVector<VarDecl *, 2> conflicts;
+        for (auto *var : llvm::drop_begin(variants)) {
+          auto varType = getType(var);
+          // Type mismatch between different patterns.
+          if (!joinType->isEqual(varType))
+            conflicts.push_back(var);
+        }
+
+        if (!conflicts.empty()) {
+          if (!cs.shouldAttemptFixes())
+            return true;
+
+          // dfdf
+          auto *locator = cs.getConstraintLocator(bodyVar);
+          if (cs.recordFix(RenameConflictingPatternVariables::create(
+                  cs, joinType, conflicts, locator)))
+            return true;
+        }
+
+        cs.setType(bodyVar, joinType);
+      }
+      }
+    }
+
+    return false;
+  }
 };
 }
 
@@ -1342,6 +1409,8 @@ class SyntacticElementSolutionApplication
       }
     }
 
+    bindSwitchCasePatternVars(context.getAsDeclContext(), caseStmt);
+
     for (auto *expected : caseStmt->getCaseBodyVariablesOrEmptyArray()) {
       assert(expected->hasName());
       auto prev = expected->getParentVarDecl();

lib/Sema/CSDiagnostics.cpp

@@ -8225,3 +8225,12 @@ void MissingExplicitExistentialCoercion::fixIt(
                               "as " + ErasedResultType->getString(printOpts) +
                                   (requiresParens ? ")" : ""));
 }
+
+bool ConflictingPatternVariables::diagnoseAsError() {
+  for (auto *var : Vars) {
+    emitDiagnosticAt(var->getStartLoc(),
+                     diag::type_mismatch_multiple_pattern_list, getType(var),
+                     ExpectedType);
+  }
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -2764,6 +2764,41 @@ class MissingExplicitExistentialCoercion final : public FailureDiagnostic {
   bool fixItRequiresParens() const;
 };
 
+/// Diagnose situations where pattern variables with the same name
+/// have conflicting types:
+///
+/// \code
+/// enum E {
+/// case a(Int)
+/// case b(String)
+/// }
+///
+/// func test(e: E) {
+///   switch e {
+///    case .a(let x), .b(let x): ...
+///   }
+/// }
+/// \endcode
+///
+/// In this example `x` is bound to `Int` and `String` at the same
+/// time which is incorrect.
+class ConflictingPatternVariables final : public FailureDiagnostic {
+  Type ExpectedType;
+  SmallVector<VarDecl *, 4> Vars;
+
+public:
+  ConflictingPatternVariables(const Solution &solution, Type expectedTy,
+                              ArrayRef<VarDecl *> conflicts,
+                              ConstraintLocator *locator)
+      : FailureDiagnostic(solution, locator),
+        ExpectedType(resolveType(expectedTy)),
+        Vars(conflicts.begin(), conflicts.end()) {
+    assert(!Vars.empty());
+  }
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -2449,3 +2449,21 @@ AddExplicitExistentialCoercion::create(ConstraintSystem &cs, Type resultTy,
   return new (cs.getAllocator())
       AddExplicitExistentialCoercion(cs, resultTy, locator);
 }
+
+bool RenameConflictingPatternVariables::diagnose(const Solution &solution,
+                                                 bool asNote) const {
+  ConflictingPatternVariables failure(solution, ExpectedType,
+                                      getConflictingVars(), getLocator());
+  return failure.diagnose(asNote);
+}
+
+RenameConflictingPatternVariables *
+RenameConflictingPatternVariables::create(ConstraintSystem &cs, Type expectedTy,
+                                          ArrayRef<VarDecl *> conflicts,
+                                          ConstraintLocator *locator) {
+  unsigned size = totalSizeToAlloc<VarDecl *>(conflicts.size());
+  void *mem = cs.getAllocator().Allocate(
+      size, alignof(RenameConflictingPatternVariables));
+  return new (mem)
+      RenameConflictingPatternVariables(cs, expectedTy, conflicts, locator);
+}

lib/Sema/CSSimplify.cpp

@@ -12894,7 +12894,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::SpecifyTypeForPlaceholder:
   case FixKind::AllowAutoClosurePointerConversion:
   case FixKind::IgnoreKeyPathContextualMismatch:
-  case FixKind::NotCompileTimeConst: {
+  case FixKind::NotCompileTimeConst:
+  case FixKind::RenameConflictingPatternVariables: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
 

test/expr/closure/multi_statement.swift

@@ -515,3 +515,35 @@ func test_missing_conformance_diagnostics_in_for_sequence() {
     }
   }
 }
+
+func test_conflicting_pattern_vars() {
+  enum E {
+  case a(Int, String)
+  case b(String, Int)
+  }
+
+  func fn(_: (E) -> Void) {}
+  func fn<T>(_: (E) -> T) {}
+
+  func test(e: E) {
+    fn {
+      switch $0 {
+      case .a(let x, let y),
+           .b(let x, let y):
+        // expected-error@-1 {{pattern variable bound to type 'String', expected type 'Int'}}
+        // expected-error@-2 {{pattern variable bound to type 'Int', expected type 'String'}}
+        _ = x
+        _ = y
+      }
+    }
+
+    fn {
+      switch $0 {
+      case .a(let x, let y),
+           .b(let y, let x): // Ok
+        _ = x
+        _ = y
+      }
+    }
+  }
+}