From aad527fdf94b8fae405cc140053336f34ec5aab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81d=C3=A1m=20Balogh?= Date: Thu, 1 Sep 2022 13:56:47 +0200 Subject: [PATCH 1/4] 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](https://github.com/spotbugs/spotbugs/issues/2142)). This PR fixes this issue. --- CHANGELOG.md | 1 + .../umd/cs/findbugs/detect/Issue2142Test.java | 38 +++++++++++++++++++ .../detect/FindFieldSelfAssignment.java | 31 ++++++++++++++- .../src/java/ghIssues/Issue2142.java | 13 +++++++ 4 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2142Test.java create mode 100644 spotbugsTestCases/src/java/ghIssues/Issue2142.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 6435707e6af..04987d24a80 100644 --- 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)) ## 4.7.1 - 2022-06-26 ### Fixed 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 new file mode 100644 index 00000000000..abb1267cfa1 --- /dev/null +++ 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 index fac48805c42..b52013d8e3c 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java @@ -69,6 +69,8 @@ public void visit(Code obj) { XField possibleOverwrite; + private int parentInstanceLoadFromRegister = -1; + @Override public void sawOpcode(int seen) { @@ -91,8 +93,12 @@ 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() != null + && third.getUserValue() instanceof Integer + && loadedFromRegister2 == (Integer) third.getUserValue())) && !third.sameValue(top) && (third.getPC() == -1 || third.getPC() > lastMethodCall)) { possibleOverwrite = f2; } @@ -161,7 +167,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 > -1) { + OpcodeStack.Item top = stack.getStackItem(0); + top.setUserValue(parentInstanceLoadFromRegister); + parentInstanceLoadFromRegister = -1; + } + } } diff --git a/spotbugsTestCases/src/java/ghIssues/Issue2142.java b/spotbugsTestCases/src/java/ghIssues/Issue2142.java new file mode 100644 index 00000000000..942da9d37b7 --- /dev/null +++ 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 + } + } +} From 8c08746cc5b7bc27e6bb5854b5507d18539f16ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Balogh=2C=20=C3=81d=C3=A1m?= Date: Fri, 2 Sep 2022 10:48:16 +0200 Subject: [PATCH 2/4] Update spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java Co-authored-by: Kengo TODA --- .../edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 index b52013d8e3c..82d3be61348 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java @@ -96,8 +96,7 @@ public void sawOpcode(int seen) { int loadedFromRegister2 = fourth.getFieldLoadedFromRegister(); if (f2 != null && f2.equals(getXFieldOperand()) && ((registerNumber2 >= 0 && registerNumber2 == third.getFieldLoadedFromRegister()) - || (loadedFromRegister2 >= 0 && third.getUserValue() != null - && third.getUserValue() instanceof Integer + || (loadedFromRegister2 >= 0 && third.getUserValue() instanceof Integer && loadedFromRegister2 == (Integer) third.getUserValue())) && !third.sameValue(top) && (third.getPC() == -1 || third.getPC() > lastMethodCall)) { possibleOverwrite = f2; From ee9ae11b993fff0c221fa36271a3c5f34bd89d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Balogh=2C=20=C3=81d=C3=A1m?= Date: Fri, 2 Sep 2022 10:48:31 +0200 Subject: [PATCH 3/4] Update spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java Co-authored-by: Kengo TODA --- .../edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 82d3be61348..5c2bfb4afb5 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindFieldSelfAssignment.java @@ -97,7 +97,7 @@ public void sawOpcode(int seen) { if (f2 != null && f2.equals(getXFieldOperand()) && ((registerNumber2 >= 0 && registerNumber2 == third.getFieldLoadedFromRegister()) || (loadedFromRegister2 >= 0 && third.getUserValue() instanceof Integer - && loadedFromRegister2 == (Integer) third.getUserValue())) + && loadedFromRegister2 == ((Integer) third.getUserValue()).intValue())) && !third.sameValue(top) && (third.getPC() == -1 || third.getPC() > lastMethodCall)) { possibleOverwrite = f2; } From 2604f15fea005addfbb880346262b18c4b2ae554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81d=C3=A1m=20Balogh?= Date: Fri, 2 Sep 2022 11:41:01 +0200 Subject: [PATCH 4/4] Fixed according to the comments of @KengoTODA --- .../umd/cs/findbugs/detect/FindFieldSelfAssignment.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 index 5c2bfb4afb5..1ea040313b8 100644 --- 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,6 +29,7 @@ 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; @@ -36,10 +37,12 @@ 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,7 +72,7 @@ public void visit(Code obj) { XField possibleOverwrite; - private int parentInstanceLoadFromRegister = -1; + private int parentInstanceLoadFromRegister = NO_REGISTER; @Override public void sawOpcode(int seen) { @@ -184,10 +187,10 @@ public void sawOpcode(int seen) { public void afterOpcode(int seen) { super.afterOpcode(seen); - if (seen == Const.GETFIELD && parentInstanceLoadFromRegister > -1) { + if (seen == Const.GETFIELD && parentInstanceLoadFromRegister > NO_REGISTER) { OpcodeStack.Item top = stack.getStackItem(0); top.setUserValue(parentInstanceLoadFromRegister); - parentInstanceLoadFromRegister = -1; + parentInstanceLoadFromRegister = NO_REGISTER; } } }