Merge pull request #20693 from xedin/forward-autoclosure-diagnostic

[CSDiagnostics] Add custom diagnostic for invalid @autoclosure forwarding

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -1081,6 +1081,8 @@ ERROR(c_function_pointer_from_function_with_context,none,
       "%select{local function|closure}0 that captures "
       "%select{context|generic parameters|dynamic Self type}1",
       (bool, unsigned))
+ERROR(invalid_autoclosure_forwarding,none,
+      "add () to forward @autoclosure parameter", ())
 
 //------------------------------------------------------------------------------
 // MARK: Type Check Declarations

lib/Sema/CSDiagnostics.cpp

@@ -1206,3 +1206,19 @@ bool ContextualFailure::trySequenceSubsequenceFixIts(InFlightDiagnostic &diag,
 
   return false;
 }
+
+bool AutoClosureForwardingFailure::diagnoseAsError() {
+  auto path = getLocator()->getPath();
+  assert(!path.empty());
+
+  auto &last = path.back();
+  assert(last.getKind() == ConstraintLocator::ApplyArgToParam);
+
+  // We need a raw anchor here because `getAnchor()` is simplified
+  // to the argument expression.
+  auto *argExpr = getArgumentExpr(getRawAnchor(), last.getValue());
+  emitDiagnostic(argExpr->getLoc(), diag::invalid_autoclosure_forwarding)
+      .highlight(argExpr->getSourceRange())
+      .fixItInsertAfter(argExpr->getEndLoc(), "()");
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -39,6 +39,9 @@ class FailureDiagnostic {
   ConstraintSystem &CS;
   ConstraintLocator *Locator;
 
+  /// The original anchor before any simplification.
+  Expr *RawAnchor;
+  /// Simplified anchor associated with the given locator.
   Expr *Anchor;
   /// Indicates whether locator could be simplified
   /// down to anchor expression.
@@ -47,7 +50,7 @@ class FailureDiagnostic {
 public:
   FailureDiagnostic(Expr *expr, ConstraintSystem &cs,
                     ConstraintLocator *locator)
-      : E(expr), CS(cs), Locator(locator) {
+      : E(expr), CS(cs), Locator(locator), RawAnchor(locator->getAnchor()) {
     std::tie(Anchor, HasComplexLocator) = computeAnchor();
   }
 
@@ -78,6 +81,8 @@ class FailureDiagnostic {
 
   Expr *getParentExpr() const { return E; }
 
+  Expr *getRawAnchor() const { return RawAnchor; }
+
   Expr *getAnchor() const { return Anchor; }
 
   ConstraintLocator *getLocator() const { return Locator; }
@@ -592,6 +597,16 @@ class ContextualFailure final : public FailureDiagnostic {
   }
 };
 
+/// Diagnose situations when @autoclosure argument is passed to @autoclosure
+/// parameter directly without calling it first.
+class AutoClosureForwardingFailure final : public FailureDiagnostic {
+public:
+  AutoClosureForwardingFailure(ConstraintSystem &cs, ConstraintLocator *locator)
+      : FailureDiagnostic(nullptr, cs, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -202,3 +202,14 @@ ContextualMismatch *ContextualMismatch::create(ConstraintSystem &cs, Type lhs,
                                                ConstraintLocator *locator) {
   return new (cs.getAllocator()) ContextualMismatch(cs, lhs, rhs, locator);
 }
+
+bool AutoClosureForwarding::diagnose(Expr *root, bool asNote) const {
+  auto failure =
+      AutoClosureForwardingFailure(getConstraintSystem(), getLocator());
+  return failure.diagnose(asNote);
+}
+
+AutoClosureForwarding *AutoClosureForwarding::create(ConstraintSystem &cs,
+                                                     ConstraintLocator *locator) {
+  return new (cs.getAllocator()) AutoClosureForwarding(cs, locator);
+}

lib/Sema/CSFix.h

@@ -83,6 +83,12 @@ enum class FixKind : uint8_t {
   /// Fix up one of the sides of conversion to make it seem
   /// like the types are aligned.
   ContextualMismatch,
+
+  /// Fix up @autoclosure argument to the @autoclosure parameter,
+  /// to for a call to be able to foward it properly, since
+  /// @autoclosure conversions are unsupported starting from
+  /// Swift version 5.
+  AutoClosureForwarding,
 };
 
 class ConstraintFix {
@@ -379,6 +385,29 @@ class ContextualMismatch : public ConstraintFix {
                                     ConstraintLocator *locator);
 };
 
+/// Detect situations when argument of the @autoclosure parameter is itself
+/// marked as @autoclosure and is not applied. Form a fix which suggests a
+/// proper way to forward such arguments, e.g.:
+///
+/// ```swift
+/// func foo(_ fn: @autoclosure () -> Int) {}
+/// func bar(_ fn: @autoclosure () -> Int) {
+///   foo(fn) // error - fn should be called
+/// }
+/// ```
+class AutoClosureForwarding final : public ConstraintFix {
+public:
+  AutoClosureForwarding(ConstraintSystem &cs, ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::AutoClosureForwarding, locator) {}
+
+  std::string getName() const override { return "fix @autoclosure forwarding"; }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static AutoClosureForwarding *create(ConstraintSystem &cs,
+                                       ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -848,18 +848,7 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
   auto isAutoClosureArg = [&](Expr *anchor, unsigned argIdx) -> bool {
     assert(anchor);
 
-    auto *call = dyn_cast<ApplyExpr>(anchor);
-    if (!call)
-      return false;
-
-    Expr *argExpr = nullptr;
-    if (auto *PE = dyn_cast<ParenExpr>(call->getArg())) {
-      assert(argsWithLabels.size() == 1);
-      argExpr = PE->getSubExpr();
-    } else if (auto *TE = dyn_cast<TupleExpr>(call->getArg())) {
-      argExpr = TE->getElement(argIdx);
-    }
-
+    auto *argExpr = getArgumentExpr(anchor, argIdx);
     if (!argExpr)
       return false;
 
@@ -895,11 +884,15 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
       // is itself `@autoclosure` function type in Swift < 5,
       // let's fix that up by making it look like argument is
       // called implicitly.
-      if (!cs.getASTContext().isSwiftVersionAtLeast(5)) {
-        if (param.isAutoClosure() &&
-            isAutoClosureArg(locator.getAnchor(), argIdx)) {
-          argTy = argTy->castTo<FunctionType>()->getResult();
-          cs.increaseScore(SK_FunctionConversion);
+      if (param.isAutoClosure() &&
+          isAutoClosureArg(locator.getAnchor(), argIdx)) {
+        argTy = argTy->castTo<FunctionType>()->getResult();
+        cs.increaseScore(SK_FunctionConversion);
+
+        if (cs.getASTContext().isSwiftVersionAtLeast(5)) {
+          auto *fixLoc = cs.getConstraintLocator(loc);
+          if (cs.recordFix(AutoClosureForwarding::create(cs, fixLoc)))
+            return cs.getTypeMatchFailure(loc);
         }
       }
 
@@ -5431,6 +5424,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::CoerceToCheckedCast:
   case FixKind::RelabelArguments:
   case FixKind::AddConformance:
+  case FixKind::AutoClosureForwarding:
     llvm_unreachable("handled elsewhere");
   }
 

lib/Sema/ConstraintSystem.cpp

@@ -2356,3 +2356,23 @@ Expr *constraints::simplifyLocatorToAnchor(ConstraintSystem &cs,
 
   return locator->getAnchor();
 }
+
+Expr *constraints::getArgumentExpr(Expr *expr, unsigned index) {
+  Expr *argExpr = nullptr;
+  if (auto *AE = dyn_cast<ApplyExpr>(expr))
+    argExpr = AE->getArg();
+  else if (auto *UME = dyn_cast<UnresolvedMemberExpr>(expr))
+    argExpr = UME->getArgument();
+  else if (auto *SE = dyn_cast<SubscriptExpr>(expr))
+    argExpr = SE->getIndex();
+  else
+    return nullptr;
+
+  if (auto *PE = dyn_cast<ParenExpr>(argExpr)) {
+    assert(index == 0);
+    return PE->getSubExpr();
+  }
+
+  assert(isa<TupleExpr>(argExpr));
+  return cast<TupleExpr>(argExpr)->getElement(index);
+}

lib/Sema/ConstraintSystem.h

@@ -3466,6 +3466,13 @@ void simplifyLocator(Expr *&anchor,
 /// null otherwise.
 Expr *simplifyLocatorToAnchor(ConstraintSystem &cs, ConstraintLocator *locator);
 
+/// Retrieve argument at specified index from given expression.
+/// The expression could be "application", "subscript" or "member" call.
+///
+/// \returns argument expression or `nullptr` if given "base" expression
+/// wasn't of one of the kinds listed above.
+Expr *getArgumentExpr(Expr *expr, unsigned index);
+
 class DisjunctionChoice {
   unsigned Index;
   Constraint *Choice;

test/Compatibility/attr_autoclosure.swift

@@ -36,3 +36,29 @@ do {
     foo(c)
   }
 }
+
+func passAutoClosureToSubscriptAndMember(_ fn: @autoclosure () -> Int) {
+  struct S {
+    func bar(_: Int, _ fun: @autoclosure () -> Int) {}
+
+    subscript(_ fn: @autoclosure () -> Int) -> Int { return fn() }
+
+    static func foo(_: @autoclosure () -> Int) {}
+  }
+
+  let s = S()
+  let _ = s.bar(42, fn) // Ok
+  let _ = s[fn] // Ok
+  let _ = S.foo(fn) // Ok
+}
+
+func passAutoClosureToEnumCase(_ fn: @escaping @autoclosure () -> Int) {
+  enum E {
+  case baz(@autoclosure () -> Int)
+  }
+
+  let _: E = .baz(42) // Ok
+  // FIXME: This line type-checks correctly but causes a crash
+  //        somewhere SILGen if `fn` doesn't have `@escaping`.
+  let _: E = .baz(fn) // Ok
+}

test/attr/attr_autoclosure.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -swift-version 5
 
 // Simple case.
 var fn : @autoclosure () -> Int = 4  // expected-error {{'@autoclosure' may only be used on parameters}}  expected-error {{cannot convert value of type 'Int' to specified type '() -> Int'}}
@@ -167,13 +167,37 @@ func variadicAutoclosure(_ fn: @autoclosure () -> ()...) {
 // These are all arguably invalid; the autoclosure should have to be called.
 // But as long as we allow them, we shouldn't crash.
 func passNonThrowingToNonThrowingAC(_ fn: @autoclosure () -> Int) {
-  takesAutoclosure(fn)
+  takesAutoclosure(fn) // expected-error {{add () to forward @autoclosure parameter}} {{22-22=()}}
 }
 func passNonThrowingToThrowingAC(_ fn: @autoclosure () -> Int) {
-  takesThrowingAutoclosure(fn)
+  takesThrowingAutoclosure(fn) // expected-error {{add () to forward @autoclosure parameter}} {{30-30=()}}
 }
 func passThrowingToThrowingAC(_ fn: @autoclosure () throws -> Int) {
-  takesThrowingAutoclosure(fn)
+  takesThrowingAutoclosure(fn) // expected-error {{add () to forward @autoclosure parameter}} {{30-30=()}}
+}
+
+func passAutoClosureToSubscriptAndMember(_ fn: @autoclosure () -> Int) {
+  struct S {
+    func bar(_: Int, _ fun: @autoclosure () -> Int) {}
+
+    subscript(_ fn: @autoclosure () -> Int) -> Int { return fn() }
+
+    static func foo(_ fn: @autoclosure () -> Int) {}
+  }
+
+  let s = S()
+  let _ = s.bar(42, fn) // expected-error {{add () to forward @autoclosure parameter}} {{23-23=()}}
+  let _ = s[fn] // expected-error {{add () to forward @autoclosure parameter}} {{15-15=()}}
+  let _ = S.foo(fn) // expected-error {{add () to forward @autoclosure parameter}} {{19-19=()}}
+}
+
+func passAutoClosureToEnumCase(_ fn: @autoclosure () -> Int) {
+  enum E {
+    case baz(@autoclosure () -> Int)
+  }
+
+  let _: E = .baz(42) // Ok
+  let _: E = .baz(fn) // expected-error {{add () to forward @autoclosure parameter}} {{21-21=()}}
 }
 
 // rdar://problem/20591571 - Various type inference problems with @autoclosure