Merge pull request #9147 from CodaFi/mutatis-mutandis

Refine parameter type 'inout' diagnostics

Author: CodaFi

include/swift/AST/DiagnosticsSema.def

@@ -2089,7 +2089,8 @@ ERROR(attr_only_on_parameters,none,
       "%0 may only be used on parameters", (StringRef))
 ERROR(attr_not_on_variadic_parameters,none,
       "%0 may not be used on variadic parameters", (StringRef))
-
+ERROR(attr_not_on_subscript_parameters,none,
+      "%0 may not be used on subscript parameters", (StringRef))
 
 ERROR(override_final,none,
       "%0 overrides a 'final' %1", (DescriptiveDeclKind, DescriptiveDeclKind))

lib/Sema/TypeCheckDecl.cpp

@@ -4184,7 +4184,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
                                      TypeResolutionOptions(),
                                      &resolver);
     isInvalid |= TC.typeCheckParameterList(SD->getIndices(), SD,
-                                           TypeResolutionOptions(),
+                                           TR_SubscriptParameters,
                                            resolver);
 
     if (isInvalid || SD->isInvalid()) {

lib/Sema/TypeCheckGeneric.cpp

@@ -990,7 +990,7 @@ static bool checkGenericSubscriptSignature(TypeChecker &tc,
   auto params = subscript->getIndices();
 
   badType |= tc.typeCheckParameterList(params, subscript,
-                                       TypeResolutionOptions(),
+                                       TR_SubscriptParameters,
                                        resolver);
 
   // Infer requirements from the pattern.

lib/Sema/TypeCheckPattern.cpp

@@ -766,17 +766,22 @@ static bool validateParameterType(ParamDecl *decl, DeclContext *DC,
     }
     decl->getTypeLoc().setType(Ty);
   }
-  // If the param is not a 'let' and it is not an 'inout'.
-  // It must be a 'var'. Provide helpful diagnostics like a shadow copy
-  // in the function body to fix the 'var' attribute.
-  if (!decl->isLet() &&
-      !decl->isImplicit() &&
-      (Ty.isNull() || !Ty->is<InOutType>()) &&
-      !hadError) {
-    auto func = dyn_cast_or_null<AbstractFunctionDecl>(DC);
-    diagnoseAndMigrateVarParameterToBody(decl, func, TC);
-    decl->setInvalid();
-    hadError = true;
+
+  // If the user did not explicitly write 'let', 'var', or 'inout', we'll let
+  // type inference figure out what went wrong in detail.
+  if (decl->getLetVarInOutLoc().isValid()) {
+    // If the param is not a 'let' and it is not an 'inout'.
+    // It must be a 'var'. Provide helpful diagnostics like a shadow copy
+    // in the function body to fix the 'var' attribute.
+    if (!decl->isLet() &&
+        !decl->isImplicit() &&
+        (Ty.isNull() || !Ty->is<InOutType>()) &&
+        !hadError) {
+      auto func = dyn_cast_or_null<AbstractFunctionDecl>(DC);
+      diagnoseAndMigrateVarParameterToBody(decl, func, TC);
+      decl->setInvalid();
+      hadError = true;
+    }
   }
 
   if (hadError)

lib/Sema/TypeCheckType.cpp

@@ -2786,14 +2786,19 @@ bool TypeResolver::resolveSILResults(TypeRepr *repr,
 
 Type TypeResolver::resolveInOutType(InOutTypeRepr *repr,
                                     TypeResolutionOptions options) {
-  // inout is only valid for function parameters.
-  if (!(options & TR_FunctionInput) &&
-      !(options & TR_ImmediateFunctionInput)) {
-    TC.diagnose(repr->getInOutLoc(),
-                (options & TR_VariadicFunctionInput)
-                    ? diag::attr_not_on_variadic_parameters
-                    : diag::attr_only_on_parameters,
-                "'inout'");
+  // inout is only valid for (non-subscript) function parameters.
+  if ((options & TR_SubscriptParameters) ||
+        (!(options & TR_FunctionInput) &&
+         !(options & TR_ImmediateFunctionInput))) {
+    decltype(diag::attr_only_on_parameters) diagID;
+    if (options & TR_SubscriptParameters) {
+      diagID = diag::attr_not_on_subscript_parameters;
+    } else if (options & TR_VariadicFunctionInput) {
+      diagID = diag::attr_not_on_variadic_parameters;
+    } else {
+      diagID = diag::attr_only_on_parameters;
+    }
+    TC.diagnose(repr->getInOutLoc(), diagID, "'inout'");
     repr->setInvalid();
     return ErrorType::get(Context);
   }

lib/Sema/TypeChecker.h

@@ -493,6 +493,9 @@ enum TypeResolutionFlags : unsigned {
 
   /// Whether we are checking the underlying type of a typealias.
   TR_TypeAliasUnderlyingType = 0x4000000,
+
+  /// Whether we are checking the parameter list of a subscript.
+  TR_SubscriptParameters = 0x8000000,
 };
 
 /// Option set describing how type resolution should work.

test/Sema/immutability.swift

@@ -573,3 +573,38 @@ func f(a : FooClass, b : LetStructMembers) {
   a.baz = 1 // expected-error {{cannot assign to property: 'baz' is a method}}
   b.f = 42     // expected-error {{cannot assign to property: 'f' is a method}}
 }
+
+// SR-2354: Reject subscript declarations with mutable parameters.
+class MutableSubscripts {
+  var x : Int = 0
+
+  subscript(x: inout Int) -> () { x += 1 } // expected-error {{'inout' may not be used on subscript parameters}}
+  subscript<T>(x: inout T) -> () { // expected-error {{'inout' may not be used on subscript parameters}}
+    fatalError()
+  }
+
+  static func initialize(from state: inout MutableSubscripts) -> MutableSubscripts {
+    return state
+  }
+}
+
+
+// SR-4214: Misleading location-less diagnostic when closure parameter type
+// is inferred to be inout.
+func sr4214() {
+  func sequence<T>(_ x : T, _ f : (T) -> T) -> T {
+    return f(x)
+  }
+
+  // expected-error@+1 {{expression type '(inout MutableSubscripts) -> ()' is ambiguous without more context}}
+  let closure = { val in val.x = 7 } as (inout MutableSubscripts) -> ()
+  var v = MutableSubscripts()
+  closure(&v)
+  // FIXME: This diagnostic isn't really all that much better
+  // expected-error@+1 {{cannot convert value of type '(inout MutableSubscripts) -> ()' to expected argument type '(_) -> _'}}
+  sequence(v) { (state : inout MutableSubscripts) -> () in
+    _ = MutableSubscripts.initialize(from: &state)
+    return ()
+  }
+}
+