Fix for false positive MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR when referencing but not calling an overridable method (spotbugs#2900)

* test: issue spotbugs#2837 reproducer

* fix: do not consider a method reference assigned to a field

* test: added false positive check for issue spotbugs#1867

It looks like the problem for spotbugs#1867 was fixed elsewhere

* test: added a true positive test for a lambda called in constructor

* doc: added changelog entry

* test: fixed tests

* fix: removed unused variable

* test: added a 2nd overridable method for the true positive case

Overridable methods are only reported once, add a 2nd one to make sure
the unit test shows the problem when assigning a method reference to a
field

Author: gtoison

CHANGELOG.md

@@ -28,6 +28,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Improved the bug description for VA_FORMAT_STRING_USES_NEWLINE when using text blocks, check the usage of String.formatted() ([#2881](https://github.com/spotbugs/spotbugs/pull/2881))
 - Fixed crash in ValueRangeAnalysisFactory when looking for redundant conditions used in assertions [#2887](https://github.com/spotbugs/spotbugs/pull/2887))
 - Revert again commons-text from 1.11.0 to 1.10.0 to resolve a version conflict ([#2686](https://github.com/spotbugs/spotbugs/issues/2686))
+- Fixed false positive MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR when referencing but not calling an overridable method [#2837](https://github.com/spotbugs/spotbugs/pull/2837))
 
 ### Added
 - New detector `MultipleInstantiationsOfSingletons` and introduced new bug types:

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue1867Test.java

@@ -0,0 +1,23 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.jupiter.api.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+
+class Issue1867Test extends AbstractIntegrationTest {
+
+    @Test
+    void testIssue() {
+        performAnalysis("ghIssues/Issue1867.class");
+        BugInstanceMatcher matcher = new BugInstanceMatcherBuilder()
+                .bugType("MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR")
+                .build();
+
+        assertThat(getBugCollection(), containsExactly(0, matcher));
+    }
+}

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2837Test.java

@@ -0,0 +1,38 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.jupiter.api.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+
+class Issue2837Test extends AbstractIntegrationTest {
+
+    @Test
+    void testIssue() {
+        performAnalysis("ghIssues/Issue2837.class");
+
+        assertBugCount("MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR", 1);
+        assertBugAtLine("MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR", 19);
+    }
+
+    private void assertBugCount(String type, int expectedCount) {
+        BugInstanceMatcher bugMatcher = new BugInstanceMatcherBuilder()
+                .bugType(type)
+                .build();
+
+        assertThat(getBugCollection(), containsExactly(expectedCount, bugMatcher));
+    }
+
+    private void assertBugAtLine(String type, int line) {
+        BugInstanceMatcher bugMatcher = new BugInstanceMatcherBuilder()
+                .bugType(type)
+                .atLine(line)
+                .build();
+
+        assertThat(getBugCollection(), containsExactly(1, bugMatcher));
+    }
+}

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java

@@ -146,6 +146,12 @@ public void sawOpcode(int seen) {
             }
             OpcodeStack.Item item = stack.getStackItem(0);
 
+            if (getNextOpcode() == Const.PUTFIELD) {
+                // INVOKEDYNAMIC followed by PUTFIELD means that we assigned the method reference to a field
+                // But we might be missing calls to that field afterwards, this would be a false negative
+                return;
+            }
+
             if (item.getRegisterNumber() == 0 && Const.CONSTRUCTOR_NAME.equals(getMethodName())) {
                 refCallerConstructors.put(constDyn.getBootstrapMethodAttrIndex(),
                         new CallerInfo(getXMethod(), SourceLineAnnotation.fromVisitedInstruction(this)));

spotbugsTestCases/src/java/ghIssues/Issue1867.java

@@ -0,0 +1,15 @@
+package ghIssues;
+
+import java.util.function.Function;
+
+public class Issue1867 {
+	private final Function<String, String> function;
+
+	public Issue1867() {
+		// MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR false positive: Claims "append" is overridable
+		function = v -> v + "!";
+
+		// just to ignore warnings about unused member
+		System.out.println(function.apply("hello"));
+	}
+}

spotbugsTestCases/src/java/ghIssues/Issue2837.java

@@ -0,0 +1,20 @@
+package ghIssues;
+
+import java.util.function.Predicate;
+import java.util.stream.Stream;
+
+public class Issue2837 {
+	public boolean overridable1(String s) {
+		return true;
+	}
+	
+	public boolean overridable2(String s) {
+		return true;
+	}
+
+	// overridable method referenced but not used
+	public final Predicate<String> predicate1 = this::overridable1;
+	
+	// overridable method used
+	public final long matchesCount = Stream.of("Test").filter(this::overridable2).count();
+}