Merge pull request #24644 from xedin/assorted-diagnostics-imporovements-5.1

[5.1][Diagnostics] Assorted diagnostics imporovements

Author: xedin

include/swift/AST/Decl.h

@@ -88,6 +88,7 @@ namespace swift {
   class UnboundGenericType;
   class ValueDecl;
   class VarDecl;
+  class OpaqueReturnTypeRepr;
 
 enum class DeclKind : uint8_t {
 #define DECL(Id, Parent) Id,
@@ -2677,7 +2678,10 @@ class ValueDecl : public Decl {
 
   /// Get the decl for this value's opaque result type, if it has one.
   OpaqueTypeDecl *getOpaqueResultTypeDecl() const;
-  
+
+  /// Get the representative for this value's opaque result type, if it has one.
+  OpaqueReturnTypeRepr *getOpaqueResultTypeRepr() const;
+
   /// Set the opaque return type decl for this decl.
   ///
   /// `this` must be of a decl type that supports opaque return types, and

lib/AST/Decl.cpp

@@ -2381,6 +2381,19 @@ void ValueDecl::setOverriddenDecls(ArrayRef<ValueDecl *> overridden) {
   request.cacheResult(overriddenVec);
 }
 
+OpaqueReturnTypeRepr *ValueDecl::getOpaqueResultTypeRepr() const {
+  TypeLoc returnLoc;
+  if (auto *VD = dyn_cast<VarDecl>(this)) {
+    returnLoc = VD->getTypeLoc();
+  } else if (auto *FD = dyn_cast<FuncDecl>(this)) {
+    returnLoc = FD->getBodyResultTypeLoc();
+  } else if (auto *SD = dyn_cast<SubscriptDecl>(this)) {
+    returnLoc = SD->getElementTypeLoc();
+  }
+
+  return dyn_cast_or_null<OpaqueReturnTypeRepr>(returnLoc.getTypeRepr());
+}
+
 OpaqueTypeDecl *ValueDecl::getOpaqueResultTypeDecl() const {
   if (auto func = dyn_cast<FuncDecl>(this)) {
     return func->getOpaqueResultTypeDecl();

lib/Sema/CSDiagnostics.cpp

@@ -278,18 +278,9 @@ bool RequirementFailure::diagnoseAsError() {
         anchor->getLoc(), diag::type_does_not_conform_in_opaque_return,
         namingDecl->getDescriptiveKind(), namingDecl->getFullName(), lhs, rhs);
 
-    TypeLoc returnLoc;
-    if (auto *VD = dyn_cast<VarDecl>(namingDecl)) {
-      returnLoc = VD->getTypeLoc();
-    } else if (auto *FD = dyn_cast<FuncDecl>(namingDecl)) {
-      returnLoc = FD->getBodyResultTypeLoc();
-    } else if (auto *SD = dyn_cast<SubscriptDecl>(namingDecl)) {
-      returnLoc = SD->getElementTypeLoc();
-    }
-
-    if (returnLoc.hasLocation()) {
-      emitDiagnostic(returnLoc.getLoc(), diag::opaque_return_type_declared_here)
-          .highlight(returnLoc.getSourceRange());
+    if (auto *repr = namingDecl->getOpaqueResultTypeRepr()) {
+      emitDiagnostic(repr->getLoc(), diag::opaque_return_type_declared_here)
+          .highlight(repr->getSourceRange());
     }
     return true;
   }
@@ -1697,6 +1688,33 @@ bool MissingCallFailure::diagnoseAsError() {
   if (isa<KeyPathExpr>(baseExpr))
     return false;
 
+  auto path = getLocator()->getPath();
+  if (!path.empty()) {
+    const auto &last = path.back();
+
+    switch (last.getKind()) {
+    case ConstraintLocator::ContextualType:
+    case ConstraintLocator::ApplyArgToParam: {
+      auto fnType = getType(baseExpr)->castTo<FunctionType>();
+      assert(fnType->getNumParams() == 0);
+      emitDiagnostic(baseExpr->getLoc(), diag::missing_nullary_call,
+                     fnType->getResult())
+          .fixItInsertAfter(baseExpr->getEndLoc(), "()");
+      return true;
+    }
+
+    case ConstraintLocator::AutoclosureResult: {
+      auto &cs = getConstraintSystem();
+      auto loc = cs.getConstraintLocator(getRawAnchor(), path.drop_back(),
+                                         /*summaryFlags=*/0);
+      AutoClosureForwardingFailure failure(cs, loc);
+      return failure.diagnoseAsError();
+    }
+    default:
+      break;
+    }
+  }
+
   if (auto *DRE = dyn_cast<DeclRefExpr>(baseExpr)) {
     emitDiagnostic(baseExpr->getLoc(), diag::did_not_call_function,
                    DRE->getDecl()->getBaseName().getIdentifier())
@@ -2679,3 +2697,15 @@ bool InvalidMethodRefInKeyPath::diagnoseAsError() {
                  getName(), isForKeyPathDynamicMemberLookup());
   return true;
 }
+
+bool InvalidUseOfAddressOf::diagnoseAsError() {
+  auto *anchor = cast<AssignExpr>(getAnchor());
+  emitDiagnostic(anchor->getSrc()->getLoc(), diag::extraneous_address_of);
+  return true;
+}
+
+bool ExtraneousReturnFailure::diagnoseAsError() {
+  auto *anchor = getAnchor();
+  emitDiagnostic(anchor->getLoc(), diag::cannot_return_value_from_void_func);
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -1138,6 +1138,41 @@ class InvalidMethodRefInKeyPath final : public InvalidMemberRefInKeyPath {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose extraneous use of address of (`&`) which could only be
+/// associated with arguments to inout parameters e.g.
+///
+/// ```swift
+/// struct S {}
+///
+/// var a: S = ...
+/// var b: S = ...
+///
+/// a = &b
+/// ```
+class InvalidUseOfAddressOf final : public FailureDiagnostic {
+public:
+  InvalidUseOfAddressOf(Expr *root, ConstraintSystem &cs,
+                        ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
+/// Diagnose an attempt return something from a function which
+/// doesn't have a return type specified e.g.
+///
+/// ```swift
+/// func foo() { return 42 }
+/// ```
+class ExtraneousReturnFailure final : public FailureDiagnostic {
+public:
+  ExtraneousReturnFailure(Expr *root, ConstraintSystem &cs,
+                          ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -484,3 +484,23 @@ KeyPathContextualMismatch::create(ConstraintSystem &cs, Type lhs, Type rhs,
   return new (cs.getAllocator())
       KeyPathContextualMismatch(cs, lhs, rhs, locator);
 }
+
+bool RemoveAddressOf::diagnose(Expr *root, bool asNote) const {
+  InvalidUseOfAddressOf failure(root, getConstraintSystem(), getLocator());
+  return failure.diagnose(asNote);
+}
+
+RemoveAddressOf *RemoveAddressOf::create(ConstraintSystem &cs,
+                                         ConstraintLocator *locator) {
+  return new (cs.getAllocator()) RemoveAddressOf(cs, locator);
+}
+
+bool RemoveReturn::diagnose(Expr *root, bool asNote) const {
+  ExtraneousReturnFailure failure(root, getConstraintSystem(), getLocator());
+  return failure.diagnose(asNote);
+}
+
+RemoveReturn *RemoveReturn::create(ConstraintSystem &cs,
+                                   ConstraintLocator *locator) {
+  return new (cs.getAllocator()) RemoveReturn(cs, locator);
+}

lib/Sema/CSFix.h

@@ -57,6 +57,8 @@ enum class FixKind : uint8_t {
 
   /// Introduce a '&' to take the address of an lvalue.
   AddressOf,
+  /// Remove extraneous use of `&`.
+  RemoveAddressOf,
 
   /// Replace a coercion ('as') with a forced checked cast ('as!').
   CoerceToCheckedCast,
@@ -147,6 +149,10 @@ enum class FixKind : uint8_t {
   /// Allow an invalid reference to a member declaration as part
   /// of a key path component.
   AllowInvalidRefInKeyPath,
+
+  /// Remove `return` or default last expression of single expression
+  /// function to `Void` to conform to expected result type.
+  RemoveReturn,
 };
 
 class ConstraintFix {
@@ -853,6 +859,33 @@ class AllowInvalidRefInKeyPath final : public ConstraintFix {
                                           ConstraintLocator *locator);
 };
 
+class RemoveAddressOf final : public ConstraintFix {
+  RemoveAddressOf(ConstraintSystem &cs, ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::RemoveAddressOf, locator) {}
+
+public:
+  std::string getName() const override {
+    return "remove extraneous use of `&`";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static RemoveAddressOf *create(ConstraintSystem &cs,
+                                 ConstraintLocator *locator);
+};
+
+class RemoveReturn final : public ConstraintFix {
+  RemoveReturn(ConstraintSystem &cs, ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::RemoveReturn, locator) {}
+
+public:
+  std::string getName() const override { return "remove or omit return type"; }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static RemoveReturn *create(ConstraintSystem &cs, ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -1978,6 +1978,51 @@ bool ConstraintSystem::repairFailures(
   SmallVector<LocatorPathElt, 4> path;
   auto *anchor = locator.getLocatorParts(path);
 
+  // If there is a missing explicit call it could be:
+  //
+  // a). Contextual e.g. `let _: R = foo`
+  // b). Argument is a function value passed to parameter
+  //     which expects its result type e.g. `foo(bar)`
+  // c). Assigment destination type matches return type of
+  //     of the function value e.g. `foo = bar` or `foo = .bar`
+  auto repairByInsertingExplicitCall = [&](Type srcType, Type dstType) -> bool {
+    auto fnType = srcType->getAs<FunctionType>();
+    if (!fnType || fnType->getNumParams() > 0)
+      return false;
+
+    auto resultType = fnType->getResult();
+    // If this is situation like `x = { ... }` where closure results in
+    // `Void`, let's not suggest to call the closure, because it's most
+    // likely not intended.
+    if (anchor && isa<AssignExpr>(anchor)) {
+      auto *assignment = cast<AssignExpr>(anchor);
+      if (isa<ClosureExpr>(assignment->getSrc()) && resultType->isVoid())
+        return false;
+    }
+
+    // If left-hand side is a function type but right-hand
+    // side isn't, let's check it would be possible to fix
+    // this by forming an explicit call.
+    auto convertTo = dstType->lookThroughAllOptionalTypes();
+    // Right-hand side can't be - a function, a type variable or dependent
+    // member, or `Any` (if function conversion to `Any` didn't succeed there
+    // is something else going on e.g. problem with escapiness).
+    if (convertTo->is<FunctionType>() || convertTo->isTypeVariableOrMember() ||
+        convertTo->isAny())
+      return false;
+
+    auto result = matchTypes(resultType, dstType, ConstraintKind::Conversion,
+                             TypeMatchFlags::TMF_ApplyingFix, locator);
+
+    if (result.isSuccess()) {
+      conversionsOrFixes.push_back(
+          InsertExplicitCall::create(*this, getConstraintLocator(locator)));
+      return true;
+    }
+
+    return false;
+  };
+
   if (path.empty()) {
     if (!anchor)
       return false;
@@ -1993,21 +2038,14 @@ bool ConstraintSystem::repairFailures(
         return true;
     }
 
-    if (isa<AssignExpr>(anchor)) {
-      if (auto *fnType = lhs->getAs<FunctionType>()) {
-        // If left-hand side is a function type but right-hand
-        // side isn't, let's check it would be possible to fix
-        // this by forming an explicit call.
-        auto convertTo = rhs->lookThroughAllOptionalTypes();
-        if (!convertTo->is<FunctionType>() && !convertTo->isVoid() &&
-            fnType->getNumParams() == 0 &&
-            matchTypes(fnType->getResult(), rhs, ConstraintKind::Conversion,
-                       TypeMatchFlags::TMF_ApplyingFix, locator)
-                .isSuccess()) {
-          conversionsOrFixes.push_back(
-              InsertExplicitCall::create(*this, getConstraintLocator(locator)));
-          return true;
-        }
+    if (auto *AE = dyn_cast<AssignExpr>(anchor)) {
+      if (repairByInsertingExplicitCall(lhs, rhs))
+        return true;
+
+      if (isa<InOutExpr>(AE->getSrc())) {
+        conversionsOrFixes.push_back(
+            RemoveAddressOf::create(*this, getConstraintLocator(locator)));
+        return true;
       }
     }
 
@@ -2018,6 +2056,9 @@ bool ConstraintSystem::repairFailures(
   switch (elt.getKind()) {
   case ConstraintLocator::LValueConversion:
   case ConstraintLocator::ApplyArgToParam: {
+    if (repairByInsertingExplicitCall(lhs, rhs))
+      return true;
+
     if (lhs->getOptionalObjectType() && !rhs->getOptionalObjectType()) {
       conversionsOrFixes.push_back(
           ForceOptional::create(*this, lhs, lhs->getOptionalObjectType(),
@@ -2081,6 +2122,17 @@ bool ConstraintSystem::repairFailures(
   }
 
   case ConstraintLocator::ContextualType: {
+    auto purpose = getContextualTypePurpose();
+    if (rhs->isVoid() &&
+        (purpose == CTP_ReturnStmt || purpose == CTP_ReturnSingleExpr)) {
+      conversionsOrFixes.push_back(
+          RemoveReturn::create(*this, getConstraintLocator(locator)));
+      return true;
+    }
+
+    if (repairByInsertingExplicitCall(lhs, rhs))
+      return true;
+
     // If both types are key path, the only differences
     // between them are mutability and/or root, value type mismatch.
     if (isKnownKeyPathType(lhs) && isKnownKeyPathType(rhs)) {
@@ -2098,6 +2150,12 @@ bool ConstraintSystem::repairFailures(
     break;
   }
 
+  case ConstraintLocator::AutoclosureResult: {
+    if (repairByInsertingExplicitCall(lhs, rhs))
+      return true;
+    break;
+  }
+
   default:
     break;
   }
@@ -6476,6 +6534,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   }
 
   case FixKind::InsertCall:
+  case FixKind::RemoveReturn:
+  case FixKind::RemoveAddressOf:
   case FixKind::SkipSameTypeRequirement:
   case FixKind::SkipSuperclassRequirement:
   case FixKind::ContextualMismatch:

test/Constraints/closures.swift

@@ -55,7 +55,7 @@ func inoutToSharedConversions() {
 // Autoclosure
 func f1(f: @autoclosure () -> Int) { }
 func f2() -> Int { }
-f1(f: f2) // expected-error{{function produces expected type 'Int'; did you mean to call it with '()'?}}{{9-9=()}}
+f1(f: f2) // expected-error{{add () to forward @autoclosure parameter}}{{9-9=()}}
 f1(f: 5)
 
 // Ternary in closure
@@ -268,7 +268,7 @@ func someFunc(_ foo: ((String) -> String)?,
 
 func verify_NotAC_to_AC_failure(_ arg: () -> ()) {
   func takesAC(_ arg: @autoclosure () -> ()) {}
-  takesAC(arg) // expected-error {{function produces expected type '()'; did you mean to call it with '()'?}}
+  takesAC(arg) // expected-error {{add () to forward @autoclosure parameter}} {{14-14=()}}
 }
 
 // SR-1069 - Error diagnostic refers to wrong argument

test/Constraints/fixes.swift

@@ -39,6 +39,9 @@ func forgotCall() {
   // With overloading: only one succeeds.
   a = createB // expected-error{{function produces expected type 'B'; did you mean to call it with '()'?}}
 
+  let _: A = createB // expected-error{{function produces expected type 'B'; did you mean to call it with '()'?}} {{21-21=()}}
+  let _: B = createB // expected-error{{function produces expected type 'B'; did you mean to call it with '()'?}} {{21-21=()}}
+
   // With overloading, pick the fewest number of fixes.
   var b = f7(f4, f1) // expected-error{{function produces expected type 'B'; did you mean to call it with '()'?}}
   b.iAmAB()