Autofix StrictUnusedVariable

[ferozco]

Before this PR

StrictUnusedVariables suggested fix would delete unused parameters from public and private methods causing classes to incorrectly implement interfaces/extend abstract classes and fail compilation.

After this PR

==COMMIT_MSG==
StrictUnusedVariable now ignores variables prefixed with _ and the suggested fix will rename all unused parameters in public methods instead of removing them
==COMMIT_MSG==

This change should make it easier for teams to quickly adopt the new rule since most of the changes should be automated. Because of this, I'd consider raising the default severity to ERROR

Possible downsides?

N/A

[changelog-app]

Generate changelog in changelog/@unreleased

Type

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

Description

StrictUnusedVariable now ignores variables prefixed with _ and the suggested fix will rename all unused parameters in public methods instead of removing them

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

Thoughts on applying @SuppressWarnings("unused") instead of renaming the parameter? I think it's a bit clearer what we're suppressing in that case.

[ferozco]

I actually found it pretty annoying even having to add unused prefix when manually fixing up witchcraft since it can be longer than the actual variable name. I personally prefer _ since its so short, easy to add/remove and stands out in the code

[j-baker]

I'm not sure I understand this change.

1. The check shouldn't (in general) be dinging you for unused parameters that you received by implementing an interface. In general, this is not an antipattern, and the bug (if any) is on the side of the interface definer, rather than on the side of the interface implementer.
2. Therefore, the number of instances where this check actually needs to delete something should be tiny, and mostly by removing the argument.
3. And so I'm not sure why we'd want to introduce a new variable convention to support what is essentially an edge case?

Also, if you want to make this change you have to either

1. Have error prone be the thing that validates parameter names, in a context sensitive way.
2. Globally allow underscores in parameter names, since the current regex is (^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)

[j-baker]

aaand, it's probably worth also making it so that variable names that start with _ or unused or whatever can never actually be used

[dansanduleac]

1. The check shouldn't (in general) be dinging you for unused parameters that you received by implementing an interface. In general, this is not an antipattern, and the bug (if any) is on the side of the interface definer, rather than on the side of the interface implementer.

I feel like there are exceptions to this but mostly it boils down to the intent of each parameter.
If it's "something you might need" like a context, then it's fine to ignore, but if it's "a piece of data that needs to travel from A to B / that you're not allowed to drop" (e.g. an auth header) then that's a different story.

[iamdanfox]

The check shouldn't (in general) be dinging you for unused parameters that you received by implementing an interface. In general, this is not an antipattern, and the bug (if any) is on the side of the interface definer, rather than on the side of the interface implementer.

The main goal of this check is to catch people who defined a conjure API that has an AuthHeader, resulting a generated interface, e.g. doSomething(AuthHeader, ResourceIdentifier, Request) but then write an implementation that doesn't actually use the provided AuthHeader!

[dansanduleac]

I know this was already here, but I'm a bit worried about this behaviour.
If it's not going to auto-fix the call sites too (which is also dangerous), then it leaves the code in a state where it's harder for an IDE user to fix this (they'd have to hunt down the invocations).
On the other hand, if we don't autofix, they could just navigate to the method, and cmd+del the parameter, after which IntelliJ would autofix the callsites.
(I assume there's similar functionality in Eclipse though I haven't checked)

[j-baker]

@iamdanfox I get that, which is why I'm not sure that this check is really what we need here. I get that it's useful from an auth perspective - it's just not useful in many cases other than auth.

[iamdanfox]

@j-baker Rust has a similar dead_code lint baked in which guarantees you don't accidentally have floating unused params/methods. Given the choice, I'd want all my greenfield codebases to pass this check.

I'm aware that going from an unenforced -> enforced world requires taking an action for a lot of parameters (because errorprone can't tell whether they're intentionally or unintentionally unused), but I think this is a one-time cost that is worth paying because it exposes more bugs in addition to security issues.

For exmaple, @robert3005 actually spent the time to make his repo pass this check and found a legit bug where he forgot to use the statusCode param:

    @Override
    public void status(int statusCode, long inputDatasetSize) {
+       if (statusCode / 100 == 5) {
            registry.meter(MetricName.builder()
                    .safeName("preview.server.response.error")
                    .putSafeTags("inputDatasetSize", Long.toString(inputDatasetSize))
                    .build())
                    .mark();
+       }
    }

[iamdanfox]

I'm in favour of this change. Whether we do the AuthHeader subset or not, I think the autofix improvement is necessary because we definitely don't want to recommend deleting args when they're just diligently implementing an interface!

I like the underscore thing because it matches Rust's pattern for declaring unused variables and reduces the noise of adding an unused prefix or the @SuppressWarnings("unused").

[j-baker]

Dan, I think we might be misunderstanding each other.

To me, it's a little curious that we:

1. Would find that this catches enough bugs per change that it is more ham than spam
2. and simultaneously triggers frequently enough that we'd change our parameter naming rules in order to allow a reduced overhead version.

I also have some implementation notes (we need to change checkstyle to allow variable names that start with underscore, and also need to ban actually using variables that start with _ or else this degrades), but those should be uncontentious.

Please don't think I'm arguing against the existence of a check like this! I'm just surprised by this change.

[j-baker]

Figured given this is approved, I need a disapproval to block an automerge. This breaks the repo, because it will refactor the code into a place where checkstyle fails.

[iamdanfox]

Ok so tasks remaining to move forwards:

• amend checkstyle regex to allow a leading underscore on parameter names
• add some logic to make sure that normal variables don't start with underscores (we want to be an exact indicator of unusedness, not something that people start littering their codebase with).

[ferozco]

I've updated the PR so that checkstyle will allow parameter names with a leading _ and updated the check so that it will fail if you have used variables that start with _ or unused. It will also go ahead and rename variables so that the check is not inadvertently suppressed.

I'd also like to make it so that _ is the only prefix which disables the rule. This will cause a bit of pain for repos that have already adopted the change, but I think its worth it given that there are only a few early adopters and that we can automate the correction process

[iamdanfox]

Looks great. I think it's fine to take aways the 'unused' prefix in a separate PR.

@j-baker I think Felipe has addressed your concerns - any other blockers?

Confirmed in person blockers are addressed

[iamdanfox]

So the remaining question is whether we should run this as an RC or pTML on a few repos just to sanity check the diff that it produces.

i.e. should it be disabled in test code? should it be disabled when method bodies are empty etc

[ferozco]

Happy to give the feature a spin on a few repos internally.

should it be disabled in test code?

I don't think so. Given the changes we've made, getting a source set to pass the check should be very easy. It can also be hard to correctly identify test code from non-test code and so we would get inconsistent behaviour at best.

should it be disabled when method bodies are empty

I'd prefer to avoid adding special cases to the rule since It will make it harder to maintain compatibility with upstream. Also similar to above, the cost of not special casing the code should be incredibly low because of the fixes we've added.

[iamdanfox]

To see what this looks like, I've pushed commits to the following internal repos:

• https://internal-github/foundry/sls-logging/pull/904

[ferozco]

Heres an example of it running on conjure-java-runtime: https://github.com/palantir/conjure-java-runtime/compare/fo/unused-variables?expand=1

[pkoenig10]

This check causes compile failures if generated code has variables that start with _ (derive4j does this) and -Werror is used.