Ignore disabled checks passed to -XepPatchChecks

[oxkitsune]

Rather than throwing an NoSuchElementException.

Fixes #3908.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Stephan202]

@oxkitsune these PRs are generally merged with the PR text as commit message, so I would also add this sentence to the PR body (just like in the commit message):

Rather than throwing an NoSuchElementException.

[rickie]

@oxkitsune these PRs are generally merged with the PR text as commit message, so I would also add this sentence to the PR body (just like in the commit message):

That's already the case right? It should be good like it is right now.

[Stephan202]

I would swear that my browser didn't show it. But I must have been confused. 🤔

[cushon]

Thanks for this fix!

I finally did some internal testing of this, and one thing I encountered is that this changes of behaviour of things like -Xep:DeadException:WARN -XepPatchChecks:GetClassOnClass--previously refactorings weren't emitted for DeadException even though it was explicitly enabled, because it wasn't listed in -XepPatchChecks. With this change refactorings are emitted for DeadException.

Do you have thoughts on what the least surprising behaviour here would be?

[cushon]

Un-approving until the question in #4028 (comment) is resolved

[oxkitsune]

Do you have thoughts on what the least surprising behaviour here would be?

Personally I think it would make the most sense, if checks passed to -Xep:PatchChecks are the only checks emitting patches, unless they're explicitly disabled.

E.g.

-Xep:DeadException:WARN -XepPatchChecks:GetClassOnClass

would emit patches for GetClassOnClass, and warnings for DeadException.

-Xep:DeadException:OFF -XepPatchChecks:GetClassOnClass,DeadException

would emit just patches for GetClassOnClass

[cushon]

Do you have thoughts on what the least surprising behaviour here would be?

Personally I think it would make the most sense, if checks passed to -Xep:PatchChecks are the only checks emitting patches, unless they're explicitly disabled.

E.g.

-Xep:DeadException:WARN -XepPatchChecks:GetClassOnClass

would emit patches for GetClassOnClass, and warnings for DeadException.

-Xep:DeadException:OFF -XepPatchChecks:GetClassOnClass,DeadException

would emit just patches for GetClassOnClass

That sounds good to me.

I think that there isn't an order that applyOverrides and filter can be called in that does quite what we want here: calling applyOverrides first causes #3908, and calling filter first causes refactorings to be emitted for checks that are explicitly enabled with -Xep: flags.

I wonder if it makes sense of -XepPatchChecks to just override any -Xep:...:[OFF|WARN|ERROR] flags. That would mean that we went back to the pre-2.19 behaviour where refactorings were emitted for -Xep:CheckName:OFF -XepPatchChecks:CheckName. Would that work for you?

[Stephan202]

I wonder if it makes sense of -XepPatchChecks to just override any -Xep:...:[OFF|WARN|ERROR] flags. That would mean that we went back to the pre-2.19 behaviour where refactorings were emitted for -Xep:CheckName:OFF -XepPatchChecks:CheckName. Would that work for you?

I'll need to find some time later to think this through more thoroughly, but I wonder whether we'll regret that if #947 is ever merged (still hoping 😉): then -XepPatchChecks: (without further args) would enable patching for all checks, and realistically users will want to apply some exclusions.

That said, we have the #947 change in our fork, and right now we do have this "patch all except those that are OFF" behavior, with a baseline more recent than 2.19.

Easiest is perhaps to first add a few more tests, and then assess impact on #947.

[cushon]

I'll need to find some time later to think this through more thoroughly

My too, I think :)

The existing logic only calls filter if !namedCheckers.isEmpty(), if we keep that part and do the rest of what I described, I think that would interact well with #947

i.e. something like:

                  ScannerSupplier toUse = ErrorPronePlugins.loadPlugins(scannerSupplier, context);
                  ImmutableSet<String> namedCheckers = epOptions.patchingOptions().namedCheckers();
                  if (!namedCheckers.isEmpty()) {
                    toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
                  } else {
                    toUse = toUse.applyOverrides(epOptions);
                  }
                  return ErrorProneScannerTransformer.create(toUse.get());

[Stephan202]

Rebased this branch to include the changes of #947 (diff applied cleanly; thanks for merging @cushon!), which should make it marginally easier to reason about the solution space. (Actual reasoning still TBD 😅)

[oxkitsune]

Hey @cushon apologies for the inactivity 😅
I've just applied the suggestions and I'm happy with this solution!

[cushon]

Hey @cushon apologies for the inactivity 😅 I've just applied the suggestions and I'm happy with this solution!

No worries, thanks for coming back to this!