From d546886f2ba59cc8352935d40dc2c735fe8c5f99 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Mon, 4 Sep 2023 06:41:03 -0700 Subject: [PATCH] Create `ReturnAtTheEndOfVoidFunction` which requires that void functions have no `return;` as a last statement which are a no-op. PiperOrigin-RevId: 562545799 --- .../ReturnAtTheEndOfVoidFunction.java | 71 ++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../ReturnAtTheEndOfVoidFunctionTest.java | 151 ++++++++++++++++++ .../ReturnAtTheEndOfVoidFunction.md | 19 +++ 4 files changed, 243 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunction.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunctionTest.java create mode 100644 docs/bugpattern/ReturnAtTheEndOfVoidFunction.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunction.java b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunction.java new file mode 100644 index 00000000000..075d164baab --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunction.java @@ -0,0 +1,71 @@ +/* + * Copyright 2023 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.bugpatterns; + +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getType; + +import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.StatementTree; +import com.sun.tools.javac.code.Type; +import java.util.List; +import javax.lang.model.type.TypeKind; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = "`return;` is unnecessary at the end of void methods and constructors.", + severity = WARNING) +public final class ReturnAtTheEndOfVoidFunction extends BugChecker implements MethodTreeMatcher { + + @Override + public Description matchMethod(MethodTree methodTree, VisitorState state) { + + // is a constructor or has a void return type (Void should pass, as `return null;` is required) + Type returnType = getType(methodTree.getReturnType()); + if (returnType != null && returnType.getKind() != TypeKind.VOID) { + return NO_MATCH; + } + + // has a body (not abstract or native) + BlockTree body = methodTree.getBody(); + if (body == null) { + return NO_MATCH; + } + + // body is not empty + List statements = body.getStatements(); + if (statements == null || statements.isEmpty()) { + return NO_MATCH; + } + + // last statement is a return + StatementTree lastStatement = Iterables.getLast(statements); + if (lastStatement.getKind() != StatementTree.Kind.RETURN) { + return NO_MATCH; + } + + return describeMatch(methodTree, SuggestedFix.delete(lastStatement)); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index c6a6a6740d9..c6d1533a419 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -320,6 +320,7 @@ import com.google.errorprone.bugpatterns.RequiredModifiersChecker; import com.google.errorprone.bugpatterns.RestrictedApiChecker; import com.google.errorprone.bugpatterns.RethrowReflectiveOperationExceptionAsLinkageError; +import com.google.errorprone.bugpatterns.ReturnAtTheEndOfVoidFunction; import com.google.errorprone.bugpatterns.ReturnValueIgnored; import com.google.errorprone.bugpatterns.ReturnsNullCollection; import com.google.errorprone.bugpatterns.RobolectricShadowDirectlyOn; @@ -1013,6 +1014,7 @@ public static ScannerSupplier warningChecks() { ReachabilityFenceUsage.class, ReferenceEquality.class, RethrowReflectiveOperationExceptionAsLinkageError.class, + ReturnAtTheEndOfVoidFunction.class, ReturnFromVoid.class, RobolectricShadowDirectlyOn.class, RxReturnValueIgnored.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunctionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunctionTest.java new file mode 100644 index 00000000000..bed2653de76 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnAtTheEndOfVoidFunctionTest.java @@ -0,0 +1,151 @@ +/* + * Copyright 2023 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.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for the {@link ReturnAtTheEndOfVoidFunction}. */ +@RunWith(JUnit4.class) +public class ReturnAtTheEndOfVoidFunctionTest { + + private final BugCheckerRefactoringTestHelper helper = + BugCheckerRefactoringTestHelper.newInstance(ReturnAtTheEndOfVoidFunction.class, getClass()); + + @Test + public void lastReturnIsDeleted() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public void stuff() {", + " int x = 5;", + " return;", + " }", + "}") + .addOutputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public void stuff() {", + " int x = 5;", + " }", + "}") + .doTest(); + } + + @Test + public void lastReturnIsNotDeleted() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public int stuff() {", + " int x = 5;", + " return x;", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void returnAtDifferentPositionIsNotDeleted() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public void stuff() {", + " int x = 5;", + " if(x > 2) {", + " return;", + " }", + " int z = 2173;", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void emptyFunctionIsUnchanged() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public void nothing() {", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void nullReturnInVoidIsUnchanged() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public Void nothing() {", + " return null;", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void constructorIsCleaned() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public Builder() {", + " return;", + " }", + "}") + .addOutputLines( + "Builder.java", + "package com.google.gporeba;", + "public final class Builder {", + " public Builder() {", + " }", + "}") + .doTest(); + } + + @Test + public void abstractDoesntCrash() { + helper + .addInputLines( + "Builder.java", + "package com.google.gporeba;", + "public abstract class Builder {", + " public abstract void stuff();", + "}") + .expectUnchanged() + .doTest(); + } +} diff --git a/docs/bugpattern/ReturnAtTheEndOfVoidFunction.md b/docs/bugpattern/ReturnAtTheEndOfVoidFunction.md new file mode 100644 index 00000000000..2129e45b3cb --- /dev/null +++ b/docs/bugpattern/ReturnAtTheEndOfVoidFunction.md @@ -0,0 +1,19 @@ +Detects no-op `return` statements in `void` functions when they occur at the end +of the method. + +Instead of: + +```java +public void stuff() { + int x = 5; + return; +} +``` + +do: + +```java +public void stuff() { + int x = 5; +} +```