Merge pull request swiftlang#23867 from rjmccall/var-is-too-mutated

Various improvements to the variable-is-never-mutated diagnostic

Author: rjmccall

lib/Sema/MiscDiagnostics.cpp

@@ -2276,7 +2276,8 @@ class VarDeclUsageChecker : public ASTWalker {
       vdi->second |= Flag;
   }
 
-  void markBaseOfAbstractStorageDeclStore(Expr *E, ConcreteDeclRef decl);
+  void markBaseOfStorageUse(Expr *E, ConcreteDeclRef decl, unsigned flags);
+  void markBaseOfStorageUse(Expr *E, bool isMutating);
 
   void markStoredOrInOutExpr(Expr *E, unsigned Flags);
 
@@ -2655,29 +2656,57 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
   }
 }
 
-/// Handle a store to "x.y" where 'base' is the expression for x and 'decl' is
-/// the decl for 'y'.
-void VarDeclUsageChecker::
-markBaseOfAbstractStorageDeclStore(Expr *base, ConcreteDeclRef decl) {
-  // If the base is a class or an rvalue, then this store just loads the base.
-  if (base->getType()->isAnyClassReferenceType() ||
-      !(base->getType()->hasLValueType() || base->isSemanticallyInOutExpr())) {
+/// Handle a use of "x.y" or "x[0]" where 'base' is the expression for x and
+/// 'decl' is the property or subscript.
+///
+/// TODO: Rip this out and just rely on LValueAccessKind.
+void VarDeclUsageChecker::markBaseOfStorageUse(Expr *base, ConcreteDeclRef decl,
+                                               unsigned flags) {
+  // If the base is an rvalue, then we know that this is a non-mutating access.
+  // Note that we can have mutating accesses even when the base has class or
+  // metatype type due to protocols and protocol extensions.
+  if (!base->getType()->hasLValueType() &&
+      !base->isSemanticallyInOutExpr()) {
     base->walk(*this);
     return;
   }
 
-  // If the store is to a non-mutating member, then this is just a load, even
-  // if the base is an inout expr.
-  auto *ASD = cast<AbstractStorageDecl>(decl.getDecl());
-  if (ASD->isSettable(nullptr) && !ASD->isSetterMutating()) {
-    // Sema conservatively converts the base to inout expr when it is an lvalue.
-    // Look through it because we know it isn't actually doing a load/store.
-    if (auto *ioe = dyn_cast<InOutExpr>(base))
-      base = ioe->getSubExpr();
+  // Compute whether this access is to a mutating member.
+  auto *ASD = dyn_cast_or_null<AbstractStorageDecl>(decl.getDecl());
+  bool isMutating = false;
+  if (!ASD) {
+    // If there's no abstract storage declaration (which should hopefully
+    // only happen with invalid code), treat the base access as mutating if
+    // the subobject is being mutated and the base type is not a class
+    // or metatype.
+    if (flags & RK_Written) {
+      Type type = base->getType()->getRValueType()->getInOutObjectType();
+      if (!type->isAnyClassReferenceType() && !type->is<AnyMetatypeType>())
+        isMutating = true;
+    }
+  } else {
+    // Otherwise, consider whether the accessors are mutating.
+    if (flags & RK_Read)
+      isMutating |= ASD->isGetterMutating();
+    if (flags & RK_Written)
+      isMutating |= ASD->isSettable(nullptr) && ASD->isSetterMutating();
+  }
+
+  markBaseOfStorageUse(base, isMutating);
+}
+
+void VarDeclUsageChecker::markBaseOfStorageUse(Expr *base, bool isMutating) {
+  // CSApply sometimes wraps the base in an InOutExpr just because the
+  // base is an l-value; look through that so we can get more precise
+  // checking.
+  if (auto *ioe = dyn_cast<InOutExpr>(base))
+    base = ioe->getSubExpr();
+
+  if (!isMutating) {
     base->walk(*this);
     return;
   }
-  
+
   // Otherwise this is a read and write of the base.
   return markStoredOrInOutExpr(base, RK_Written|RK_Read);
 }
@@ -2712,36 +2741,33 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) {
   if (auto *SE = dyn_cast<SubscriptExpr>(E)) {
     // The index of the subscript is evaluated as an rvalue.
     SE->getIndex()->walk(*this);
-    if (SE->hasDecl())
-      markBaseOfAbstractStorageDeclStore(SE->getBase(), SE->getDecl());
-    else  // FIXME: Should not be needed!
-      markStoredOrInOutExpr(SE->getBase(), RK_Written|RK_Read);
-    
+    markBaseOfStorageUse(SE->getBase(), SE->getDecl(), Flags);
     return;
   }
 
   // Likewise for key path applications. An application of a WritableKeyPath
-  // reads and writes its base.
+  // reads and writes its base; an application of a ReferenceWritableKeyPath
+  // only reads its base; the other KeyPath types cannot be written at all.
   if (auto *KPA = dyn_cast<KeyPathApplicationExpr>(E)) {
     auto &C = KPA->getType()->getASTContext();
     KPA->getKeyPath()->walk(*this);
-    if (KPA->getKeyPath()->getType()->getAnyNominal()
-          == C.getWritableKeyPathDecl())
-      markStoredOrInOutExpr(KPA->getBase(), RK_Written|RK_Read);
-    if (KPA->getKeyPath()->getType()->getAnyNominal()
-          == C.getReferenceWritableKeyPathDecl())
-      markStoredOrInOutExpr(KPA->getBase(), RK_Read);
+
+    bool isMutating =
+      (Flags & RK_Written) &&
+      KPA->getKeyPath()->getType()->getAnyNominal()
+        == C.getWritableKeyPathDecl();
+    markBaseOfStorageUse(KPA->getBase(), isMutating);
     return;
   }
 
   if (auto *ioe = dyn_cast<InOutExpr>(E))
     return markStoredOrInOutExpr(ioe->getSubExpr(), RK_Written|RK_Read);
 
   if (auto *MRE = dyn_cast<MemberRefExpr>(E)) {
-    markBaseOfAbstractStorageDeclStore(MRE->getBase(), MRE->getMember());
+    markBaseOfStorageUse(MRE->getBase(), MRE->getMember(), Flags);
     return;
   }
-  
+
   if (auto *TEE = dyn_cast<TupleElementExpr>(E))
     return markStoredOrInOutExpr(TEE->getBase(), Flags);
 
@@ -2750,6 +2776,12 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) {
 
   if (auto *BOE = dyn_cast<BindOptionalExpr>(E))
     return markStoredOrInOutExpr(BOE->getSubExpr(), Flags);
+
+  // Bind existential expressions.
+  if (auto *OEE = dyn_cast<OpenExistentialExpr>(E)) {
+    OpaqueValueMap[OEE->getOpaqueValue()] = OEE->getExistentialValue();
+    return markStoredOrInOutExpr(OEE->getSubExpr(), Flags);
+  }
 
   // If this is an OpaqueValueExpr that we've seen a mapping for, jump to the
   // mapped value.
@@ -2788,8 +2820,16 @@ std::pair<bool, Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
     if (auto VD = dyn_cast<VarDecl>(MRE->getMember().getDecl())) {
       if (AssociatedGetter == VD && AssociatedGetterRefExpr == nullptr)
         AssociatedGetterRefExpr = MRE;
+      markBaseOfStorageUse(MRE->getBase(), MRE->getMember(), RK_Read);
+      return { false, E };
     }
   }
+  if (auto SE = dyn_cast<SubscriptExpr>(E)) {
+    SE->getIndex()->walk(*this);
+    markBaseOfStorageUse(SE->getBase(), SE->getDecl(), RK_Read);
+    return { false, E };
+  }
+
   // If this is an AssignExpr, see if we're mutating something that we know
   // about.
   if (auto *assign = dyn_cast<AssignExpr>(E)) {
@@ -2810,6 +2850,13 @@ std::pair<bool, Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
   // If we see an OpenExistentialExpr, remember the mapping for its OpaqueValue.
   if (auto *oee = dyn_cast<OpenExistentialExpr>(E))
     OpaqueValueMap[oee->getOpaqueValue()] = oee->getExistentialValue();
+
+  // Visit bindings.
+  if (auto ove = dyn_cast<OpaqueValueExpr>(E)) {
+    if (auto mapping = OpaqueValueMap.lookup(ove))
+      mapping->walk(*this);
+    return { false, E };
+  }
 
   // If we saw an ErrorExpr, take note of this.
   if (isa<ErrorExpr>(E))

test/Constraints/anyhashable-collection-cast.swift

@@ -18,6 +18,6 @@ func test() {
 
 func testLValueCoerce() {
 	var lvalue = "lvalue"
-	var map: [AnyHashable : Any] = [lvalue: lvalue]
+	let map: [AnyHashable : Any] = [lvalue: lvalue]
 	lvalue = map[lvalue] as! String
 }

test/decl/var/usage.swift

@@ -96,22 +96,90 @@ func testInOut(_ x : inout Int) {  // Ok.
 
 struct TestStruct {
   var property = 42
-}
 
+  var mutatingProperty: Int {
+    mutating get { return 0 }
+    mutating set {}
+  }
+  var nonmutatingProperty: Int {
+    nonmutating get { return 0 }
+    nonmutating set {}
+  }
+  subscript(mutating index: Int) -> Int {
+    mutating get { return 0 }
+    mutating set {}
+  }
+  subscript(nonmutating index: Int) -> Int {
+    nonmutating get { return 0 }
+    nonmutating set {}
+  }
+}
 
 func testStructMember() -> TestStruct {
   var x = TestStruct()  // ok
   x.property = 17
   return x
 }
 
+func testMutatingProperty_get() -> TestStruct {
+  var x = TestStruct()  // ok
+  _ = x.mutatingProperty
+  return x
+}
+
+func testMutatingProperty_set() -> TestStruct {
+  var x = TestStruct()  // ok
+  x.mutatingProperty = 17
+  return x
+}
+
+func testNonmutatingProperty_get() -> TestStruct {
+  var x = TestStruct()  // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  _ = x.nonmutatingProperty
+  return x
+}
+
+func testNonmutatingProperty_set() -> TestStruct {
+  var x = TestStruct()  // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  x.nonmutatingProperty = 17
+  return x
+}
+
+func testMutatingSubscript_get() -> TestStruct {
+  var x = TestStruct()  // ok
+  _ = x[mutating: 4]
+  return x
+}
+
+func testMutatingSubscript_set() -> TestStruct {
+  var x = TestStruct()  // ok
+  x[mutating: 4] = 17
+  return x
+}
+
+func testNonmutatingSubscript_get() -> TestStruct {
+  var x = TestStruct()  // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  _ = x[nonmutating: 4]
+  return x
+}
+
+func testNonmutatingSubscript_set() -> TestStruct {
+  var x = TestStruct()  // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  x[nonmutating: 4] = 17
+  return x
+}
 
 func testSubscript() -> [Int] {
   var x = [1,2,3] // ok
   x[1] = 27
   return x
 }
 
+func testSubscriptNeverMutated() -> [Int] {
+  var x = [1,2,3] // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  _ = x[1]
+  return x
+}
 
 func testTuple(_ x : Int) -> Int {
   var x = x
@@ -178,7 +246,10 @@ func testBuildConfigs() {
 protocol Fooable {
   mutating func mutFoo()
   func immutFoo()
+  var mutatingProperty: Int { mutating get mutating set }
+  var nonmutatingProperty: Int { nonmutating get nonmutating set }
 }
+
 func testOpenExistential(_ x: Fooable,
                          y: Fooable) {
   var x = x
@@ -187,6 +258,25 @@ func testOpenExistential(_ x: Fooable,
   y.immutFoo()
 }
 
+func testOpenExistential_mutatingProperty_get(p: Fooable) {
+  var x = p
+  _ = x.mutatingProperty
+}
+
+func testOpenExistential_mutatingProperty_set(p: Fooable) {
+  var x = p
+  x.mutatingProperty = 4
+}
+
+func testOpenExistential_nonmutatingProperty_get(p: Fooable) {
+  var x = p // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  _ = x.nonmutatingProperty
+}
+
+func testOpenExistential_nonmutatingProperty_set(p: Fooable) {
+  var x = p // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
+  x.nonmutatingProperty = 4
+}
 
 func couldThrow() throws {}
 

test/multifile/lazy.swift

@@ -9,3 +9,9 @@ func test1(s: Test1) {
 func test2(s: Test2) {
   _ = s.property
 }
+
+// rdar://49482742 - shouldn't warn about 's' never being mutated
+func test3() {
+  var s = Test1()
+  _ = s.property
+}

test/stdlib/StringCompatibilityDiagnostics4.swift

@@ -1,7 +1,7 @@
 // RUN: %target-swift-frontend -typecheck -swift-version 4 %s -verify
 
 func testPopFirst() {
-  var str = "abc"
+  let str = "abc"
   var charView: String.CharacterView // expected-warning{{'CharacterView' is deprecated: Please use String directly}}
   charView = str.characters // expected-warning{{'characters' is deprecated: Please use String directly}}
   dump(charView)

test/type/infer/local_variables.swift

@@ -20,11 +20,11 @@ func infer_type(_ i: Int, f: Float) {
 
 func infer_generic_args() {
   // Simple types
-  var x : Dictionary = ["Hello" : 1]
+  let x : Dictionary = ["Hello" : 1]
   var i : Int = x["Hello"]!
 
   // Tuples
-  var (d, s) : (Dictionary, Array) = ( ["Hello" : 1], [1, 2, 3] )
+  let (d, s) : (Dictionary, Array) = ( ["Hello" : 1], [1, 2, 3] )
   i = d["Hello"]!
   i = s[i]