Allow local variable names that start with underscore

[ash211]

Before this PR

For code with an unused local variable name like this:

// fire and forget
Future<?> ignored = submitFuture();

There's an error prone rule StrictUnusedVariable that fires with a message like

 error: [StrictUnusedVariable] The local variable 'ignored' is never read. Intentional occurrences are acknowledged by renaming the unused variable with a leading underscore. '_ignored', for example.
              Future<?> ignored = submitFuture();
                        ^
      (see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)

But then if you take the suggestion and name the variable according to its suggestion like this:

// fire and forget
Future<?> _ignored = submitFuture();

Then now checkstyle fails with an error like:

MyFile.java:296: error: Local variable name '_ignored' must match pattern '^[a-z][a-zA-Z0-9]+$'.

It is problematic that the baseline error prone suggests a format that then the baseline checkstyle configuration rejects.

After this PR

==COMMIT_MSG==
Allow local variable names that start with underscore
==COMMIT_MSG==

With this PR, now the recommendation from StrictUnusedVariable can actually be taken.

The regex is similar to the existing regex ~15 lines below for ParameterName

Alternative: no variable assignment

Instead of make this allowed in checkstyle, an alternative could be to stop recommending it in error-prone. Now users should resolve the original error by removing the variable assignment entirely, like this:

// fire and forget
submitFuture();

However, this interacts poorly with the FutureReturnValueIgnored bug pattern, which would now fail with this message:

 warning: [FutureReturnValueIgnored] Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future.
              submitFuture();
                                                             ^
      (see https://errorprone.info/bugpattern/FutureReturnValueIgnored)
    Did you mean 'var unused = submitFuture();' or to remove this line?

and is recommending to create an unused variable assignment again!

For users truly trying to fire and forget a future, they would have to suppress the error-prone at the surrounding method. Unfortunately this is a very wide suppression and can suppress over hundreds of lines when the problem is only in one. Like this:

@SuppressWarnings("FutureReturnValueIgnored")
void myFunction() {
  //
  // ..snip hundreds of lines
  //

  // fire and forget
  submitFuture();

  //
  // ..snip hundreds of lines
  //
}

Because of this bad interaction with FutureReturnValueIgnored, I think it's better to move towards an explicit assignment to an unused variable that starts with an underscore.

In the end, making this the desirable and allowed phrasing:

// fire and forget
Future<?> _ignored = submitFuture();

and allowing the _ prefix for variable names in checkstyle (what this PR does)

[changelog-app]

Generate changelog in changelog/@unreleased

Type (Select exactly one)

• Feature (Adding new functionality)
• Improvement (Improving existing functionality)
• Fix (Fixing an issue with existing functionality)
• Break (Creating a new major version by breaking public APIs)
• Deprecation (Removing functionality in a non-breaking way)
• Migration (Automatically moving data/functionality to a new system)

Description

Allow local variable names that start with underscore

Check the box to generate changelog(s)

• Generate changelog entry

[changelog-app]

✅ Successfully generated changelog entry!

Need to regenerate?

Simply interact with the changelog bot comment again to regenerate these entries.

---

📋Changelog Preview

💡 Improvements

• Allow local variable names that start with underscore (#3422)

[pkoenig10]

👎 from me.

There's no reason to declare a variable that is unused. If the goal is to silence the FutureReturnValueIgnored check, then we should do that explicitly with @SuppressWarnings instead of declaring an unused variable so the intention is clear to readers.

I don't really agree with the FutureReturnValueIgnored suggested fix. I assume this suggestion fix exists for other reasons - because they want to be able to automate fixes and it's non-trivial to find a place to put @SuppressWarnings annotation (plus it's a bit weird if the suggested fix for a check is to just suppress the check).