Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False-positive MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR for method reference #2837

Open
nmatt opened this issue Jan 24, 2024 · 4 comments
Open

Comments

@nmatt
Copy link

nmatt commented Jan 24, 2024

MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR is reported for mere method references, without them being invoked. This happens with SpotBugs version 4.8.3 (current release).

Example code:

public class Example
{
    public String s;
    
    public boolean overridable(String s) { return this.s.equals(s); }
    
    public final Predicate<String> predicate1 = this::overridable;

    public final Predicate<String> predicate2 = Predicate.not(this::overridable);
}

Both predicate1 and predicate2 produce the bug. In case of predicate1, the method is clearly not called. In the case of predicate2, it might of course theoretically be called, but there are also a lot of use cases (like here) where the method reference is just passed to be stored for later use. (One of the actual use cases I have is a memoization wrapper for the overridable method, stored as a field.)

One issue is that this also cannot be worked around by adding @SuppressFBWarnings("MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR") to the field declaration.

Maybe, if the analysis cannot determine that the overridable method is being passed for immediate invocation (like when passed to well-known methods like Map::compute), this should instead map to a separate bug code.

@gtoison
Copy link
Contributor

gtoison commented Mar 12, 2024

I think this is the same issue as #1867

hazendaz pushed a commit that referenced this issue Mar 27, 2024
… referencing but not calling an overridable method (#2900)

* test: issue #2837 reproducer

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

* test: added false positive check for issue #1867

It looks like the problem for #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
PatrikScully pushed a commit to PatrikScully/spotbugs that referenced this issue Jun 14, 2024
… 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
@JuditKnoll
Copy link
Collaborator

As far as I can see, this was fixed by #2900.

@nmatt
Copy link
Author

nmatt commented Aug 28, 2024

@JuditKnoll: This doesn't appear to be actually fixed. The false positive still occurs in 4.8.6 (and 4.8.4 and 4.8.5) with the provided example. Can you please reopen?

@JuditKnoll
Copy link
Collaborator

Sorry, my bad. Thanks for checking it.

@JuditKnoll JuditKnoll reopened this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants