Merge pull request #58986 from xedin/multi-statement-closure-improvements-5.7

[5.7][ConstraintSystem] A couple of improvements to multi-statement closure handling

Author: hborla

include/swift/Sema/CSFix.h

@@ -528,6 +528,8 @@ class RelabelArguments final
 
   bool diagnose(const Solution &solution, bool asNote = false) const override;
 
+  bool diagnoseForAmbiguity(CommonFixesArray commonFixes) const override;
+
   static RelabelArguments *create(ConstraintSystem &cs,
                                   llvm::ArrayRef<Identifier> correctLabels,
                                   ConstraintLocator *locator);

lib/Sema/CSClosure.cpp

@@ -37,6 +37,9 @@ Expr *getVoidExpr(ASTContext &ctx) {
 
 /// Find any type variable references inside of an AST node.
 class TypeVariableRefFinder : public ASTWalker {
+  /// A stack of all closures the walker encountered so far.
+  SmallVector<DeclContext *> ClosureDCs;
+
   ConstraintSystem &CS;
   ASTNode Parent;
 
@@ -46,9 +49,16 @@ class TypeVariableRefFinder : public ASTWalker {
   TypeVariableRefFinder(
       ConstraintSystem &cs, ASTNode parent,
       llvm::SmallPtrSetImpl<TypeVariableType *> &referencedVars)
-      : CS(cs), Parent(parent), ReferencedVars(referencedVars) {}
+      : CS(cs), Parent(parent), ReferencedVars(referencedVars) {
+    if (auto *closure = getAsExpr<ClosureExpr>(Parent))
+      ClosureDCs.push_back(closure);
+  }
 
   std::pair<bool, Expr *> walkToExprPre(Expr *expr) override {
+    if (auto *closure = dyn_cast<ClosureExpr>(expr)) {
+      ClosureDCs.push_back(closure);
+    }
+
     if (auto *DRE = dyn_cast<DeclRefExpr>(expr)) {
       auto *decl = DRE->getDecl();
 
@@ -81,20 +91,33 @@ class TypeVariableRefFinder : public ASTWalker {
     return {true, expr};
   }
 
+  Expr *walkToExprPost(Expr *expr) override {
+    if (auto *closure = dyn_cast<ClosureExpr>(expr)) {
+      ClosureDCs.pop_back();
+    }
+    return expr;
+  }
+
   std::pair<bool, Stmt *> walkToStmtPre(Stmt *stmt) override {
     // Return statements have to reference outside result type
     // since all of them are joined by it if it's not specified
     // explicitly.
     if (isa<ReturnStmt>(stmt)) {
       if (auto *closure = getAsExpr<ClosureExpr>(Parent)) {
-        inferVariables(CS.getClosureType(closure)->getResult());
+        // Return is only viable if it belongs to a parent closure.
+        if (currentClosureDC() == closure)
+          inferVariables(CS.getClosureType(closure)->getResult());
       }
     }
 
     return {true, stmt};
   }
 
 private:
+  DeclContext *currentClosureDC() const {
+    return ClosureDCs.empty() ? nullptr : ClosureDCs.back();
+  }
+
   void inferVariables(Type type) {
     type = type->getWithoutSpecifierType();
     // Record the type variable itself because it has to
@@ -440,7 +463,7 @@ class ClosureConstraintGenerator
 
     cs.addConstraint(
         ConstraintKind::Conversion, elementType, initType,
-        cs.getConstraintLocator(contextualLocator,
+        cs.getConstraintLocator(sequenceLocator,
                                 ConstraintLocator::SequenceElementType));
 
     // Reference the makeIterator witness.
@@ -449,9 +472,10 @@ class ClosureConstraintGenerator
 
     Type makeIteratorType =
         cs.createTypeVariable(locator, TVO_CanBindToNoEscape);
-    cs.addValueWitnessConstraint(LValueType::get(sequenceType), makeIterator,
-                                 makeIteratorType, closure,
-                                 FunctionRefKind::Compound, contextualLocator);
+    cs.addValueWitnessConstraint(
+        LValueType::get(sequenceType), makeIterator, makeIteratorType,
+        closure, FunctionRefKind::Compound,
+        cs.getConstraintLocator(sequenceLocator, ConstraintLocator::Witness));
 
     // After successful constraint generation, let's record
     // solution application target with all relevant information.
@@ -1176,10 +1200,6 @@ class ClosureConstraintApplication
     if (isa<IfConfigDecl>(decl))
       return;
 
-    // Variable declaration would be handled by a pattern binding.
-    if (isa<VarDecl>(decl))
-      return;
-
     // Generate constraints for pattern binding declarations.
     if (auto patternBinding = dyn_cast<PatternBindingDecl>(decl)) {
       SolutionApplicationTarget target(patternBinding);

lib/Sema/CSDiagnostics.h

@@ -1188,6 +1188,12 @@ class MissingMemberFailure : public InvalidMemberRefFailure {
       : InvalidMemberRefFailure(solution, baseType, memberName, locator) {}
 
   SourceLoc getLoc() const override {
+    auto *locator = getLocator();
+
+    if (locator->findLast<LocatorPathElt::ClosureBodyElement>()) {
+      return constraints::getLoc(getAnchor());
+    }
+
     // Diagnostic should point to the member instead of its base expression.
     return constraints::getLoc(getRawAnchor());
   }

lib/Sema/CSFix.cpp

@@ -242,6 +242,38 @@ bool RelabelArguments::diagnose(const Solution &solution, bool asNote) const {
   return failure.diagnose(asNote);
 }
 
+bool RelabelArguments::diagnoseForAmbiguity(
+    CommonFixesArray commonFixes) const {
+  SmallPtrSet<ValueDecl *, 4> overloadChoices;
+
+  // First, let's find overload choice associated with each
+  // re-labeling fix.
+  for (const auto &fix : commonFixes) {
+    auto &solution = *fix.first;
+
+    auto calleeLocator = solution.getCalleeLocator(getLocator());
+    if (!calleeLocator)
+      return false;
+
+    auto overloadChoice = solution.getOverloadChoiceIfAvailable(calleeLocator);
+    if (!overloadChoice)
+      return false;
+
+    auto *decl = overloadChoice->choice.getDeclOrNull();
+    if (!decl)
+      return false;
+
+    (void)overloadChoices.insert(decl);
+  }
+
+  // If all of the fixes point to the same overload choice then it's
+  // exactly the same issue since the call site is static.
+  if (overloadChoices.size() == 1)
+    return diagnose(*commonFixes.front().first);
+
+  return false;
+}
+
 RelabelArguments *
 RelabelArguments::create(ConstraintSystem &cs,
                          llvm::ArrayRef<Identifier> correctLabels,

lib/Sema/CSStep.cpp

@@ -942,13 +942,10 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
         // and scoring information.
         Snapshot.reset();
 
-        // Restore original scores of outer context before
-        // trying to produce a combined solution.
-        restoreOriginalScores();
-
         // Apply all of the information deduced from the
         // conjunction (up to the point of ambiguity)
         // back to the outer context and form a joined solution.
+        unsigned numSolutions = 0;
         for (auto &solution : Solutions) {
           ConstraintSystem::SolverScope scope(CS);
 
@@ -958,24 +955,47 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
           // of the constraint system, so they have to be
           // restored right afterwards because score of the
           // element does contribute to the overall score.
-          restoreOriginalScores();
+          restoreBestScore();
+          restoreCurrentScore(solution.getFixedScore());
+
+          // Transform all of the unbound outer variables into
+          // placeholders since we are not going to solve for
+          // each ambguous solution.
+          {
+            unsigned numHoles = 0;
+            for (auto *typeVar : CS.getTypeVariables()) {
+              if (!typeVar->getImpl().hasRepresentativeOrFixed()) {
+                CS.assignFixedType(
+                    typeVar, PlaceholderType::get(CS.getASTContext(), typeVar));
+                ++numHoles;
+              }
+            }
+            CS.increaseScore(SK_Hole, numHoles);
+          }
+
+          if (CS.worseThanBestSolution())
+            continue;
 
           // Note that `worseThanBestSolution` isn't checked
           // here because `Solutions` were pre-filtered, and
           // outer score is the same for all of them.
           OuterSolutions.push_back(CS.finalize());
+          ++numSolutions;
         }
 
-        return done(/*isSuccess=*/true);
+        return done(/*isSuccess=*/numSolutions > 0);
       }
 
+      auto solution = Solutions.pop_back_val();
+      auto score = solution.getFixedScore();
+
       // Restore outer type variables and prepare to solve
       // constraints associated with outer context together
       // with information deduced from the conjunction.
-      Snapshot->setupOuterContext(Solutions.pop_back_val());
+      Snapshot->setupOuterContext(std::move(solution));
 
-      // Pretend that conjunction never happend.
-      restoreOuterState();
+      // Pretend that conjunction never happened.
+      restoreOuterState(score);
 
       // Now that all of the information from the conjunction has
       // been applied, let's attempt to solve the outer scope.
@@ -987,10 +1007,11 @@ StepResult ConjunctionStep::resume(bool prevFailed) {
   return take(prevFailed);
 }
 
-void ConjunctionStep::restoreOuterState() const {
+void ConjunctionStep::restoreOuterState(const Score &solutionScore) const {
   // Restore best/current score, since upcoming step is going to
   // work with outer scope in relation to the conjunction.
-  restoreOriginalScores();
+  restoreBestScore();
+  restoreCurrentScore(solutionScore);
 
   // Active all of the previously out-of-scope constraints
   // because conjunction can propagate type information up

lib/Sema/CSStep.h

@@ -962,8 +962,11 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
 
     // Restore best score only if conjunction fails because
     // successful outcome should keep a score set by `restoreOuterState`.
-    if (HadFailure)
-      restoreOriginalScores();
+    if (HadFailure) {
+      auto solutionScore = Score();
+      restoreBestScore();
+      restoreCurrentScore(solutionScore);
+    }
 
     if (OuterTimeRemaining) {
       auto anchor = OuterTimeRemaining->first;
@@ -1015,16 +1018,19 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
 
 private:
   /// Restore best and current scores as they were before conjunction.
-  void restoreOriginalScores() const {
-    CS.solverState->BestScore = BestScore;
+  void restoreCurrentScore(const Score &solutionScore) const {
     CS.CurrentScore = CurrentScore;
+    CS.increaseScore(SK_Fix, solutionScore.Data[SK_Fix]);
+    CS.increaseScore(SK_Hole, solutionScore.Data[SK_Hole]);
   }
 
+  void restoreBestScore() const { CS.solverState->BestScore = BestScore; }
+
   // Restore constraint system state before conjunction.
   //
   // Note that this doesn't include conjunction constraint
   // itself because we don't want to re-solve it.
-  void restoreOuterState() const;
+  void restoreOuterState(const Score &solutionScore) const;
 };
 
 } // end namespace constraints

test/Constraints/closures.swift

@@ -1170,7 +1170,6 @@ func test(arr: [[Int]]) {
   }
 
   arr.map { ($0 as? [Int]).map { A($0) } } // expected-error {{missing argument label 'arg:' in call}} {{36-36=arg: }}
-  // expected-warning@-1 {{conditional cast from '[Int]' to '[Int]' always succeeds}}
 }
 
 func closureWithCaseArchetype<T>(_: T.Type) {

test/Constraints/generics.swift

@@ -902,7 +902,8 @@ func rdar78781552() {
     // expected-error@-1 {{generic struct 'Test' requires that '(((Int) throws -> Bool) throws -> [Int])?' conform to 'RandomAccessCollection'}}
     // expected-error@-2 {{generic parameter 'Content' could not be inferred}} expected-note@-2 {{explicitly specify the generic arguments to fix this issue}}
     // expected-error@-3 {{cannot convert value of type '(((Int) throws -> Bool) throws -> [Int])?' to expected argument type '[(((Int) throws -> Bool) throws -> [Int])?]'}}
-    // expected-error@-4 {{missing argument for parameter 'filter' in call}}
+    // expected-error@-4 {{missing argument label 'data:' in call}}
+    // expected-error@-5 {{missing argument for parameter 'filter' in call}}
   }
 }
 

test/expr/closure/multi_statement.swift

@@ -234,6 +234,20 @@ func test_local_function_capturing_vars() {
   }
 }
 
+func test_test_invalid_redeclaration() {
+  func test(_: () -> Void) {
+  }
+
+  test {
+    let foo = 0 // expected-note {{'foo' previously declared here}}
+    let foo = foo // expected-error {{invalid redeclaration of 'foo'}}
+  }
+
+  test {
+    let (foo, foo) = (5, 6) // expected-error {{invalid redeclaration of 'foo'}} expected-note {{'foo' previously declared here}}
+  }
+}
+
 func test_pattern_ambiguity_doesnot_crash_compiler() {
   enum E {
   case hello(result: Int) // expected-note 2 {{found this candidate}}
@@ -350,3 +364,43 @@ func test_no_crash_with_circular_ref_due_to_error() {
     return 0
   }
 }
+
+func test_diagnosing_on_missing_member_in_case() {
+  enum E {
+    case one
+  }
+
+  func test(_: (E) -> Void) {}
+
+  test {
+    switch $0 {
+    case .one: break
+    case .unknown: break // expected-error {{type 'E' has no member 'unknown'}}
+    }
+  }
+}
+
+// Type finder shouldn't bring external closure result type
+// into the scope of an inner closure e.g. while solving
+// init of pattern binding `x`.
+func test_type_finder_doesnt_walk_into_inner_closures() {
+  func test<T>(fn: () -> T) -> T { fn() }
+
+  _ = test { // Ok
+    let x = test {
+      42
+    }
+
+    let _ = test {
+      test { "" }
+    }
+
+    // multi-statement
+    let _ = test {
+      _ = 42
+      return test { "" }
+    }
+
+    return x
+  }
+}

test/expr/unary/keypath/keypath.swift

@@ -507,6 +507,7 @@ func testLabeledSubscript() {
   // TODO: These ought to work without errors.
   let _ = \AA.[keyPath: k]
   // expected-error@-1 {{cannot convert value of type 'KeyPath<AA, Int>' to expected argument type 'Int'}}
+  // expected-error@-2 {{extraneous argument label 'keyPath:' in call}}
 
   let _ = \AA.[keyPath: \AA.[labeled: 0]] // expected-error {{extraneous argument label 'keyPath:' in call}}
   // expected-error@-1 {{cannot convert value of type 'KeyPath<AA, Int>' to expected argument type 'Int'}}