Generate unused variable warnings in top level statements #7023

KingOfBrian · 2017-01-25T05:07:55Z

This PR walks the VarDeclUsageChecker on SourceFile.Decls to generate warnings for statements that aren't contained in abstract function declarations. This also disables the unused variable warnings for global variables since tracking global variables is complicated and felt like it was outside the scope of this bug.

Resolves SR-2115.
I force pushed over my first attempt at this (#6971) to clean up history. I forgot it would kill the PR.

slavapestov · 2017-01-25T08:11:28Z

@swift-ci Please test

slavapestov · 2017-01-25T08:11:36Z

Nice!

swift-ci · 2017-01-25T08:50:20Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - edc193e
Test requested by - @slavapestov

jrose-apple · 2017-01-25T18:28:07Z

lib/Sema/TypeChecker.cpp

@@ -731,6 +732,7 @@ void swift::performWholeModuleTypeChecking(SourceFile &SF) {
  Ctx.diagnoseObjCMethodConflicts(SF);
  Ctx.diagnoseObjCUnsatisfiedOptReqConflicts(SF);
  Ctx.diagnoseUnintendedObjCMethodOverrides(SF);
+  performTopLevelDeclDiagnostics(SF);


This section is only for diagnostics that apply across files. performTopLevelDeclDiagnostics is something we can just do in the regular performTypeChecking, and ideally only on the newly-added decls.

Good catch! I actually only put it there because I was originally exploring cross-file global variable checking, which turned out to be more complicated. I logged SR-3721 to track that. I'll move this to a better place.

I moved the invocation but I'm not sure I followed the ideally only on the newly-added decls comment.

jrose-apple · 2017-01-25T21:29:04Z

lib/Sema/TypeChecker.cpp

@@ -691,6 +692,7 @@ void swift::performTypeChecking(SourceFile &SF, TopLevelContext &TLC,
      TC.processREPLTopLevel(SF, TLC, StartElem);

    typeCheckFunctionsAndExternalDecls(TC);
+    performTopLevelDeclDiagnostics(SF);


…and now that I look at it a second time, does it make sense to sink it into typeCheckTopLevelCodeDecl? That way we don't have to walk things twice.

Nice! This also un-covered a lot of masked warnings since I shared the VarDeclUsageChecker across multiple statements. This produced a bunch of new UT failures that I fixed and also un-covered a bug when guard adds a variable to the global scope.

KingOfBrian · 2017-01-26T15:08:30Z

@swift-ci Please test

jrose-apple

Ah, I see the problem. We would need the same VarDeclUsageChecker to walk all of the TLCDs in order to track usage of local variables. Okay, sorry for the bad advice; I think it was better as a full pass where you can catch guard let as well.

jrose-apple · 2017-01-26T16:37:47Z

lib/Sema/MiscDiagnostics.cpp

-    D->walk(checker);
-  }
+void swift::
+performTopLevelDeclDiagnostics(TypeChecker &TC, TopLevelCodeDecl &TLCD) {


For whatever reason, we don't really pass AST nodes as references, just pointers. Please use that convention for now.

jrose-apple · 2017-01-26T16:38:08Z

lib/Sema/TypeCheckStmt.cpp

@@ -1585,4 +1585,6 @@ void TypeChecker::typeCheckTopLevelCodeDecl(TopLevelCodeDecl *TLCD) {
  StmtChecker(*this, TLCD).typeCheckStmt(Body);
  TLCD->setBody(Body);
  checkTopLevelErrorHandling(TLCD);
+  if (TLCD) 


Since TLCD is dereferenced two lines above, it should be okay to drop this check.

KingOfBrian · 2017-01-26T23:11:44Z

Nope, the advice was good, and good to know about the conventions. I logged SR-3721 for unused global variable warnings since I think that would still be a valuable feature, especially for scripting.

jrose-apple · 2017-01-26T23:22:20Z

Yeah, unused (and only-written) variables are good things to catch.

Do you disagree with my comment about guard let, for this patch?

KingOfBrian · 2017-01-27T02:07:38Z

I don't think we can do a better job with guard statements under a TLCD. Since they introduce global variables, and global variables can't be effectively tracked without more work. I'm interested expanding the checker to work better as scopes change, but would need some help making sure I have a good strategy, because the walking strategy would need to be changed substantially. I'll add some more notes on issues I've seen in the checker to SR-3721.

rintaro · 2017-01-27T02:30:45Z

lib/Sema/MiscDiagnostics.cpp

+            });
+          }
+        }
+        else if (node.is<Stmt *>()) {


nitpick: } else if . (no new-line after })

slavapestov · 2017-01-27T02:53:25Z

@KingOfBrian The new ASTScope stuff being worked on by @DougGregor might help with modeling bindings introduced by guard statements...

jrose-apple · 2017-01-27T03:15:06Z

Ah, guard statements do not introduce global variables. You can see this in the AST dump:

