Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that -XDshould-stop.ifError=FLOW is set when running Error Prone #4618

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

copybara-service[bot]
Copy link
Contributor

Check that -XDshould-stop.ifError=FLOW is set when running Error Prone

This is necessary to ensure javac compiles the 'flow' phase for subsequent compilation units after Error Prone reports diagnostics. The 'flow' phase is necessary among other things to set EFFECTIVELY_FINAL. Configuring a later stop policy also causes more diagnostics to be shown at once for failing builds when using -XDcompilePolicy=simple, which is helpful.

#4595

RELNOTES=The javac flag -XDshould-stop.ifError=FLOW is now required when running Error Prone, see #4595

This is necessary to ensure javac compiles the 'flow' phase for subsequent compilation units after Error Prone reports diagnostics. The 'flow' phase is necessary among other things to set `EFFECTIVELY_FINAL`. Configuring a later stop policy also causes more diagnostics to be shown at once for failing builds when using `-XDcompilePolicy=simple`, which is helpful.

#4595

RELNOTES=The javac flag `-XDshould-stop.ifError=FLOW` is now required when running Error Prone, see #4595
PiperOrigin-RevId: 686102738
@copybara-service copybara-service bot merged commit 5828416 into master Oct 15, 2024
@copybara-service copybara-service bot deleted the test_686096407 branch October 15, 2024 14:50
@tbroyer
Copy link
Contributor

tbroyer commented Oct 15, 2024

@cushon Do you know off-hand if versions of ErrorProne before 2.11 (those versions compatible with JDK 8) would benefit from the JDK 8 -XDshouldStopPolicyIfError=FLOW?
I'm in the process of adding -XDshould-stop.ifError=FLOW to the gradle-errorprone-plugin and noticed it appeared in JDK 9, so wondered if I should add both variants for JDK 8 (that I currently still support, with older Error Prone versions) and JDK 9+, or if the JDK 9+ variant would be enough.

Fwiw, I won't write a test for that; I apparently can just pass both options and javac doesn't care and just ignores the unknown one, but if you tell me it's useless to pass the JDK 8 variant then I'll just pass the JDK 9+ variant.

@cushon
Copy link
Collaborator

cushon commented Oct 15, 2024

@cushon Do you know off-hand if versions of ErrorProne before 2.11 (those versions compatible with JDK 8) would benefit from the JDK 8 -XDshouldStopPolicyIfError=FLOW?

Yes, I believe the same behaviour exists in JDK 8 and it would benefit from the equivalent flag. (The flag got renamed in JDK 9 by openjdk/jdk@29aa24a)

I also left a related note in #4595 (comment), I'm not completely sure what form of the flag we want to recommend when this change is released.

@tbroyer
Copy link
Contributor

tbroyer commented Oct 16, 2024

Oh, BTW, I noticed 2 uses of the JDK 8 name in the codebase (in tests): https://github.com/search?q=repo%3Agoogle%2Ferror-prone%20shouldStopPolicyIfError&type=code

@cushon
Copy link
Collaborator

cushon commented Oct 16, 2024

Oh, BTW, I noticed 2 uses of the JDK 8 name in the codebase (in tests): https://github.com/search?q=repo%3Agoogle%2Ferror-prone%20shouldStopPolicyIfError&type=code

Thanks! I will clean those up

@tbroyer
Copy link
Contributor

tbroyer commented Oct 17, 2024

May I ask you for a quick review? tbroyer/gradle-errorprone-plugin#109
In the end, I added a test inspired by the ones added here.

I might revisit this later to conditionally pass -XDshouldStopPolicyIfError=FLOW for JDK 8, --should-stop:ifError=FLOW for JDK 9 and 10, and --should-stop=ifError=FLOW for JDK 11+
Let me know if you think I should do that from the start. TIA

copybara-service bot pushed a commit that referenced this pull request Oct 17, 2024
…stop=ifError=FLOW` is the default

As pointed out in #4618 (comment)

PiperOrigin-RevId: 686640507
copybara-service bot pushed a commit that referenced this pull request Oct 17, 2024
…stop=ifError=FLOW` is the default

As pointed out in #4618 (comment)

PiperOrigin-RevId: 686916247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants