Merge pull request #26525 from xedin/diag-missing-addrof-in-args

[Diagnostics] Extend missing `&` diagnostic to cover pointer conversions

Author: xedin

lib/Sema/CSDiag.cpp

@@ -2266,17 +2266,6 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
           return true;
         }
       }
-    } else if (contextDecl == CS.TC.Context.getUnsafePointerDecl() ||
-               contextDecl == CS.TC.Context.getUnsafeMutablePointerDecl() ||
-               contextDecl == CS.TC.Context.getUnsafeRawPointerDecl() ||
-               contextDecl == CS.TC.Context.getUnsafeMutableRawPointerDecl()) {
-      for (Type arg : genericType->getGenericArgs()) {
-        if (arg->isEqual(exprType) && CS.getType(expr)->hasLValueType()) {
-          diagnose(expr->getLoc(), diagID, exprType, contextualType).
-            fixItInsert(expr->getStartLoc(), "&");
-          return true;
-        }
-      }
     }
   }
 

lib/Sema/CSDiagnostics.cpp

@@ -886,9 +886,17 @@ bool MissingAddressOfFailure::diagnoseAsError() {
     return false;
 
   auto *anchor = getAnchor();
-  auto type = getType(anchor)->getRValueType();
-  emitDiagnostic(anchor->getLoc(), diag::missing_address_of, type)
-      .fixItInsert(anchor->getStartLoc(), "&");
+  auto argTy = getFromType();
+  auto paramTy = getToType();
+
+  if (paramTy->getAnyPointerElementType()) {
+    emitDiagnostic(anchor->getLoc(), diag::cannot_convert_argument_value, argTy,
+                   paramTy)
+        .fixItInsert(anchor->getStartLoc(), "&");
+  } else {
+    emitDiagnostic(anchor->getLoc(), diag::missing_address_of, argTy)
+        .fixItInsert(anchor->getStartLoc(), "&");
+  }
   return true;
 }
 

lib/Sema/CSDiagnostics.h

@@ -544,17 +544,6 @@ class MissingForcedDowncastFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
-/// Diagnose failures related to passing value of some type
-/// to `inout` parameter, without explicitly specifying `&`.
-class MissingAddressOfFailure final : public FailureDiagnostic {
-public:
-  MissingAddressOfFailure(Expr *expr, ConstraintSystem &cs,
-                          ConstraintLocator *locator)
-      : FailureDiagnostic(expr, cs, locator) {}
-
-  bool diagnoseAsError() override;
-};
-
 /// Diagnose failures related attempt to implicitly convert types which
 /// do not support such implicit converstion.
 /// "as" or "as!" has to be specified explicitly in cases like that.
@@ -750,6 +739,17 @@ class ContextualFailure : public FailureDiagnostic {
   void tryComputedPropertyFixIts(Expr *expr) const;
 };
 
