Request: better docs for InjectOnBugCheckers

[msridhar]

I just got a warning from the new InjectOnBugCheckers checker when updating NullAway to EP 2.18.0. But, I'm not sure why the missing @Inject annotation on the bug checker constructor is a problem. Could the docs be updated to add a bit more explanation? I don't want to have NullAway take another dependence to get the javax.inject.Inject annotation, so we'd probably only fix this if it's serious.

[Stephan202]

As of this release the error_prone_core artifact pulls in javax.inject, so IIUC i.c.w. Error Prone 2.18.0 the dependency could be declared provided. (I think that'd also work with older versions of Error Prone, because Java doesn't require referenced annotations to be on the classpath.)

I suspect that the new InjectOnBugCheckers check is a precursor to #3701. 🤔

However, if one adds @Inject right now, then UnnecessarilyVisible complains about the constructor being public, while ScannerSupplierImpl#instantiateChecker(BugCheckerInfo) only considers public constructors: a catch-22.

[cushon]

cc @graememorgan

The context is that we are investigating using guice to instantiate bugcheckers, instead of relying on what's effectively a home-grown DI implementation: #3701

[lazaroclapp]

Would #3701 basically need us to provide different versions of the core NullAway class/jar before and after a certain EP version? As of right now, we support running NullAway with any EP version between 2.4.0 and 2.18.0 (and test at least the extremes in our CI). Moving the minimum supported EP version is certainly doable, but there might be a few moving pieces with that. In general, there are codebases we want to support that might not always be on the latest EP release, so I wonder what's the path for developing a third-party EP checker that works on pre- and post-Guice EP releases?

[cushon]

@lazaroclapp I'd like to avoid having this be an abrupt breaking change, so if there are ways to make this less invasive / easier to absorb, I'm open to suggestions.

I think this logic ensures that the @Inject is optional for now, and if necessary we use the old logic to instantiate the check:

error-prone/check_api/src/main/java/com/google/errorprone/scanner/ScannerSupplierImpl.java

Lines 75 to 78 in d8a0abc

	} catch (ProvisionException | com.google.inject.ConfigurationException e) {
	// Fall back to the old path for external checks.
	// TODO(b/263227221): Consider stripping this internally after careful testing.
	return instantiateCheckerOldPath(checker);

Also, I think adding the @Inject annotations would be backwards compatible, in that older versions of Error Prone would just ignore them and still be able to instantiate the checks?

[lazaroclapp]

Ok, if I follow that correctly, then I think the above seems quite sufficient for us to maintain compatibility with multiple EP versions even once #3701 lands. Thanks!

Though, if I understand this correctly:

However, if one adds @Inject right now, then UnnecessarilyVisible complains about the constructor being public, while ScannerSupplierImpl#instantiateChecker(BugCheckerInfo) only considers public constructors: a catch-22.

Then I assume for a while we would need to suppress UnnecessarilyVisible and keep the constructor both public and marked @Inject. But that's very doable.

[cushon]

However, if one adds @Inject right now, then UnnecessarilyVisible complains about the constructor being public, while ScannerSupplierImpl#instantiateChecker(BugCheckerInfo) only considers public constructors: a catch-22.

Then I assume for a while we would need to suppress UnnecessarilyVisible and keep the constructor both public and marked @Inject. But that's very doable.

I think maybe we should just teach UnnecessarilyVisible to not report those diagnostics for BugChecker class for now, I'll take a look at that