New check request: Unreachable methods in anonymous classes

[kevinb9n]

Any non-override, non-private method in an anonymous class is at least "dodgy" and might fool a dead code detector.

Non-override methods are hard to reach from outside the class body itself. There are two kinds of anonymous class:

• In a "class instance creation expression with class body", a non-override method is only reachable one of two ways:
  • calling it directly with the anonymous class expression itself as the receiver (new Object() { void foo() {} }.foo())
  • if the anonymous class expression is assigned to a local variable that uses var instead of an explicit type
• In an "enum constant with class body", a non-override method is not usable from outside that body at all.

If such a method is marked public, that might prevent a dead-code detector from recognizing it can be cleaned up. It would probably be a good practice to always make it private. Unfortunately, those two weird cases for non-enum anonymous classes should probably be exempted.

No idea if this is worth investigating further, just realized this as another strange wart in the language.

[tbroyer]

I wouldn't oppose a warning to make all non-override methods in an anonymous class private, even without detecting this edge case (those using it for a good reason could still suppress the warning, or disable the check; but they could also refactor it to a non-anonymous class)

[cushon]

I wouldn't oppose a warning to make all non-override methods in an anonymous class private, even without detecting this edge case

To double-check, making it private is safe even with the edge case, right? Private visibility would still allow access within the same scope where the anonymous class is declared, which is the only way to call the method directly on the new class expression or on the var-typed variable.

I was wondering about checking if there are any uses of the method in the current compilation unit, and making it private if it is used, and deleting it if it is not. I don't think there's any way these methods can be accessed across compilation boundaries.

[cpovirk]

Given that anonymous-class members can't be used outside the compilation unit, would it be good enough for Error Prone to suggest removing them? (I think it probably already does?)

At that point, adding private feels a little bit like noise, though I do know that some people are a fan of being explicit.

I do think that removing other modifiers (public, protected, final (maybe already covered by the Google-internal RedundancyRemover?)) could make some sense. RedundancyRemover is also something that we haven't enabled by default even internally, but we could look into at least releasing it, even if it remains off by default.

[kevinb9n]

Ahh right, I always forget what private actually means :-)
Indeed, the edge cases don't pose an issue.
It seems that every non-override method in an anonymous class can safely be private.
(That said, maybe users would rather not bother having to write private. I guess the main things clearly worth warning about are writing public or protected...)

The other half I overlooked is that every override method can safely have the minimal visibility forced by its supermethods.

Removal could be a remedy, but in other ways it makes sense to keep dead-code-removal activities sort of distinct from fix-up-the-code-you-have activities. I dunno.

[cushon]

Many of the results I've found so far are things that are used reflectively. I added handling for @Keep (and annotations that are meta-annotated with @Keep). There are some more false positives that are using other annotations, or that are just directly accessed reflectively.

I think it's still worthwhile as a warning, it can be suppressed for those deliberate cases using reflection.

[cushon]

It turns out that https://errorprone.info/bugpattern/UnusedMethod already handles these and will suggest deleting them if they're actually unused.

I am still looking at doing something about the modifiers, though.