Skip to content

Commit

Permalink
Pass should-stop.ifError=FLOW in addition to compilePolicy=simple
Browse files Browse the repository at this point in the history
It fixes a few issues in all versions of Error Prone, and the
next version will make it mandatory.
  • Loading branch information
tbroyer committed Oct 16, 2024
1 parent c1ab7d3 commit a04bded
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ class ErrorPronePluginIntegrationTest : AbstractPluginIntegrationTest() {
File(
testProjectDir.resolve("customCheck/src/main/java/com/google/errorprone/sample").apply { mkdirs() },
"MyCustomCheck.java",
).writeText(javaClass.getResource("/com/google/errorprone/sample/MyCustomCheck.java").readText())
).writeText(javaClass.getResource("/com/google/errorprone/sample/MyCustomCheck.java")!!.readText())

buildFile.appendText(
"""
Expand All @@ -157,7 +157,7 @@ class ErrorPronePluginIntegrationTest : AbstractPluginIntegrationTest() {
File(
testProjectDir.resolve("src/main/java/com/google/errorprone/sample").apply { mkdirs() },
"Hello.java",
).writeText(javaClass.getResource("/com/google/errorprone/sample/Hello.java").readText())
).writeText(javaClass.getResource("/com/google/errorprone/sample/Hello.java")!!.readText())

// when
val result = testProjectDir.buildWithArgsAndFail("compileJava")
Expand Down Expand Up @@ -200,4 +200,112 @@ class ErrorPronePluginIntegrationTest : AbstractPluginIntegrationTest() {
// As it didn't fail, it means the rest of the configuration was properly persisted/reloaded.
assertThat(result.output).contains("-Xplugin:ErrorProne")
}

// Inspired by the tests added in Error Prone's https://github.com/google/error-prone/pull/4618
@Test
fun `should-stop ifError`() {
// given
settingsFile.appendText(
"""
include(":customCheck")
""".trimIndent(),
)
File(testProjectDir.resolve("customCheck").apply { mkdirs() }, "build.gradle.kts").writeText(
"""
plugins {
java
}
repositories {
mavenCentral()
}
dependencies {
compileOnly("com.google.errorprone:error_prone_check_api:$errorproneVersion")
}
${if (testJavaVersion.isJava8) {
""
} else {
"""
tasks {
compileJava {
options.compilerArgs.addAll(listOf(
"--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED"
))
}
}
""".trimIndent()
}}
""".trimIndent(),
)
File(
testProjectDir.resolve("customCheck/src/main/resources/META-INF/services").apply { mkdirs() },
"com.google.errorprone.bugpatterns.BugChecker",
).writeText(
"""
com.google.errorprone.sample.CPSChecker
com.google.errorprone.sample.EffectivelyFinalChecker
""".trimIndent(),
)

File(
testProjectDir.resolve("customCheck/src/main/java/com/google/errorprone/sample").apply { mkdirs() },
"CPSChecker.java",
).writeText(javaClass.getResource("/com/google/errorprone/sample/CPSChecker.java")!!.readText())
File(
testProjectDir.resolve("customCheck/src/main/java/com/google/errorprone/sample").apply { mkdirs() },
"EffectivelyFinalChecker.java",
).writeText(javaClass.getResource("/com/google/errorprone/sample/EffectivelyFinalChecker.java")!!.readText())

buildFile.appendText(
"""
dependencies {
errorprone(project(":customCheck"))
}
tasks.withType<JavaCompile>().configureEach {
options.errorprone.error("CPSChecker", "EffectivelyFinalChecker")
}
""".trimIndent(),
)

File(
testProjectDir.resolve("src/main/java/com/google/errorprone/sample").apply { mkdirs() },
"A.java",
).writeText(
"""
package com.google.errorprone.sample;
class A {
int f(int x) {
return x;
}
}
""".trimIndent(),
)
File(
testProjectDir.resolve("src/main/java/com/google/errorprone/sample").apply { mkdirs() },
"B.java",
).writeText(
"""
package com.google.errorprone.sample;
class B {
int f(int x) {
return x;
}
}
""".trimIndent(),
)

// when
val result = testProjectDir.buildWithArgsAndFail("compileJava")

// then
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.FAILED)
assertThat(result.output).contains("A.java:5: error: [CPSChecker]")
assertThat(result.output).contains("B.java:5: error: [CPSChecker]")
assertThat(result.output).doesNotContain("[EffectivelyFinalChecker]")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright 2011 The Error Prone Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.sample;

import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.ReturnTree;

@BugPattern(
name = "CPSChecker",
summary = "Using 'return' is considered harmful",
explanation = "Please refactor your code into continuation passing style.",
severity = ERROR)
public class CPSChecker extends BugChecker implements ReturnTreeMatcher {
@Override
public Description matchReturn(ReturnTree tree, VisitorState state) {
return describeMatch(tree);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright 2011 The Error Prone Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.sample;

import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.Description.NO_MATCH;

import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.VariableTree;

@BugPattern(
name = "EffectivelyFinalChecker",
summary = "All variables should be effectively final",
severity = ERROR)
public class EffectivelyFinalChecker extends BugChecker implements VariableTreeMatcher {
@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
if (ASTHelpers.isConsideredFinal(ASTHelpers.getSymbol(tree))) {
return NO_MATCH;
}
return describeMatch(tree);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ internal class ErrorProneCompilerArgumentProvider(

override fun asArguments(): Iterable<String> {
return when {
errorproneOptions.isEnabled.getOrElse(false) -> listOf("-Xplugin:ErrorProne $errorproneOptions", "-XDcompilePolicy=simple")
// should-stop.ifError is for JDK 9+, shouldStopPolicyIfError for JDK 8; it's safe to indiscriminately pass both
errorproneOptions.isEnabled.getOrElse(false) -> listOf("-Xplugin:ErrorProne $errorproneOptions", "-XDcompilePolicy=simple", "-XDshould-stop.ifError=FLOW", "-XDshouldStopPolicyIfError=FLOW")
else -> emptyList()
}
}
Expand Down

0 comments on commit a04bded

Please sign in to comment.