Checker reporting an ERROR affects EFFECTIVELY_FINAL symbol flag in subsequent source files

[XN137]

I was upgrading error-prone to 2.31.0 on a branch for a project and it started reporting both DefaultCharset and FormatStringAnnotation violations in a module.
The former were expected and valid but the latter left me scratching my head as the FormatStringAnnotation checker was not modified between the versions afaict.

I tried reproducing the FormatStringAnnotation violations in an error-prone unit test but the code was passing the check just fine.

Then I tried to reproduce the whole scenario in an error-prone integration test and was successful, so I have pushed the test code here:
XN137#1

note:

• it looks like after an ERROR has been reported by UnusedVariable, the symbol flags are different in subsequent files and the EFFECTIVELY_FINAL flag is no longer set and so FormatStringAnnotation is reporting "phantom violations"
• the symbol flags stay the same when UnusedVariable is set to WARN
• the symbol flags stay the same when the file comes before the file that contains the UnusedVariable ERROR violation
• in the IT I have replaced DefaultCharset with UnusedVariable to show its not related to DefaultLocale / String.format having any shared state with FormatStringAnnotation

please take a look and let me know whether this is expected behavior in errorprone (or javac) or a potential bug?
If the former, maybe this should be documented somewhere as it can be really confusing if checker B starts reporting "unjustified" violations just because checker A has already reported an ERROR.
sorry in case i missed something obvious that would explain this behavior.

[XN137]

@cushon sorry for the ping but would it be possible to get any feedback or basic ACK here ?

[cushon]

I had a look

It reproduces with the CLI:

$ javac    -J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED   -J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED   -J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED   -J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED   -XDcompilePolicy=simple   -processorpath error_prone_core-2.33.0-with-dependencies.jar:dataflow-errorprone-3.41.0-eisop1.jar   '-Xplugin:ErrorProne -XepDisableAllChecks -Xep:UnusedVariable:ERROR -Xep:FormatStringAnnotation:ERROR' Violation.java  GenericException.java SpecialException.java  -cp error_prone_annotations-2.33.0.jar -XDverboseCompilePolicy
[attribute test.Violation]
[attribute test.GenericException]
[attribute test.SpecialException]
[flow test.Violation]
Violation.java:6: error: [UnusedVariable] The local variable 'x' is never read.
    int x = 0;
        ^
    (see https://errorprone.info/bugpattern/UnusedVariable)
  Did you mean to remove this line?
SpecialException.java:9: error: [FormatStringAnnotation] All variables passed as @FormatString must be final or effectively final
    super(message2, args2);
         ^
    (see https://errorprone.info/bugpattern/FormatStringAnnotation)
2 errors

EFFECTIVELY_FINAL is set in the 'flow' phase, which hasn't executed yet here: https://github.com/openjdk/jdk/blob/bd6264420b9f248999dd8387c25c549b08bd193a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java#L2220-L2221

With Violation.java as the last source on the command line, we get:

[attribute test.GenericException]
[attribute test.SpecialException]
[attribute test.Violation]
[flow test.GenericException]
[flow test.SpecialException]
[flow test.Violation]

-XDcompilePolicy=byfile (which I don't recommend unless JDK-8155674 is fixed) also works around it

[attribute test.Violation]
[flow test.Violation]
[desugar test.Violation]
[generate code test.Violation]
[attribute test.GenericException]
[flow test.GenericException]
[desugar test.GenericException]
[generate code test.GenericException]
[attribute test.SpecialException]
[flow test.SpecialException]
[desugar test.SpecialException]
[generate code test.SpecialException]

[cushon]

I think I understand what's going on now. This logic means that TaskListener.finished gets called for 'flow' events even if the actual 'flow' phase was skipped because errors were logged: https://github.com/openjdk/jdk/blob/bd6264420b9f248999dd8387c25c549b08bd193a/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java#L1403-L1432. So if Error Prone reports an error, subsequent compilations will skip flow but still do analysis.

Passing -XDshould-stop.ifError=FLOW also resolves the error, by ensuring 'flow' happens unconditionally before Error Prone runs.

I think -XDshould-stop.ifError=FLOW is a reasonable recommendation in general, especially when using -XDcompilePolicy=simple, because it makes the behaviour more similar to the default compile policy (JDK-8134117 is related)

So the outcome might be to update the installation docs to recommend that flag.

[msridhar]

@xenoterracide this might be the issue you were seeing in uber/NullAway#923?

Edit: nevermind looks like others already spotted this connection with #4305

[cushon]

I'm tentatively planning to require setting a stop-on-error policy when running Error Prone in the next release. 5828416 uses -XDshould-stop.ifError=FLOW, which relies on javac's internal support for -XD 'debug' flags that get written directly to the options table. There's also a hidden flag that doesn't rely on the -XD prefix, so perhaps we should be recommending that instead: --should-stop=ifError=FLOW

Either way, the logic that checks the flag is set should accept the different forms of the flag.

Also note that there were some changes to the syntax of this option: https://bugs.openjdk.org/browse/JDK-8162546, https://bugs.openjdk.org/browse/JDK-8198314

[cushon]

The fix is released in https://github.com/google/error-prone/releases/tag/v2.34.0

[Stephan202]

@cushon it appears that the new --should-stop=ifError=FLOW check isn't executed at least with Maven; see this anaysis.

[cushon]

[@cushon](https://github.com/cushon) it appears that the new --should-stop=ifError=FLOW check isn't executed at least with Maven; see this anaysis.

Thanks for the catch, I missed that there are two calls to checkCompilePolicy, including one in addTaskListener that handles the plugin case here. I'll add similar checking for --should-stop=ifError=FLOW.