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.

Sign up for GitHub

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

Handle static members in inner classes in *CanBeStatic checks #4426

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Handle static members in inner classes in *CanBeStatic checks #4426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions annotations/src/main/java/module-info.java
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions check_api/src/main/java/com/google/errorprone/util/SourceVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ public static boolean supportsPatternMatchingInstanceof(Context context) {
return sourceIsAtLeast(context, 21);
}

/** Returns true if the compiler source version level supports static inner classes. */
public static boolean supportsStaticInnerClass(Context context) {
return sourceIsAtLeast(context, 16);
}

private static boolean sourceIsAtLeast(Context context, int version) {
Source lowerBound = Source.lookup(Integer.toString(version));
return lowerBound != null && Source.instance(context).compareTo(lowerBound) >= 0;
Expand Down
10 changes: 8 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/ClassCanBeStatic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.SourceVersion;
import com.sun.source.tree.ClassTree;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import javax.lang.model.element.Modifier;
Expand Down Expand Up @@ -60,12 +61,17 @@ public Description matchClass(ClassTree tree, VisitorState state) {
case TOP_LEVEL:
break;
case MEMBER:
// class is nested inside an inner class, so it can't be static
if (currentClass.owner.enclClass().hasOuterInstance()) {
if (!SourceVersion.supportsStaticInnerClass(state.context)
&& currentClass.owner.enclClass().hasOuterInstance()) {
// class is nested inside an inner class, so it can't be static
return NO_MATCH;
}
break;
case LOCAL:
if (!SourceVersion.supportsStaticInnerClass(state.context)) {
return NO_MATCH;
}
break;
case ANONYMOUS:
// members of local and anonymous classes can't be static
return NO_MATCH;
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/FieldCanBeStatic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.suppliers.Supplier;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.SourceVersion;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MemberSelectTree;
Expand Down Expand Up @@ -102,7 +103,8 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
if (enclClass == null) {
return NO_MATCH;
}
if (!enclClass.getNestingKind().equals(NestingKind.TOP_LEVEL)
if (!SourceVersion.supportsStaticInnerClass(state.context)
&& !enclClass.getNestingKind().equals(NestingKind.TOP_LEVEL)
&& !enclClass.isStatic()
&& symbol.getConstantValue() == null) {
// JLS 8.1.3: inner classes cannot declare static members, unless the member is a constant
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/MethodCanBeStatic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.SourceVersion;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.tree.ExpressionTree;
Expand Down Expand Up @@ -242,11 +243,16 @@ private static boolean isExcluded(MethodTree tree, VisitorState state) {
case TOP_LEVEL:
break;
case MEMBER:
if (sym.owner.enclClass().hasOuterInstance()) {
if (!SourceVersion.supportsStaticInnerClass(state.context)
&& sym.owner.enclClass().hasOuterInstance()) {
return true;
}
break;
case LOCAL:
if (!SourceVersion.supportsStaticInnerClass(state.context)) {
return true;
}
break;
case ANONYMOUS:
return true;
}
Expand Down
27 changes: 26 additions & 1 deletion core/src/test/java/com/google/errorprone/bugpatterns/ClassCanBeStaticTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

package com.google.errorprone.bugpatterns;

import static org.junit.Assume.assumeTrue;

import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.util.RuntimeVersion;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -30,7 +33,10 @@ public class ClassCanBeStaticTest {

@Test
public void negativeCase() {
compilationHelper.addSourceFile("ClassCanBeStaticNegativeCases.java").doTest();
compilationHelper
.addSourceFile("ClassCanBeStaticNegativeCases.java")
.setArgs("--release", "11")
.doTest();
}

@Test
Expand Down Expand Up @@ -334,6 +340,25 @@ public void nestedInLocal() {
" }",
" }",
"}")
.setArgs("--release", "11")
.doTest();
}

@Test
public void nestedInLocal_static() {
assumeTrue(RuntimeVersion.isAtLeast16());
compilationHelper
.addSourceLines(
"A.java", //
"public class A {",
" static void f() {",
" class Outer {",
" // BUG: Diagnostic contains:",
" class Inner {",
" }",
" }",
" }",
"}")
.doTest();
}

Expand Down
39 changes: 39 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/FieldCanBeStaticTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,45 @@ public void inner() {
" };",
" }",
"}")
.setArgs("--release", "11")
.doTest();
}

@Test
public void inner_static() {
assumeTrue(RuntimeVersion.isAtLeast16());
compilationHelper
.addSourceLines(
"Test.java",
"import java.time.Duration;",
"class Test {",
" class I {",
" // BUG: Diagnostic contains: can be static",
" private final Duration D = Duration.ofMillis(1);",
" // BUG: Diagnostic contains: can be static",
" private final int I = 42;",
" }",
" static class S {",
" // BUG: Diagnostic contains: can be static",
" private final Duration D = Duration.ofMillis(1);",
" // BUG: Diagnostic contains: can be static",
" private final int I = 42;",
" }",
" void f() {",
" class L {",
" // BUG: Diagnostic contains: can be static",
" private final Duration D = Duration.ofMillis(1);",
" // BUG: Diagnostic contains: can be static",
" private final int I = 42;",
" }",
" new Object() {",
" // BUG: Diagnostic contains: can be static",
" private final Duration D = Duration.ofMillis(1);",
" // BUG: Diagnostic contains: can be static",
" private final int I = 42;",
" };",
" }",
"}")
.doTest();
}

Expand Down
39 changes: 39 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/MethodCanBeStaticTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@

package com.google.errorprone.bugpatterns;

import static org.junit.Assume.assumeTrue;

import com.google.common.collect.ImmutableList;
import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.util.RuntimeVersion;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -327,6 +330,24 @@ public void innerClass() {
" }",
" }",
"}")
.setArgs("--release", "11")
.doTest();
}

@Test
public void innerClass_static() {
assumeTrue(RuntimeVersion.isAtLeast16());
testHelper
.addSourceLines(
"Test.java", //
"class Test {",
" class Inner {",
" // BUG: Diagnostic contains: static",
" private int incr(int x) {",
" return x + 1;",
" }",
" }",
"}")
.doTest();
}

Expand Down Expand Up @@ -370,6 +391,24 @@ public void negativeLocal() {
" }",
" }",
"}")
.setArgs("--release", "11")
.doTest();
}

@Test
public void positiveLocal() {
assumeTrue(RuntimeVersion.isAtLeast16());
testHelper
.addSourceLines(
"Test.java", //
"class Test {",
" static void foo() {",
" class Local {",
" // BUG: Diagnostic contains: static",
" private void foo() {}",
" }",
" }",
"}")
.doTest();
}

Expand Down
Loading