+/// Diagnose failures related to passing value of some type
+/// to `inout` or pointer parameter, without explicitly specifying `&`.
+class MissingAddressOfFailure final : public ContextualFailure {
+public:
+  MissingAddressOfFailure(Expr *expr, ConstraintSystem &cs, Type argTy,
+                          Type paramTy, ConstraintLocator *locator)
+      : ContextualFailure(expr, cs, argTy, paramTy, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 /// Diagnose mismatches relating to tuple destructuring.
 class TupleContextualFailure final : public ContextualFailure {
 public:

lib/Sema/CSFix.cpp

@@ -100,13 +100,15 @@ UnwrapOptionalBase *UnwrapOptionalBase::createWithOptionalResult(
 }
 
 bool AddAddressOf::diagnose(Expr *root, bool asNote) const {
-  MissingAddressOfFailure failure(root, getConstraintSystem(), getLocator());
+  auto &cs = getConstraintSystem();
+  MissingAddressOfFailure failure(root, cs, getFromType(), getToType(),
+                                  getLocator());
   return failure.diagnose(asNote);
 }
 
-AddAddressOf *AddAddressOf::create(ConstraintSystem &cs,
-                                   ConstraintLocator *locator) {
-  return new (cs.getAllocator()) AddAddressOf(cs, locator);
+AddAddressOf *AddAddressOf::create(ConstraintSystem &cs, Type argTy,
+                                   Type paramTy, ConstraintLocator *locator) {
+  return new (cs.getAllocator()) AddAddressOf(cs, argTy, paramTy, locator);
 }
 
 bool TreatRValueAsLValue::diagnose(Expr *root, bool asNote) const {

lib/Sema/CSFix.h

@@ -307,19 +307,6 @@ class UnwrapOptionalBase final : public ConstraintFix {
                            ConstraintLocator *locator);
 };
 
-/// Introduce a '&' to take the address of an lvalue.
-class AddAddressOf final : public ConstraintFix {
-  AddAddressOf(ConstraintSystem &cs, ConstraintLocator *locator)
-      : ConstraintFix(cs, FixKind::AddressOf, locator) {}
-
-public:
-  std::string getName() const override { return "add address-of"; }
-
-  bool diagnose(Expr *root, bool asNote = false) const override;
-
-  static AddAddressOf *create(ConstraintSystem &cs, ConstraintLocator *locator);
-};
-
 // Treat rvalue as if it was an lvalue
 class TreatRValueAsLValue final : public ConstraintFix {
   TreatRValueAsLValue(ConstraintSystem &cs, ConstraintLocator *locator)
@@ -523,6 +510,21 @@ class ContextualMismatch : public ConstraintFix {
                                     ConstraintLocator *locator);
 };
 
+/// Introduce a '&' to take the address of an lvalue.
+class AddAddressOf final : public ContextualMismatch {
+  AddAddressOf(ConstraintSystem &cs, Type argTy, Type paramTy,
+               ConstraintLocator *locator)
+      : ContextualMismatch(cs, FixKind::AddressOf, argTy, paramTy, locator) {}
+
+public:
+  std::string getName() const override { return "add address-of"; }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static AddAddressOf *create(ConstraintSystem &cs, Type argTy, Type paramTy,
+                              ConstraintLocator *locator);
+};
+
 /// Detect situations where two type's generic arguments must
 /// match but are not convertible e.g.
 ///

lib/Sema/CSSimplify.cpp

@@ -2348,6 +2348,20 @@ bool ConstraintSystem::repairFailures(
             rhs)) {
       conversionsOrFixes.push_back(fix);
     }
+
+    // If argument in l-value type and parameter is `inout` or a pointer,
+    // let's see if it's generic parameter matches and suggest adding explicit
+    // `&`.
+    if (lhs->is<LValueType>() &&
+        (rhs->is<InOutType>() || rhs->getAnyPointerElementType())) {
+      auto result = matchTypes(InOutType::get(lhs->getRValueType()), rhs,
+                               ConstraintKind::ArgumentConversion,
+                               TypeMatchFlags::TMF_ApplyingFix, locator);
+
+      if (result.isSuccess())
+        conversionsOrFixes.push_back(AddAddressOf::create(
+            *this, lhs, rhs, getConstraintLocator(locator)));
+    }
     break;
   }
 
@@ -2924,8 +2938,10 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
     // T1 is convertible to T2 (by loading the value).  Note that we cannot get
     // a value of inout type as an lvalue though.
     if (type1->is<LValueType>() && !type2->is<InOutType>()) {
-      return matchTypes(type1->getWithoutSpecifierType(), type2,
-                        kind, subflags, locator);
+      auto result = matchTypes(type1->getWithoutSpecifierType(), type2, kind,
+                               subflags, locator);
+      if (result.isSuccess() || !shouldAttemptFixes())
+        return result;
     }
   }
 
@@ -3315,16 +3331,10 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
             ForceDowncast::create(*this, type2, getConstraintLocator(locator)));
     }
 
-    if (type2->is<InOutType>()) {
-      if (type1->is<LValueType>()) {
-        // If we're converting an lvalue to an inout type, add the missing '&'.
-        conversionsOrFixes.push_back(
-          AddAddressOf::create(*this, getConstraintLocator(locator)));
-      } else {
-        // If we have a concrete type that's an rvalue, "fix" it.
-        conversionsOrFixes.push_back(
+    if (!type1->is<LValueType>() && type2->is<InOutType>()) {
+      // If we have a concrete type that's an rvalue, "fix" it.
+      conversionsOrFixes.push_back(
           TreatRValueAsLValue::create(*this, getConstraintLocator(locator)));
-      }
     }
   }