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

Don't count identifier expressions as variable usages in StrictUnusedVariable check #2304

Closed

Conversation

IlanaRadinsky
Copy link

@IlanaRadinsky IlanaRadinsky commented Jun 16, 2022

Before this PR

If you have a class like this

public class Test {
    private final String myField;

    public Test(String myField) {
        this.myField = myField;
    }
}

then the StrictUsedVariable check fails with

error: [StrictUnusedVariable] The field 'myField' is never read. Intentional occurrences are acknowledged by renaming the unused variable with a leading underscore. '_myField', for example.
    private final String myField;
                         ^

But if I apply the suggested fix

public class Test {
    private final String _myField;

    public Test(String myField) {
        this._myField = myField;
    }
}

I also get a StrictUnusedVariable error

error: [StrictUnusedVariable] The field '_myField' is read but has 'StrictUnusedVariable' suppressed because of its name.
    private final String _myField;
                         ^

See a1b70a5 https://app.circleci.com/pipelines/github/palantir/gradle-baseline/4165/workflows/756c72ee-8297-4da5-837a-40a3b010ad09/jobs/34598 for example of failing test before change.

After this PR

==COMMIT_MSG==
Don't count identifier expressions as variable usages in StrictUnusedVariable check
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Jun 16, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Don't count identifier expressions as variable usages in StrictUnusedVariable check

Check the box to generate changelog(s)

  • Generate changelog entry

@carterkozak
Copy link
Contributor

Hmm, in this case I think we'd prefer to suggest removing the variable and assignment entirely rather than allowing underscores. That can be a bit tricky to auto-suggest, though.

@IlanaRadinsky
Copy link
Author

@carterkozak Would it be preferable to update the error message without the auto-suggest, or just to close this PR?

@carterkozak
Copy link
Contributor

Yep, ideally we would fail due to unused fields, but we wouldn't suggest renaming anything.

@IlanaRadinsky
Copy link
Author

Tracking in #2361 since I haven't had a chance to come back to this yet.

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.

3 participants