Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-11992] Check variable usage within closures #34617

Closed
wants to merge 1 commit into from

Conversation

maustinstar
Copy link
Contributor

@maustinstar maustinstar commented Nov 6, 2020

Summary

Variables that are never used or mutated are not diagnosed within closure bodies that are not at the top level. This is because closure bodies were never checked with VarDeclUsageChecker unless they were at the top level. This pull request adds swift::performClosureBodyDiagnostics which performs the same variable usage check as top level declarations and AFDs.

Examples:

Before this PR:

class Foo {
    lazy var x: Int = {
        var y = 42 // no warning
        var z = 42 // no warning
        return z
    }()
}

After this PR:

class Foo {
    lazy var x: Int = {
        var y = 42 // warning: initialization of variable 'y' was never used; consider replacing with assignment to '_' or removing it
        var z = 42 // warning: variable 'z' was never mutated; consider changing to 'let' constant
        return z
    }()
}

Links

Resolves SR-11992 and SR-13821

Tagging @rintaro.

/// Perform diagnostics on closure body.
void swift::performClosureBodyDiagnostics(ClosureExpr *closure) {
if (auto *ACE = dyn_cast<AbstractClosureExpr>(closure)) {
// Skip if we aleady checked variable usage.
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead change those walkers to skip closure bodies, so that we can check them unconditionally here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The walkers can't skip the closure bodies entirely because they look for uses of a variable within the closure. But, I could try to skip the tracking of any variables declared within the closures (I'm not sure yet how trivial this is, but I will look into it). Then later, walk through all closures and track variable declarations unconditionally.

Thank you for the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

You can’t skip them in the walker itself, but you can skip them when you first enter the walker. E.g. instead of walking the entire Top Level Code Decl, you can iterate over its members and skip closures knowing they’ll be checked later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants