Merge pull request #17653 from jckarter/no-unapplied-mutating-method-references-4.2

jckarter · web-flow · commit 607903271e74 · 2018-06-30T21:42:29.000-07:00
[4.2] Sema: Diagnose completely unapplied references to mutating methods.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2702,12 +2702,19 @@ NOTE(ambiguous_because_of_trailing_closure,none,
 // Partial application of foreign functions not supported
 ERROR(partial_application_of_function_invalid,none,
       "partial application of %select{"
-        "function with 'inout' parameters|"
         "'mutating' method|"
         "'super.init' initializer chain|"
         "'self.init' initializer delegation"
       "}0 is not allowed",
       (unsigned))
+WARNING(partial_application_of_function_invalid_swift4,none,
+      "partial application of %select{"
+        "'mutating' method|"
+        "'super.init' initializer chain|"
+        "'self.init' initializer delegation"
+      "}0 is not allowed; calling the function has undefined behavior and will "
+      "be an error in future Swift versions",
+      (unsigned))
 
 ERROR(self_assignment_var,none,
       "assigning a variable to itself", ())
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -86,14 +86,18 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
     // Selector for the partial_application_of_function_invalid diagnostic
     // message.
     struct PartialApplication {
-      unsigned level : 29;
       enum : unsigned {
-        Function,
         MutatingMethod,
         SuperInit,
         SelfInit,
       };
-      unsigned kind : 3;
+      enum : unsigned {
+        Error,
+        CompatibilityWarning,
+      };
+      unsigned compatibilityWarning: 1;
+      unsigned kind : 2;
+      unsigned level : 29;
     };
 
     // Partial applications of functions that are not permitted.  This is
@@ -104,49 +108,18 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
     ~DiagnoseWalker() override {
       for (auto &unapplied : InvalidPartialApplications) {
         unsigned kind = unapplied.second.kind;
-        TC.diagnose(unapplied.first->getLoc(),
-                    diag::partial_application_of_function_invalid,
-                    kind);
-      }
-    }
-
-    /// If this is an application of a function that cannot be partially
-    /// applied, arrange for us to check that it gets fully applied.
-    void recordUnsupportedPartialApply(ApplyExpr *expr, Expr *fnExpr) {
-
-      if (isa<OtherConstructorDeclRefExpr>(fnExpr)) {
-        auto kind = expr->getArg()->isSuperExpr()
-                  ? PartialApplication::SuperInit
-                  : PartialApplication::SelfInit;
-
-        // Partial applications of delegated initializers aren't allowed, and
-        // don't really make sense to begin with.
-        InvalidPartialApplications.insert({ expr, {1, kind} });
-        return;
-      }
-
-      auto fnDeclRef = dyn_cast<DeclRefExpr>(fnExpr);
-      if (!fnDeclRef)
-        return;
-
-      auto fn = dyn_cast<FuncDecl>(fnDeclRef->getDecl());
-      if (!fn)
-        return;
-
-      unsigned kind =
-        fn->isInstanceMember() ? PartialApplication::MutatingMethod
-                               : PartialApplication::Function;
-      
-      // Functions with inout parameters cannot be partially applied.
-      if (!expr->getArg()->getType()->isMaterializable()) {
-        // We need to apply all argument clauses.
-        InvalidPartialApplications.insert({
-          fnExpr, {fn->getNumParameterLists(), kind}
-        });
+        if (unapplied.second.compatibilityWarning) {
+          TC.diagnose(unapplied.first->getLoc(),
+                      diag::partial_application_of_function_invalid_swift4,
+                      kind);
+        } else {
+          TC.diagnose(unapplied.first->getLoc(),
+                      diag::partial_application_of_function_invalid,
+                      kind);
+        }
       }
     }
 
-    /// This method is called in post-order over the AST to validate that
     /// methods are fully applied when they can't support partial application.
     void checkInvalidPartialApplication(Expr *E) {
       if (auto AE = dyn_cast<ApplyExpr>(E)) {
@@ -157,8 +130,18 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
           fnExpr = dotSyntaxExpr->getRHS();
 
         // Check to see if this is a potentially unsupported partial
-        // application.
-        recordUnsupportedPartialApply(AE, fnExpr);
+        // application of a constructor delegation.
+        if (isa<OtherConstructorDeclRefExpr>(fnExpr)) {
+          auto kind = AE->getArg()->isSuperExpr()
+                    ? PartialApplication::SuperInit
+                    : PartialApplication::SelfInit;
+
+          // Partial applications of delegated initializers aren't allowed, and
+          // don't really make sense to begin with.
+          InvalidPartialApplications.insert(
+            {E, {PartialApplication::Error, kind, 1}});
+          return;
+        }
 
         // If this is adding a level to an active partial application, advance
         // it to the next level.
@@ -172,11 +155,36 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
         InvalidPartialApplications.erase(foundApplication);
         if (level > 1) {
           // We have remaining argument clauses.
-          InvalidPartialApplications.insert({ AE, {level - 1, kind} });
+          // Partial applications were always diagnosed in Swift 4 and before,
+          // so there's no need to preserve the compatibility warning bit.
+          InvalidPartialApplications.insert(
+            {AE, {PartialApplication::Error, kind, level - 1}});
         }
         return;
       }
+      
+      /// If this is a reference to a mutating method, it cannot be partially
+      /// applied or even referenced without full application, so arrange for
+      /// us to check that it gets fully applied.
+      auto fnDeclRef = dyn_cast<DeclRefExpr>(E);
+      if (!fnDeclRef)
+        return;
+
+      auto fn = dyn_cast<FuncDecl>(fnDeclRef->getDecl());
+      if (!fn || !fn->isInstanceMember() || !fn->isMutating())
+        return;
 
+      // Swift 4 and earlier failed to diagnose a reference to a mutating method
+      // without any applications at all, which would get miscompiled into a
+      // function with undefined behavior. Warn for source compatibility.
+      auto errorBehavior = TC.Context.LangOpts.isSwiftVersionAtLeast(5)
+        ? PartialApplication::Error
+        : PartialApplication::CompatibilityWarning;
+
+      InvalidPartialApplications.insert(
+        {fnDeclRef, {errorBehavior,
+                     PartialApplication::MutatingMethod,
+                     fn->getNumParameterLists()}});
     }
 
     // Not interested in going outside a basic expression.
diff --git a/test/Compatibility/members.swift b/test/Compatibility/members.swift
@@ -7,5 +7,4 @@ struct X {
 
 func g0(_: (inout X) -> (Float) -> ()) {}
 
-// This becomes an error in Swift 4 mode -- probably a bug
-g0(X.f1)
+g0(X.f1) // expected-warning{{partial application of 'mutating' method}}
diff --git a/test/Constraints/members.swift b/test/Constraints/members.swift
@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -swift-version 4
+// RUN: %target-typecheck-verify-swift -swift-version 5
 
 ////
 // Members of structs
@@ -28,7 +28,7 @@ func g0(_: (inout X) -> (Float) -> ()) {}
 _ = x.f0(i)
 x.f0(i).f1(i)
 
-g0(X.f1)
+g0(X.f1) // expected-error{{partial application of 'mutating' method}}
 
 _ = x.f0(x.f2(1))
 _ = x.f0(1).f2(i)
diff --git a/test/Constraints/mutating_members.swift b/test/Constraints/mutating_members.swift
@@ -0,0 +1,22 @@
+// RUN: %target-swift-frontend -typecheck -verify -swift-version 5 %s
+
+struct Foo {
+  mutating func boom() {}
+}
+
+let x = Foo.boom // expected-error{{partial application of 'mutating' method}}
+var y = Foo()
+let z0 = y.boom // expected-error{{partial application of 'mutating' method}}
+let z1 = Foo.boom(&y) // expected-error{{partial application of 'mutating' method}}
+
+func fromLocalContext() -> (inout Foo) -> () -> () {
+  return Foo.boom // expected-error{{partial application of 'mutating' method}}
+}
+func fromLocalContext2(x: inout Foo, y: Bool) -> () -> () {
+  if y {
+    return x.boom // expected-error{{partial application of 'mutating' method}}
+  } else {
+    return Foo.boom(&x) // expected-error{{partial application of 'mutating' method}}
+  }
+}
+
diff --git a/test/Constraints/mutating_members_compat.swift b/test/Constraints/mutating_members_compat.swift
@@ -0,0 +1,22 @@
+// RUN: %target-swift-frontend -typecheck -verify -swift-version 4 %s
+
+struct Foo {
+  mutating func boom() {}
+}
+
+let x = Foo.boom // expected-warning{{partial application of 'mutating' method}}
+var y = Foo()
+let z0 = y.boom // expected-error{{partial application of 'mutating' method}}
+let z1 = Foo.boom(&y) // expected-error{{partial application of 'mutating' method}}
+
+func fromLocalContext() -> (inout Foo) -> () -> () {
+  return Foo.boom // expected-warning{{partial application of 'mutating' method}}
+}
+func fromLocalContext2(x: inout Foo, y: Bool) -> () -> () {
+  if y {
+    return x.boom // expected-error{{partial application of 'mutating' method}}
+  } else {
+    return Foo.boom(&x) // expected-error{{partial application of 'mutating' method}}
+  }
+}
+
