Report bug SA_FIELD_SELF_ASSIGNMENT in nested classes as well (#2161)

* Report bug `SA_FIELD_SELF_ASSIGNMENT` in nested classes as well Bug `SA_FIELD_SELF_ASSIGNMENT` was not reported in nested classes, only in the outer class. This lead to inconsistent behavior. See issue ([#2142](#2142)). This PR fixes this issue. * Update spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java Co-authored-by: Kengo TODA <skypencil+github@gmail.com> * Update spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java Co-authored-by: Kengo TODA <skypencil+github@gmail.com> * Fixed according to the comments of @KengoTODA Co-authored-by: Kengo TODA <skypencil@gmail.com> Co-authored-by: Kengo TODA <skypencil+github@gmail.com>
spotbugs · Sep 5, 2022 · 7c835b6 · 7c835b6
1 parent 4c0c1b9
commit 7c835b6
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Bump up logback to `1.4.0`
 - Bump up log4j2 binding to `2.18.0`
 - Fixed InvalidInputException in Eclipse while bug reporting ([#2134](https://github.com/spotbugs/spotbugs/issues/2134))
+- Bug `SA_FIELD_SELF_ASSIGNMENT` is now reported from nested classes as well ([#2142](https://github.com/spotbugs/spotbugs/issues/2142))
 - Avoid warning on use of security manager on Java 17 and newer. ([#1579](https://github.com/spotbugs/spotbugs/issues/1579))
 - Fixed false positives `EI_EXPOSE_REP` thrown in case of fields initialized by the `of` or `copyOf` method of a `List`, `Map` or `Set` ([#1771](https://github.com/spotbugs/spotbugs/issues/1771))
 - Fixed CFGBuilderException thrown when `dup_x2` is used to swap the reference and wide-value (double, long) in the stack ([#2146](https://github.com/spotbugs/spotbugs/pull/2146))

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2142Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2142Test.java
@@ -0,0 +1,38 @@
+package edu.umd.cs.findbugs.detect;
+
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class Issue2142Test extends AbstractIntegrationTest {
+    @Test
+    public void test() {
+        performAnalysis("ghIssues/Issue2142.class",
+                "ghIssues/Issue2142$Inner.class");
+        BugInstanceMatcher matcher = new BugInstanceMatcherBuilder()
+                .bugType("SA_FIELD_SELF_ASSIGNMENT").build();
+        assertThat(getBugCollection(), containsExactly(2, matcher));
+
+        matcher = new BugInstanceMatcherBuilder()
+                .bugType("SA_FIELD_SELF_ASSIGNMENT")
+                .inClass("Issue2142")
+                .inMethod("foo1")
+                .atLine(6)
+                .build();
+        assertThat(getBugCollection(), hasItem(matcher));
+
+        matcher = new BugInstanceMatcherBuilder()
+                .bugType("SA_FIELD_SELF_ASSIGNMENT")
+                .inClass("Issue2142$Inner")
+                .inMethod("foo2")
+                .atLine(10)
+                .build();
+        assertThat(getBugCollection(), hasItem(matcher));
+    }
+}
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java
@@ -29,17 +29,20 @@
 import edu.umd.cs.findbugs.BugReporter;
 import edu.umd.cs.findbugs.LocalVariableAnnotation;
 import edu.umd.cs.findbugs.OpcodeStack;
+import edu.umd.cs.findbugs.OpcodeStack.CustomUserValue;
 import edu.umd.cs.findbugs.OpcodeStack.Item;
 import edu.umd.cs.findbugs.Priorities;
 import edu.umd.cs.findbugs.StatelessDetector;
 import edu.umd.cs.findbugs.SystemProperties;
 import edu.umd.cs.findbugs.ba.XField;
 import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
 
+@CustomUserValue
 public class FindFieldSelfAssignment extends OpcodeStackDetector implements StatelessDetector {
     private final BugReporter bugReporter;
 
     private static final boolean DEBUG = SystemProperties.getBoolean("fsa.debug");
+    private static final int NO_REGISTER = -1;
     int state;
 
     public FindFieldSelfAssignment(BugReporter bugReporter) {
@@ -69,6 +72,8 @@ public void visit(Code obj) {
 
     XField possibleOverwrite;
 
+    private int parentInstanceLoadFromRegister = NO_REGISTER;
+
     @Override
     public void sawOpcode(int seen) {
 
@@ -91,8 +96,11 @@ public void sawOpcode(int seen) {
                 OpcodeStack.Item fourth = stack.getStackItem(3);
                 XField f2 = third.getXField();
                 int registerNumber2 = fourth.getRegisterNumber();
-                if (f2 != null && f2.equals(getXFieldOperand()) && registerNumber2 >= 0
-                        && registerNumber2 == third.getFieldLoadedFromRegister()
+                int loadedFromRegister2 = fourth.getFieldLoadedFromRegister();
+                if (f2 != null && f2.equals(getXFieldOperand())
+                        && ((registerNumber2 >= 0 && registerNumber2 == third.getFieldLoadedFromRegister())
+                                || (loadedFromRegister2 >= 0 && third.getUserValue() instanceof Integer
+                                        && loadedFromRegister2 == ((Integer) third.getUserValue()).intValue()))
                         && !third.sameValue(top) && (third.getPC() == -1 || third.getPC() > lastMethodCall)) {
                     possibleOverwrite = f2;
                 }
@@ -161,7 +169,28 @@ public void sawOpcode(int seen) {
         default:
             break;
         }
+        if (seen == Const.GETFIELD) {
+            XField f = getXFieldOperand();
+            OpcodeStack.Item top = stack.getStackItem(0);
+            XField topF = top.getXField();
+            if (f == null || topF == null) {
+                return;
+            }
 
+            if ("this$0".equals(topF.getName())) {
+                parentInstanceLoadFromRegister = top.getFieldLoadedFromRegister();
+            }
+        }
     }
 
+    @Override
+    public void afterOpcode(int seen) {
+        super.afterOpcode(seen);
+
+        if (seen == Const.GETFIELD && parentInstanceLoadFromRegister > NO_REGISTER) {
+            OpcodeStack.Item top = stack.getStackItem(0);
+            top.setUserValue(parentInstanceLoadFromRegister);
+            parentInstanceLoadFromRegister = NO_REGISTER;
+        }
+    }
 }
diff --git a/spotbugsTestCases/src/java/ghIssues/Issue2142.java b/spotbugsTestCases/src/java/ghIssues/Issue2142.java
@@ -0,0 +1,13 @@
+package ghIssues;
+
+public class Issue2142 {
+    int foo;
+    void foo1() {
+        foo = foo++;  // can report a warning in this line
+    }
+    class Inner {
+        void foo2() {
+            foo = foo++; // should report a warning in this line
+        }
+    }    
+}