Sema: More accurate VarDeclUsageChecker analysis with local functions

We used to take all the captures of a local function and treat them all
as read and write usages of vars from an outer scope. Instead, let's
refactor the analysis to walk into local functions.

Author: slavapestov

lib/Sema/MiscDiagnostics.cpp

@@ -2250,13 +2250,10 @@ class VarDeclUsageChecker : public ASTWalker {
 
   /// This is a mapping from an OpaqueValue to the expression that initialized
   /// it.
-  llvm::SmallDenseMap<OpaqueValueExpr*, Expr*> OpaqueValueMap;
+  llvm::SmallDenseMap<OpaqueValueExpr *, Expr *> OpaqueValueMap;
 
-  /// The getter associated with a setter function declaration.
-  const VarDecl *AssociatedGetter = nullptr;
-
-  /// The first reference to the associated getter.
-  const Expr *AssociatedGetterRefExpr = nullptr;
+  /// The first reference to the given property.
+  llvm::SmallDenseMap<VarDecl *, Expr *> AssociatedGetterRefExpr;
 
   /// This is a mapping from VarDecls to the if/while/guard statement that they
   /// occur in, when they are in a pattern in a StmtCondition.
@@ -2272,36 +2269,8 @@ class VarDeclUsageChecker : public ASTWalker {
   void operator=(const VarDeclUsageChecker &) = delete;
 
 public:
-  VarDeclUsageChecker(AbstractFunctionDecl *AFD)
-    : Diags(AFD->getASTContext().Diags) {
-    // If this AFD is a setter, track the parameter and the getter for
-    // the containing property so if newValue isn't used but the getter is used
-    // an error can be reported.
-    if (auto FD = dyn_cast<AccessorDecl>(AFD)) {
-      if (FD->getAccessorKind() == AccessorKind::Set) {
-        if (auto getter = dyn_cast<VarDecl>(FD->getStorage())) {
-          auto arguments = FD->getParameters();
-          VarDecls[arguments->get(0)] = RK_Defined;
-          AssociatedGetter = getter;
-        }
-      }
-    }
-  }
-
   VarDeclUsageChecker(DiagnosticEngine &Diags) : Diags(Diags) {}
 
-  VarDeclUsageChecker(VarDecl *vd) : Diags(vd->getASTContext().Diags) {
-    // Track a specific VarDecl
-    VarDecls[vd] = RK_Defined;
-    if (auto *childVd = vd->getCorrespondingCaseBodyVariable().getPtrOrNull()) {
-      VarDecls[childVd] = RK_Defined;
-    }
-  }
-
-  void suppressDiagnostics() {
-    sawError = true; // set this flag so that no diagnostics will be emitted on delete.
-  }
-    
   // After we have scanned the entire region, diagnose variables that could be
   // declared with a narrower usage kind.
   ~VarDeclUsageChecker() override;
@@ -2404,23 +2373,31 @@ class VarDeclUsageChecker : public ASTWalker {
       return false;
 
     if (auto *afd = dyn_cast<AbstractFunctionDecl>(D)) {
-      // If this is a nested function with a capture list, mark any captured
-      // variables.
+      // If this AFD is a setter, track the parameter and the getter for
+      // the containing property so if newValue isn't used but the getter is used
+      // an error can be reported.
+      if (auto FD = dyn_cast<AccessorDecl>(afd)) {
+        if (FD->getAccessorKind() == AccessorKind::Set) {
+          if (isa<VarDecl>(FD->getStorage())) {
+            auto arguments = FD->getParameters();
+            VarDecls[arguments->get(0)] = RK_Defined;
+          }
+        }
+      }
+
       if (afd->isBodyTypeChecked()) {
+        // FIXME: We don't actually need captures here anymore, but this happens
+        // to be the place where they get computed. Move this somewhere else.
         TypeChecker::computeCaptures(afd);
-        for (const auto &capture : afd->getCaptureInfo().getCaptures())
-          addMark(capture.getDecl(), RK_Read|RK_Written);
-      } else {
-        // If the body hasn't been type checked yet, be super-conservative and
-        // mark all variables as used.  This can be improved later, e.g. by
-        // walking the untype-checked body to look for things that could
-        // possibly be used.
-        VarDecls.clear();
+        return true;
       }
-      
-      // Don't walk into it though, it may not even be type checked yet.
+
+      // Don't walk into a body that has not yet been type checked. This should
+      // only occur for top-level code.
+      VarDecls.clear();
       return false;
     }
+
     if (auto *TLCD = dyn_cast<TopLevelCodeDecl>(D)) {
       // If this is a TopLevelCodeDecl, scan for global variables
       auto *body = TLCD->getBody();
@@ -2682,9 +2659,11 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
     if (auto param = dyn_cast<ParamDecl>(var)) {
       auto FD = dyn_cast<AccessorDecl>(param->getDeclContext());
       if (FD && FD->getAccessorKind() == AccessorKind::Set) {
-        auto getter = dyn_cast<VarDecl>(FD->getStorage());
-        if ((access & RK_Read) == 0 && AssociatedGetter == getter) {
-          if (auto DRE = AssociatedGetterRefExpr) {
+        auto VD = dyn_cast<VarDecl>(FD->getStorage());
+        if ((access & RK_Read) == 0) {
+          auto found = AssociatedGetterRefExpr.find(VD);
+          if (found != AssociatedGetterRefExpr.end()) {
+            auto *DRE = found->second;
             Diags.diagnose(DRE->getLoc(), diag::unused_setter_parameter,
                            var->getName());
             Diags.diagnose(DRE->getLoc(), diag::fixit_for_unused_setter_parameter,
@@ -3028,16 +3007,14 @@ std::pair<bool, Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
 
     // If the Expression is a read of a getter, track for diagnostics
     if (auto VD = dyn_cast<VarDecl>(DRE->getDecl())) {
-      if (AssociatedGetter == VD && AssociatedGetterRefExpr == nullptr)
-        AssociatedGetterRefExpr = DRE;
+      AssociatedGetterRefExpr.insert(std::make_pair(VD, DRE));
     }
   }
   // If the Expression is a member reference, see if it is a read of the getter
   // to track for diagnostics.
   if (auto *MRE = dyn_cast<MemberRefExpr>(E)) {
     if (auto VD = dyn_cast<VarDecl>(MRE->getMember().getDecl())) {
-      if (AssociatedGetter == VD && AssociatedGetterRefExpr == nullptr)
-        AssociatedGetterRefExpr = MRE;
+      AssociatedGetterRefExpr.insert(std::make_pair(VD, MRE));
       markBaseOfStorageUse(MRE->getBase(), MRE->getMember(), RK_Read);
       return { false, E };
     }
@@ -3124,18 +3101,22 @@ performTopLevelDeclDiagnostics(TopLevelCodeDecl *TLCD) {
 }
 
 /// Perform diagnostics for func/init/deinit declarations.
-void swift::performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD,
-                                               BraceStmt *body) {
-  assert(body && "Need a body to check");
-  
+void swift::performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD) {
   // Don't produce these diagnostics for implicitly generated code.
   if (AFD->getLoc().isInvalid() || AFD->isImplicit() || AFD->isInvalid())
     return;
 
   // Check for unused variables, as well as variables that are could be
-  // declared as constants.
-  body->walk(VarDeclUsageChecker(AFD));
-  
+  // declared as constants. Skip local functions though, since they will
+  // be checked as part of their parent function or TopLevelCodeDecl.
+  if (!AFD->getDeclContext()->isLocalContext()) {
+    auto &ctx = AFD->getDeclContext()->getASTContext();
+    VarDeclUsageChecker checker(ctx.Diags);
+    AFD->walk(checker);
+  }
+
+  auto *body = AFD->getBody();
+
   // If the function has an opaque return type, check the return expressions
   // to determine the underlying type.
   if (auto opaqueResultTy = AFD->getOpaqueResultTypeDecl()) {

lib/Sema/MiscDiagnostics.h

@@ -40,8 +40,7 @@ void performSyntacticExprDiagnostics(const Expr *E, const DeclContext *DC,
 /// Emit diagnostics for a given statement.
 void performStmtDiagnostics(ASTContext &ctx, const Stmt *S);
 
-void performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD,
-                                        BraceStmt *body);
+void performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD);
 
 /// Perform diagnostics on the top level code declaration.
 void performTopLevelDeclDiagnostics(TopLevelCodeDecl *TLCD);

lib/Sema/TypeCheckStmt.cpp

@@ -1972,12 +1972,13 @@ TypeCheckFunctionBodyUntilRequest::evaluate(Evaluator &evaluator,
     }
   }
 
+  // Wire up the function body now.
+  AFD->setBody(body, AbstractFunctionDecl::BodyKind::TypeChecked);
+
   // If nothing went wrong yet, perform extra checking.
   if (!hadError && endTypeCheckLoc.isInvalid())
-    performAbstractFuncDeclDiagnostics(AFD, body);
+    performAbstractFuncDeclDiagnostics(AFD);
 
-  // Wire up the function body now.
-  AFD->setBody(body, AbstractFunctionDecl::BodyKind::TypeChecked);
   return hadError;
 }
 

test/SILGen/c_function_pointers.swift

@@ -1,7 +1,7 @@
 // RUN: %target-swift-emit-silgen -verify %s | %FileCheck %s
 
 if true {
-  var x = 0
+  var x = 0 // expected-warning {{variable 'x' was never mutated; consider changing to 'let' constant}}
   func local() -> Int { return 0 }
   func localWithContext() -> Int { return x }
   func transitiveWithoutContext() -> Int { return local() }

test/SILGen/capture_order.swift

@@ -47,6 +47,7 @@ public func captureBeforeDefUnownedVar(obj: AnyObject) -> () -> AnyObject? {
   }
   let closure = getter
   unowned var unownedObj: AnyObject = obj // expected-note{{captured value declared here}}
+  // expected-warning@-1 {{variable 'unownedObj' was never mutated; consider changing to 'let' constant}}
   return closure
 }
 

test/SILGen/nested_types_referencing_nested_functions.swift

@@ -36,6 +36,7 @@ do {
 // Invalid case
 do {
   var x = 123 // expected-note {{captured value declared here}}
+  // expected-warning@-1 {{variable 'x' was never mutated; consider changing to 'let' constant}}
 
   func local() {
     // expected-error@-1 {{closure captures 'x' before it is declared}}

test/SILGen/statements.swift

@@ -525,6 +525,7 @@ func defer_in_closure_in_generic<T>(_ x: T) {
 // CHECK-LABEL: sil hidden [ossa] @$s10statements13defer_mutableyySiF
 func defer_mutable(_ x: Int) {
   var x = x
+  // expected-warning@-1 {{variable 'x' was never mutated; consider changing to 'let' constant}}
   // CHECK: [[BOX:%.*]] = alloc_box ${ var Int }
   // CHECK-NEXT: project_box [[BOX]]
   // CHECK-NOT: [[BOX]]

test/decl/var/properties.swift

@@ -930,7 +930,7 @@ class ObservingPropertiesNotMutableInWillSet {
   }
 
   func localCase() {
-    var localProperty: Int = 42 {
+    var localProperty: Int = 42 { // expected-warning {{variable 'localProperty' was written to, but never read}}
       willSet {
         localProperty = 19   // expected-warning {{attempting to store to property 'localProperty' within its own willSet}}
       }

test/decl/var/usage.swift

@@ -426,9 +426,10 @@ func testLocalFunc() {
   // expected-warning@-1 {{initialization of variable 'unusedVar' was never used; consider replacing with assignment to '_' or removing it}}
 
   var notMutatedVar = 0
-  // FIXME: All captures are read/write for the purposes of this analysis.
+  // expected-warning@-1 {{variable 'notMutatedVar' was never mutated; consider changing to 'let' constant}}
 
   var mutatedVar = 0
+  // expected-warning@-1 {{variable 'mutatedVar' was written to, but never read}}
 
   func localFunc() {
     _ = notMutatedVar

test/stmt/statements.swift

@@ -15,6 +15,7 @@ func invalid_semi() {
 
 func nested1(_ x: Int) {
   var y : Int
+  // expected-warning@-1 {{variable 'y' was never mutated; consider changing to 'let' constant}}
 
   func nested2(_ z: Int) -> Int {
     return x+y+z