diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ArrayRecordComponent.java b/core/src/main/java/com/google/errorprone/bugpatterns/ArrayRecordComponent.java new file mode 100644 index 00000000000..72f80dd1099 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ArrayRecordComponent.java @@ -0,0 +1,41 @@ +/* + * Copyright 2024 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 com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +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; +import javax.lang.model.type.TypeKind; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Record components should not be arrays.", severity = WARNING) +public final class ArrayRecordComponent extends BugChecker implements VariableTreeMatcher { + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + var sym = ASTHelpers.getSymbol(tree); + // isRecord(VarSymbol) is true iff the symbol represents a record component + if (ASTHelpers.isRecord(sym) && sym.asType().getKind() == TypeKind.ARRAY) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } +} 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 786d0668bd9..682043064f7 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -34,6 +34,7 @@ import com.google.errorprone.bugpatterns.ArrayEquals; import com.google.errorprone.bugpatterns.ArrayFillIncompatibleType; import com.google.errorprone.bugpatterns.ArrayHashCode; +import com.google.errorprone.bugpatterns.ArrayRecordComponent; import com.google.errorprone.bugpatterns.ArrayToString; import com.google.errorprone.bugpatterns.ArraysAsListPrimitiveArray; import com.google.errorprone.bugpatterns.AssertFalse; @@ -855,6 +856,7 @@ public static ScannerSupplier warningChecks() { AnnotateFormatMethod.class, ArgumentSelectionDefectChecker.class, ArrayAsKeyOfSetOrMap.class, + ArrayRecordComponent.class, AssertEqualsArgumentOrderChecker.class, AssertThrowsMultipleStatements.class, AssertionFailureIgnored.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ArrayRecordComponentTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ArrayRecordComponentTest.java new file mode 100644 index 00000000000..f2b57218e1c --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ArrayRecordComponentTest.java @@ -0,0 +1,81 @@ +/* + * Copyright 2024 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.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class ArrayRecordComponentTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(ArrayRecordComponent.class, getClass()); + + @Test + public void positive_recordWithArrayComponents() { + testHelper + .addSourceLines( + "Foo.java", + "record Foo(", + " String string,", + " // BUG: Diagnostic contains: Record components should not be arrays", + " int[] array,", + " String otherString,", + " // BUG: Diagnostic contains: Record components should not be arrays", + " String[] otherArray) {}") + .doTest(); + } + + @Test + public void negative_recordWithNonArrayComponents() { + testHelper + .addSourceLines( + "Foo.java", + "record Foo(", + " String string,", + " java.util.List list,", + " int integer) {}") + .doTest(); + } + + @Test + public void negative_recordWithStaticArrayField() { + testHelper + .addSourceLines( + "Foo.java", + "record Foo(String string) {", + " private static final int[] ints = {1, 2, 3};", + "}") + .doTest(); + } + + @Test + public void negative_nonRecordWithArrayField() { + testHelper + .addSourceLines( + "IntArrayHolder.java", + "public class IntArrayHolder {", + " public final int[] ints;", + "", + " public IntArrayHolder(int[] ints) {", + " this.ints = ints;", + " }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/ArrayRecordComponent.md b/docs/bugpattern/ArrayRecordComponent.md new file mode 100644 index 00000000000..f3b6425b58f --- /dev/null +++ b/docs/bugpattern/ArrayRecordComponent.md @@ -0,0 +1,12 @@ +There are two main problems with having a component of a record be an array. + +1. By default, the generated `equals` and `hashCode` will just call `equals` or + `hashCode` on the array. Two distinct arrays are never considered equal by + `equals` even if their contents are the same. The generated `toString` is + similarly not useful, since it will be something like `[B@723279cf`. + +2. Arrays are mutable, but records should not be mutable. A client of a record + with an array component can change the contents of the array. + +Instead of an array component, consider something like `ImmutableList`, +or, for primitive arrays, something like `ByteString` or `ImmutableIntArray`.