let x = 5
guard let y = x as Optional else { fatalError() }

(source_file
  (top_level_code_decl
    (brace_stmt
      (pattern_binding_decl
        (pattern_named type='Int' 'x')
        (call_expr implicit type='Int' location=<stdin>:1:9 range=[<stdin>:1:9 - line:1:9] nothrow  arg_labels=_builtinIntegerLiteral:
          (constructor_ref_call_expr implicit type='(Int2048) -> Int' location=<stdin>:1:9 range=[<stdin>:1:9 - line:1:9] nothrow
            (declref_expr implicit type='(Int.Type) -> (Int2048) -> Int' location=<stdin>:1:9 range=[<stdin>:1:9 - line:1:9] decl=Swift.(file).Int.init(_builtinIntegerLiteral:) function_ref=single specialized=no)
            (type_expr implicit type='Int.Type' location=<stdin>:1:9 range=[<stdin>:1:9 - line:1:9] typerepr='Int'))
          (tuple_expr implicit type='(_builtinIntegerLiteral: Int2048)' location=<stdin>:1:9 range=[<stdin>:1:9 - line:1:9] names=_builtinIntegerLiteral
            (integer_literal_expr type='Int2048' location=<stdin>:1:9 range=[<stdin>:1:9 - line:1:9] value=5))))
))
  (var_decl "x" type='Int' access=internal let storage_kind=stored)
  (top_level_code_decl
    (brace_stmt
      (guard_stmt
        (pattern
          (optional_some_element implicit type='Optional<Int>'
            (pattern_let implicit type='Int'
              (pattern_named type='Int' 'y')))
          (coerce_expr type='Optional<Int>' location=<stdin>:2:17 range=[<stdin>:2:15 - line:2:20] writtenType='Optional<Int>'
            (inject_into_optional implicit type='Optional<Int>' location=<stdin>:2:15 range=[<stdin>:2:15 - line:2:15]
              (declref_expr type='Int' location=<stdin>:2:15 range=[<stdin>:2:15 - line:2:15] decl=main.(file).x@<stdin>:1:5 direct_to_storage function_ref=unapplied specialized=no))))
        (brace_stmt
          (call_expr type='Never' location=<stdin>:2:36 range=[<stdin>:2:36 - line:2:47] nothrow  arg_labels=
            (declref_expr type='(@autoclosure () -> String, StaticString, UInt) -> Never' location=<stdin>:2:36 range=[<stdin>:2:36 - line:2:36] decl=Swift.(file).fatalError(_:file:line:) function_ref=single specialized=no)
            (tuple_shuffle_expr implicit type='(@autoclosure () -> String, file: StaticString, line: UInt)' location=<stdin>:2:46 range=[<stdin>:2:46 - line:2:47] elements=[-1, -3, -3] variadic_sources=[]
              (tuple_expr type='()' location=<stdin>:2:46 range=[<stdin>:2:46 - line:2:47]))))))))

Note the VarDecl for x is outside the TLCDs, but there isn't one for y.

@slavapestov: I don't think we can count on getting ASTScope any time soon.

jrose-apple · 2017-01-27T03:15:25Z

P.S. swiftc -ast-dump :-)

rintaro · 2017-01-27T03:26:51Z

It used to (8e14bba), but it's disabled by you in 4cdac3f :)

jrose-apple · 2017-01-27T03:28:42Z

Deliberately so. It never makes sense to access a guard let variable from another file because it might not be there!

KingOfBrian · 2017-01-27T04:06:57Z

Oh neat, I actually didn't realize file scope was possible without access control modifiers. Still the VarDeclUsageChecker doesn't work well scoping variables through TypeDecl's or AbstractFuncDecls. Currently an inner function squashes the checker since the func decl is not type checked when the checker runs. ie:

func outerFunction() {
    var noWarning: String? = "due to innerFunction not being typechecked"
    func innerFunction() { // This AbstractFuncDecl removes any variables in the checker
        var warningGenerated: String? = ""
    }
    var warningGenerated: String? = ""
}

It also doesn't traverse types at all so the following also won't be detected:

let global: String? = ""
guard let file = global else { fatalError() }

class Foo { // TypeDecl's are not descended
    let variable = file // This usage won't be flagged
}

The above isn't an issue currently, since the checker never runs on variables that a type can close over.

So I'm not sure how much better this can get without a new abstraction, and it sounds like one is planned. If you think this fix should get punted till that abstraction is in place, that's cool by me. I learned a lot about how the driver worked and got somewhat comfortable with my first ASTWalker!

jrose-apple · 2017-01-27T04:11:08Z

Heh, that latter code snippet actually crashes if you get to SILGen. Arguably you shouldn't be allowed to do that; as before, the class might be referenced from another file. You've convinced me that this is a reasonable limitation for now.

jrose-apple · 2017-01-27T04:11:19Z

@swift-ci Please smoke test

KingOfBrian · 2017-01-27T04:13:14Z

