Merge pull request swiftlang#26912 from xedin/allow-inout-conversion-fix

[ConstraintSystem] Introduce a fix to allow conversion between `inout` types

Author: xedin

lib/Sema/CSDiag.cpp

@@ -1668,15 +1668,6 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
   if (!exprType)
     return CS.TC.Diags.hadAnyError();
 
-  // If we contextually had an inout type, and got a non-lvalue result, then
-  // we fail with a mutability error.
-  if (contextualType->is<InOutType>() && !exprType->is<LValueType>()) {
-    AssignmentFailure failure(recheckedExpr, CS, recheckedExpr->getLoc(),
-                              diag::cannot_pass_rvalue_inout_subelement,
-                              diag::cannot_pass_rvalue_inout);
-    return failure.diagnose();
-  }
-
   // If we don't have a type for the expression, then we cannot use it in
   // conversion constraint diagnostic generation.  If the types match, then it
   // must not be the contextual type that is the problem.

lib/Sema/CSDiagnostics.cpp

@@ -1567,17 +1567,7 @@ bool AssignmentFailure::diagnoseAsError() {
     Type neededType = contextualType->getInOutObjectType();
     Type actualType = getType(immInfo.first)->getInOutObjectType();
     if (!neededType->isEqual(actualType)) {
-      if (DeclDiagnostic.ID == diag::cannot_pass_rvalue_inout_subelement.ID) {
-        // We have a special diagnostic with tailored wording for this
-        // common case.
-        emitDiagnostic(Loc, diag::cannot_pass_rvalue_inout_converted,
-                       actualType, neededType)
-            .highlight(immInfo.first->getSourceRange());
-
-        if (auto inoutExpr = dyn_cast<InOutExpr>(immInfo.first))
-          fixItChangeInoutArgType(inoutExpr->getSubExpr(), actualType,
-                                  neededType);
-      } else {
+      if (DeclDiagnostic.ID != diag::cannot_pass_rvalue_inout_subelement.ID) {
         emitDiagnostic(Loc, DeclDiagnostic,
                        "implicit conversion from '" + actualType->getString() +
                            "' to '" + neededType->getString() +
@@ -1601,68 +1591,6 @@ bool AssignmentFailure::diagnoseAsError() {
   return true;
 }
 
-void AssignmentFailure::fixItChangeInoutArgType(const Expr *arg,
-                                                Type actualType,
-                                                Type neededType) const {
-  auto *DC = getDC();
-  auto *DRE = dyn_cast<DeclRefExpr>(arg);
-  if (!DRE)
-    return;
-
-  auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl());
-  if (!VD)
-    return;
-
-  // Don't emit for non-local variables.
-  // (But in script-mode files, we consider module-scoped
-  // variables in the same file to be local variables.)
-  auto VDC = VD->getDeclContext();
-  bool isLocalVar = VDC->isLocalContext();
-  if (!isLocalVar && VDC->isModuleScopeContext()) {
-    auto argFile = DC->getParentSourceFile();
-    auto varFile = VDC->getParentSourceFile();
-    isLocalVar = (argFile == varFile && argFile->isScriptMode());
-  }
-  if (!isLocalVar)
-    return;
-
-  SmallString<32> scratch;
-  SourceLoc endLoc;   // Filled in if we decide to diagnose this
-  SourceLoc startLoc; // Left invalid if we're inserting
-
-  auto isSimpleTypelessPattern = [](Pattern *P) -> bool {
-    if (auto VP = dyn_cast_or_null<VarPattern>(P))
-      P = VP->getSubPattern();
-    return P && isa<NamedPattern>(P);
-  };
-
-  auto typeRange = VD->getTypeSourceRangeForDiagnostics();
-  if (typeRange.isValid()) {
-    startLoc = typeRange.Start;
-    endLoc = typeRange.End;
-  } else if (isSimpleTypelessPattern(VD->getParentPattern())) {
-    endLoc = VD->getNameLoc();
-    scratch += ": ";
-  }
-
-  if (endLoc.isInvalid())
-    return;
-
-  scratch += neededType.getString();
-
-  // Adjust into the location where we actually want to insert
-  endLoc = Lexer::getLocForEndOfToken(getASTContext().SourceMgr, endLoc);
-
-  // Since we already adjusted endLoc, this will turn an insertion
-  // into a zero-character replacement.
-  if (!startLoc.isValid())
-    startLoc = endLoc;
-
-  emitDiagnostic(VD->getLoc(), diag::inout_change_var_type_if_possible,
-                 actualType, neededType)
-      .fixItReplaceChars(startLoc, endLoc, scratch);
-}
-
 std::pair<Expr *, Optional<OverloadChoice>>
 AssignmentFailure::resolveImmutableBase(Expr *expr) const {
   auto &cs = getConstraintSystem();
@@ -4348,3 +4276,79 @@ bool ThrowingFunctionConversionFailure::diagnoseAsError() {
                  getFromType(), getToType());
   return true;
 }
+
+bool InOutConversionFailure::diagnoseAsError() {
+  auto *anchor = getAnchor();
+  emitDiagnostic(anchor->getLoc(), diag::cannot_pass_rvalue_inout_converted,
+                 getFromType(), getToType());
+  fixItChangeArgumentType();
+  return true;
+}
+
+void InOutConversionFailure::fixItChangeArgumentType() const {
+  auto *argExpr = getAnchor();
+  auto *DC = getDC();
+
+  if (auto *IOE = dyn_cast<InOutExpr>(argExpr))
+    argExpr = IOE->getSubExpr();
+
+  auto *DRE = dyn_cast<DeclRefExpr>(argExpr);
+  if (!DRE)
+    return;
+
+  auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl());
+  if (!VD)
+    return;
+
+  // Don't emit for non-local variables.
+  // (But in script-mode files, we consider module-scoped
+  // variables in the same file to be local variables.)
+  auto VDC = VD->getDeclContext();
+  bool isLocalVar = VDC->isLocalContext();
+  if (!isLocalVar && VDC->isModuleScopeContext()) {
+    auto argFile = DC->getParentSourceFile();
+    auto varFile = VDC->getParentSourceFile();
+    isLocalVar = (argFile == varFile && argFile->isScriptMode());
+  }
+  if (!isLocalVar)
+    return;
+
+  auto actualType = getFromType();
+  auto neededType = getToType();
+
+  SmallString<32> scratch;
+  SourceLoc endLoc;   // Filled in if we decide to diagnose this
+  SourceLoc startLoc; // Left invalid if we're inserting
+
+  auto isSimpleTypelessPattern = [](Pattern *P) -> bool {
+    if (auto VP = dyn_cast_or_null<VarPattern>(P))
+      P = VP->getSubPattern();
+    return P && isa<NamedPattern>(P);
+  };
+
+  auto typeRange = VD->getTypeSourceRangeForDiagnostics();
+  if (typeRange.isValid()) {
+    startLoc = typeRange.Start;
+    endLoc = typeRange.End;
+  } else if (isSimpleTypelessPattern(VD->getParentPattern())) {
+    endLoc = VD->getNameLoc();
+    scratch += ": ";
+  }
+
+  if (endLoc.isInvalid())
+    return;
+
+  scratch += neededType.getString();
+
+  // Adjust into the location where we actually want to insert
+  endLoc = Lexer::getLocForEndOfToken(getASTContext().SourceMgr, endLoc);
+
+  // Since we already adjusted endLoc, this will turn an insertion
+  // into a zero-character replacement.
+  if (!startLoc.isValid())
+    startLoc = endLoc;
+
+  emitDiagnostic(VD->getLoc(), diag::inout_change_var_type_if_possible,
+                 actualType, neededType)
+      .fixItReplaceChars(startLoc, endLoc, scratch);
+}

lib/Sema/CSDiagnostics.h

@@ -574,9 +574,6 @@ class AssignmentFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 
 private:
-  void fixItChangeInoutArgType(const Expr *arg, Type actualType,
-                               Type neededType) const;
-
   /// Given an expression that has a non-lvalue type, dig into it until
   /// we find the part of the expression that prevents the entire subexpression
   /// from being mutable.  For example, in a sequence like "x.v.v = 42" we want
@@ -1509,6 +1506,24 @@ class MissingContextualConformanceFailure final : public ContextualFailure {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose a conversion mismatch between object types of `inout`
+/// argument/parameter e.g. `'inout S' argument conv 'inout P'`.
+///
+/// Even if `S` conforms to `P` there is no subtyping rule for
+/// argument type of `inout` parameter, they have to be equal.
+class InOutConversionFailure final : public ContextualFailure {
+public:
+  InOutConversionFailure(Expr *root, ConstraintSystem &cs, Type argType,
+                         Type paramType, ConstraintLocator *locator)
+      : ContextualFailure(root, cs, argType, paramType, locator) {}
+
+  bool diagnoseAsError() override;
+
+protected:
+  /// Suggest to change a type of the argument if possible.
+  void fixItChangeArgumentType() const;
+};
+
 /// Diagnose generic argument omission e.g.
 ///
 /// ```swift

lib/Sema/CSFix.cpp

@@ -773,3 +773,17 @@ IgnoreContextualType *IgnoreContextualType::create(ConstraintSystem &cs,
   return new (cs.getAllocator())
       IgnoreContextualType(cs, resultTy, specifiedTy, locator);
 }
+
+bool AllowInOutConversion::diagnose(Expr *root, bool asNote) const {
+  auto &cs = getConstraintSystem();
+  InOutConversionFailure failure(root, cs, getFromType(), getToType(),
+                                 getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowInOutConversion *AllowInOutConversion::create(ConstraintSystem &cs,
+                                                   Type argType, Type paramType,
+                                                   ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowInOutConversion(cs, argType, paramType, locator);
+}

lib/Sema/CSFix.h

@@ -1312,6 +1312,26 @@ class IgnoreContextualType : public ContextualMismatch {
                                       ConstraintLocator *locator);
 };
 
+/// If this is an argument-to-parameter conversion which is associated with
+/// `inout` parameter, subtyping is now permitted, types have to
+/// be identical.
+class AllowInOutConversion final : public ContextualMismatch {
+  AllowInOutConversion(ConstraintSystem &cs, Type argType, Type paramType,
+                       ConstraintLocator *locator)
+      : ContextualMismatch(cs, argType, paramType, locator) {}
+
+public:
+  std::string getName() const override {
+    return "allow conversions between argument/parameter marked as `inout`";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static AllowInOutConversion *create(ConstraintSystem &cs, Type argType,
+                                      Type paramType,
+                                      ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -2512,6 +2512,19 @@ bool ConstraintSystem::repairFailures(
     if (lhs->getOptionalObjectType() && !rhs->getOptionalObjectType()) {
       conversionsOrFixes.push_back(
           ForceOptional::create(*this, lhs, lhs->getOptionalObjectType(), loc));
+      break;
+    }
+
+    // There is no subtyping between object types of inout argument/parameter.
+    if (elt.getKind() == ConstraintLocator::LValueConversion) {
+      auto result = matchTypes(lhs, rhs, ConstraintKind::Conversion,
+                               TMF_ApplyingFix, locator);
+
+      if (!result.isFailure()) {
+        conversionsOrFixes.push_back(
+            AllowInOutConversion::create(*this, lhs, rhs, loc));
+        break;
+      }
     }
 
     if (elt.getKind() != ConstraintLocator::ApplyArgToParam)

test/Constraints/lvalues.swift

@@ -90,7 +90,8 @@ z[0] = 0.0 // expected-error{{cannot assign through subscript: subscript is get-
 f2(&z[0]) // expected-error{{cannot pass immutable value as inout argument: subscript is get-only}}
 f1(&z[0]) // expected-error{{cannot pass immutable value as inout argument: subscript is get-only}}
 z[0] += 0.0 // expected-error{{left side of mutating operator isn't mutable: subscript is get-only}}
-+++z[0] // expected-error{{cannot pass immutable value as inout argument: subscript is get-only}}
++++z[0] // expected-error{{cannot convert value of type 'Double' to expected argument type 'X'}}
++++z[(i: 0, j: 0)] // expected-error{{cannot pass immutable value to mutating operator: subscript is get-only}}
 
 // settable property of an rvalue value type is non-settable:
 fz().settable_x = x // expected-error{{cannot assign to property: 'fz' returns immutable value}}
@@ -111,7 +112,7 @@ z.non_settable_x.property = 1.0 // expected-error{{cannot assign to property: 'n
 f2(&z.non_settable_x.property) // expected-error{{cannot pass immutable value as inout argument: 'non_settable_x' is a get-only property}}
 f1(&z.non_settable_x.property) // expected-error{{cannot pass immutable value as inout argument: 'non_settable_x' is a get-only property}}
 z.non_settable_x.property += 1.0 // expected-error{{left side of mutating operator isn't mutable: 'non_settable_x' is a get-only property}}
-+++z.non_settable_x.property // expected-error{{cannot pass immutable value as inout argument: 'non_settable_x' is a get-only property}}
++++z.non_settable_x.property // expected-error{{cannot convert value of type 'Double' to expected argument type 'X'}}
 
 // settable property of a non-settable reference type IS SETTABLE:
 z.non_settable_reftype.property = 1.0

test/Constraints/operator.swift

@@ -205,9 +205,8 @@ func rdar37290898(_ arr: inout [P_37290898], _ element: S_37290898?) {
 // SR-8221
 infix operator ??=
 func ??= <T>(lhs: inout T?, rhs: T?) {}
-var c: Int = 0
-c ??= 5 // expected-error{{binary operator '??=' cannot be applied to two 'Int' operands}}
-// expected-note@-1{{expected an argument list of type '(inout T?, T?)'}}
+var c: Int = 0 // expected-note {{change variable type to 'Int?' if it doesn't need to be declared as 'Int'}}
+c ??= 5 // expected-error{{inout argument could be set to a value with a type other than 'Int'; use a value declared as type 'Int?' instead}}
 
 func rdar46459603() {
   enum E {