From e69955b7e49eb0e96b9a3a883ad88402a5141027 Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Thu, 12 Mar 2026 08:00:51 -0700 Subject: [PATCH] Cloned from CL 870815517 by 'g4 patch'. Added an ErrorProne check to detect record accessors inside the compact canonical ctors: As they read uninitialized fields, using them inside the compact canonical constructor is unlikely intentional. See also: https://gemini.google.com/share/bc7d0a22b687 PiperOrigin-RevId: 882592649 --- .../RecordAccessorInCompactConstructor.java | 111 +++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + ...ecordAccessorInCompactConstructorTest.java | 296 ++++++++++++++++++ .../RecordAccessorInCompactConstructor.md | 66 ++++ 4 files changed, 475 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructor.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructorTest.java create mode 100644 docs/bugpattern/RecordAccessorInCompactConstructor.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructor.java b/core/src/main/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructor.java new file mode 100644 index 00000000000..aa9f7736137 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructor.java @@ -0,0 +1,111 @@ +/* + * Copyright 2026 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.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Flags; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.RecordComponent; + +/** + * Detects when record accessors read uninitialized fields inside a compact constructor. Use the + * component parameter instead. + * + *

Example: + * + * {@snippet : + * record R(int x) { + * public R { + * int y = x(); // BUG: use `x` instead + * } + * } + * } + */ +@BugPattern( + summary = + "Record accessors read uninitialized fields inside a compact constructor. Use the" + + " component parameter instead.", + severity = ERROR) +public final class RecordAccessorInCompactConstructor extends BugChecker + implements MethodInvocationTreeMatcher { + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + MethodTree enclosingMethod = ASTHelpers.findEnclosingNode(state.getPath(), MethodTree.class); + if (enclosingMethod == null) { + return Description.NO_MATCH; + } + + MethodSymbol methodSym = ASTHelpers.getSymbol(enclosingMethod); + if (methodSym == null || (methodSym.flags() & Flags.COMPACT_RECORD_CONSTRUCTOR) == 0) { + return Description.NO_MATCH; + } + + if (state.findEnclosing(LambdaExpressionTree.class) != null) { + return Description.NO_MATCH; + } + + MethodSymbol calledMethod = ASTHelpers.getSymbol(tree); + if (calledMethod == null) { + return Description.NO_MATCH; + } + + ClassSymbol recordClass = methodSym.enclClass(); + if (!calledMethod.owner.equals(recordClass)) { + return Description.NO_MATCH; + } + + String componentName = null; + for (RecordComponent rc : recordClass.getRecordComponents()) { + if (calledMethod.equals(rc.accessor)) { + componentName = rc.name.toString(); + break; + } + } + + if (componentName == null) { + return Description.NO_MATCH; + } + + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver != null + && !(receiver instanceof IdentifierTree identifierTree + && identifierTree.getName().contentEquals("this"))) { + return Description.NO_MATCH; + } + + return buildDescription(tree) + .setMessage( + "Record accessors inside compact constructors read uninitialized fields (JLS" + + " §8.10.4.2). Use the parameter directly.") + .addFix(SuggestedFix.replace(tree, componentName)) + .build(); + } +} 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 18c37066110..a240f0b7bba 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -338,6 +338,7 @@ import com.google.errorprone.bugpatterns.RandomCast; import com.google.errorprone.bugpatterns.RandomModInteger; import com.google.errorprone.bugpatterns.ReachabilityFenceUsage; +import com.google.errorprone.bugpatterns.RecordAccessorInCompactConstructor; import com.google.errorprone.bugpatterns.RedundantControlFlow; import com.google.errorprone.bugpatterns.RedundantOverride; import com.google.errorprone.bugpatterns.RedundantSetterCall; @@ -851,6 +852,7 @@ public static ScannerSupplier warningChecks() { ProvidesNull.class, RandomCast.class, RandomModInteger.class, + RecordAccessorInCompactConstructor.class, RectIntersectReturnValueIgnored.class, RedundantSetterCall.class, RequiredModifiersChecker.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructorTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructorTest.java new file mode 100644 index 00000000000..b1d3a790400 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/RecordAccessorInCompactConstructorTest.java @@ -0,0 +1,296 @@ +/* + * Copyright 2026 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** {@link RecordAccessorInCompactConstructor}Test */ +@RunWith(JUnit4.class) +public final class RecordAccessorInCompactConstructorTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(RecordAccessorInCompactConstructor.class, getClass()); + + @Test + public void accessorCalledInCompactConstructor_flags() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + int x = d(); + } + } + """) + .doTest(); + } + + @Test + public void explicitThisAccessorCalledInCompactConstructor_flags() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + int x = this.d(); + } + } + """) + .doTest(); + } + + @Test + public void accessorCalledInNormalConstructor_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test(int d) { + this.d = d; + int x = d(); + } + } + """) + .doTest(); + } + + @Test + public void otherInstanceAccessorCalled_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + Test other = new Test(1); + int x = other.d(); + } + } + """) + .doTest(); + } + + @Test + public void nonAccessorMethodCalled_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + foo(); + } + + void foo() {} + } + """) + .doTest(); + } + + @Test + public void parameterAccessedDirectly_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + int x = d; + } + } + """) + .doTest(); + } + + @Test + public void normalClass_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Test { + private final int d; + + Test(int d) { + this.d = d; + int x = d(); + } + + int d() { + return d; + } + } + """) + .doTest(); + } + + @Test + public void fixApplied() { + BugCheckerRefactoringTestHelper.newInstance( + RecordAccessorInCompactConstructor.class, getClass()) + .addInputLines( + "Test.java", + """ + record Test(int d) { + public Test { + int x = d(); + int y = this.d(); + } + } + """) + .addOutputLines( + "Test.java", + """ + record Test(int d) { + public Test { + int x = d; + int y = d; + } + } + """) + .doTest(); + } + + @Test + public void lambdaInsideCompactConstructor_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + Runnable r = + () -> { + int x = d(); + }; + } + } + """) + .doTest(); + } + + @Test + public void anonymousClassInsideCompactConstructor_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + var r = + new Runnable() { + @Override + public void run() { + int x = d(); + } + }; + } + } + """) + .doTest(); + } + + @Test + public void multipleComponents_flags() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int a, String b, double c) { + public Test { + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + int x = a(); + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + String y = b(); + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + double z = this.c(); + } + } + """) + .doTest(); + } + + @Test + public void customAccessor_flags() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Test(int d) { + public Test { + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + int x = d(); + } + + @Override + public int d() { + return d; + } + } + """) + .doTest(); + } + + @Test + public void nestedRecord_flags() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Outer { + record Inner(int d) { + public Inner { + // BUG: Diagnostic contains: RecordAccessorInCompactConstructor + int x = d(); + } + } + } + """) + .doTest(); + } + + @Test + public void nestedRecord_callingOuterAccessor_noMatch() { + compilationHelper + .addSourceLines( + "Test.java", + """ + record Outer(int d) { + public Outer { + int x = d; + } + + record Inner(int e) { + public Inner { + Outer o = new Outer(1); + int x = o.d(); + } + } + } + """) + .doTest(); + } +} diff --git a/docs/bugpattern/RecordAccessorInCompactConstructor.md b/docs/bugpattern/RecordAccessorInCompactConstructor.md new file mode 100644 index 00000000000..bccc1c65798 --- /dev/null +++ b/docs/bugpattern/RecordAccessorInCompactConstructor.md @@ -0,0 +1,66 @@ +# RecordAccessorInCompactConstructor + +**Summary:** Record accessors should not be used inside compact constructors +because they read uninitialized fields. + +## The Problem + +In a Java `record`, using the accessor method (like `d()`) inside a **compact +constructor** (the one without arguments, `Foo { ... }`) reads the record's +underlying field before it has been set. This means the method will always +return `null` (for objects) or `0`/`false` (for primitives), regardless of what +arguments were passed to the constructor. + +## Examples + +### Bad: Using the accessor method + +```java +record User(String name) { + User { + // BUG: name() reads the uninitialized field 'this.name', which is currently + // null. This throws a NullPointerException immediately. + if (name().isEmpty()) { + throw new IllegalArgumentException("Name cannot be empty"); + } + } +} + +``` + +### Good: Using the parameter name directly + +```java +record User(String name) { + User { + // CORRECT: Reads the constructor parameter 'name'. + if (name.isEmpty()) { + throw new IllegalArgumentException("Name cannot be empty"); + } + } +} + +``` + +## Explanation + +The **Compact Constructor** in Java is a special initialization block that runs +*before* the record's fields are automatically assigned. + +1. **Code Execution:** The code inside your compact constructor `User { ... }` + runs first. +2. **Field Assignment:** The compiler automatically assigns the parameters to + the fields (`this.name = name;`) only *after* your code block finishes + ([JLS §8.10.4.2](https://docs.oracle.com/javase/specs/jls/se25/html/jls-8.html#jls-8.10.4.2)). + +When you call `name()`, it attempts to read `this.name`. Since the assignment +hasn't happened yet, `this.name` still holds its default value, which is `null` +([JLS §4.12.5](https://docs.oracle.com/javase/specs/jls/se25/html/jls-4.html#jls-4.12.5)). + +To fix this, refer to the component by its name (e.g., `name`). This accesses +the **parameter** passed to the constructor, which holds the correct value. + +## Suppression + +Suppress false positives by adding the suppression annotation +`@SuppressWarnings("RecordAccessorInCompactConstructor")` to the enclosing code.