Also, to be less defeatist about this, we could invoke this checker a bit later in the type check phase. If the tree was fully typechecked I think we could check function usages better and add support for the class traversal -- but the former may be expensive because it may require another walk, and the later may actually not be a desired feature to support.

jrose-apple · 2017-01-27T04:19:27Z

I'm happy to come back and improve this later. Right now this'll at least catch a few more cases than we did before.

KingOfBrian · 2017-01-27T20:39:18Z

@swift-ci Please smoke test

slavapestov · 2017-01-30T02:06:28Z

@swift-ci Please smoke test

slavapestov · 2017-01-30T02:06:47Z

@swift-ci Please smoke test

Brian King added 2 commits January 24, 2017 22:50

Run the VarDeclUsageChecker on top level declarations

6091cda

Fix warnings in top level statements

edc193e

jrose-apple reviewed Jan 25, 2017

View reviewed changes

Move invocation of the top level diagnostics

5d6bd40

jrose-apple reviewed Jan 25, 2017

View reviewed changes

Brian King added 3 commits January 26, 2017 09:59

Align TLCD checking better with existing approach

36e36a1

Flag global variables declared in guard constructs

d835167

Fix unit tests

0c57aeb

jrose-apple suggested changes Jan 26, 2017

View reviewed changes

Code review feedback

b389fc3

rintaro reviewed Jan 27, 2017

View reviewed changes

lib/Sema/MiscDiagnostics.cpp

});

}

}

else if (node.is<Stmt *>()) {

Copy link

Member

rintaro Jan 27, 2017 •

edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: } else if . (no new-line after })

Make sure braces are consistently cuddled

3c4fec8

slavapestov self-assigned this Jan 27, 2017

jrose-apple approved these changes Jan 27, 2017

View reviewed changes

Fix a crash with a nil pattern returned from StmtConditionElement

e9e2378

Fix a test failure

c231b8c

slavapestov merged commit 71cf245 into apple:master Jan 30, 2017

KingOfBrian deleted the bugfix/SR-2115 branch January 30, 2017 04:35

swift-ci mentioned this pull request Jan 25, 2017

[SR-2115] unused variable unreported by compiler #44723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate unused variable warnings in top level statements #7023

Generate unused variable warnings in top level statements #7023

KingOfBrian commented Jan 25, 2017

slavapestov commented Jan 25, 2017

slavapestov commented Jan 25, 2017

swift-ci commented Jan 25, 2017

jrose-apple Jan 25, 2017

KingOfBrian Jan 25, 2017

KingOfBrian Jan 25, 2017

jrose-apple Jan 25, 2017

KingOfBrian Jan 26, 2017

KingOfBrian commented Jan 26, 2017

jrose-apple left a comment

jrose-apple Jan 26, 2017

jrose-apple Jan 26, 2017

KingOfBrian commented Jan 26, 2017

jrose-apple commented Jan 26, 2017

KingOfBrian commented Jan 27, 2017

rintaro Jan 27, 2017 •

edited

slavapestov commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

rintaro commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

KingOfBrian commented Jan 27, 2017 •

edited

jrose-apple commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

KingOfBrian commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

KingOfBrian commented Jan 27, 2017

slavapestov commented Jan 30, 2017

slavapestov commented Jan 30, 2017

Generate unused variable warnings in top level statements #7023

Generate unused variable warnings in top level statements #7023

Conversation

KingOfBrian commented Jan 25, 2017

slavapestov commented Jan 25, 2017

slavapestov commented Jan 25, 2017

swift-ci commented Jan 25, 2017

jrose-apple Jan 25, 2017

Choose a reason for hiding this comment

KingOfBrian Jan 25, 2017

Choose a reason for hiding this comment

KingOfBrian Jan 25, 2017

Choose a reason for hiding this comment

jrose-apple Jan 25, 2017

Choose a reason for hiding this comment

KingOfBrian Jan 26, 2017

Choose a reason for hiding this comment

KingOfBrian commented Jan 26, 2017

jrose-apple left a comment

Choose a reason for hiding this comment

jrose-apple Jan 26, 2017

Choose a reason for hiding this comment

jrose-apple Jan 26, 2017

Choose a reason for hiding this comment

KingOfBrian commented Jan 26, 2017

jrose-apple commented Jan 26, 2017

KingOfBrian commented Jan 27, 2017

rintaro Jan 27, 2017 • edited

Choose a reason for hiding this comment

slavapestov commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

rintaro commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

KingOfBrian commented Jan 27, 2017 • edited

jrose-apple commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

KingOfBrian commented Jan 27, 2017

jrose-apple commented Jan 27, 2017

KingOfBrian commented Jan 27, 2017

slavapestov commented Jan 30, 2017

slavapestov commented Jan 30, 2017

rintaro Jan 27, 2017 •

edited

KingOfBrian commented Jan 27, 2017 •

edited