mebigfatguy / fb-contrib

a FindBugs/SpotBugs plugin for doing static code analysis for java code bases
http://fb-contrib.sf.net
GNU Lesser General Public License v2.1
153 stars 45 forks source link

FCBL false positive with interfaces #320

Open Serranya opened 5 years ago

Serranya commented 5 years ago

Consider the following code

interface A {
    void doStuff()
}

class B implements A {
    private final Session session;

    public B(Session session) {
        this.session = session
    }

    @Override
    public void doStuff() {
         // do stuff with session
    }
}

this will trigger FCBL_FIELD_COULD_BE_LOCAL suggesting to pass session directly to the doStuff method instead using the constructor for that.

But the signature of doStuff is fixed because if the interface A.

I would suggest not to trigger FCBL_FIELD_COULD_BE_LOCAL when the method that uses the variable overrides another method.

What do you think?

ThrawnCA commented 5 years ago

Having looked at this, I think it's more broken than that; from my reading of the bug description, it already should not be firing here. It's supposed to fire when methods always store the variable before loading it, which clearly is not the case in your sample.

However, I can confirm that it does indeed trigger the detector. A somewhat more complete sample:

interface A {
    void doStuff();
}

class B implements A {
    private final String s;

    public B(String s) {
        this.s = s;
    }

    @Override
    public void doStuff() {
         System.out.println(s);
    }
}
mebigfatguy commented 5 years ago

Thanks for the report. i have no idea what the interface has to do with the problem. I would think it would be irrelevant. I'll take a look

mebigfatguy commented 5 years ago

I've added the sample provided, and it does not fire any false positives, can you take a look and see where i've gone wrong? (at bottom of file)

Serranya commented 5 years ago

@mebigfatguy The following of mine Projects triggers the Check: https://github.com/Serranya/de.serra.rocker-for-takes-maven-plugin/tree/reproducer

Just build the reproducer branch with mvn clean verify -Pdev.

You will need my parent pom to build the project. You can get it from here: https://bintray.com/serranya/serras-artifacts/de.serra.parent/1.0.26

You can for example add this to your settings.xml:

<repositories>
    <repository>
        <id>serras-artifacts</id>
        <name>bintray</name>
        <url>https://dl.bintray.com/serranya/serras-artifacts</url>
    </repository>
</repositories>

Maven output:

[...]
[INFO] --- spotbugs-maven-plugin:3.1.9:check (default) @ rocker-for-takes-maven-plugin ---
[INFO] BugInstance size is 3
[INFO] Error size is 0
[INFO] Total bugs: 3
[ERROR] Class de.serra.rft.DefaultResponseClass defines fields that are used only as locals [de.serra.rft.DefaultResponseClass] At DefaultResponseClass.java:[line 21] FCBL_FIELD_COULD_BE_LOCAL
[ERROR] Class de.serra.rft.FileWritingResponseClass defines fields that are used only as locals [de.serra.rft.FileWritingResponseClass] At FileWritingResponseClass.java:[line 19] FCBL_FIELD_COULD_BE_LOCAL
[ERROR] Class de.serra.rft.FileWritingResponseClass defines fields that are used only as locals [de.serra.rft.FileWritingResponseClass] At FileWritingResponseClass.java:[line 18] FCBL_FIELD_COULD_BE_LOCAL
[INFO] 

To see bug detail using the Spotbugs GUI, use the following command "mvn spotbugs:gui"
[...]
Serranya commented 5 years ago

@mebigfatguy Does the reproducer help you?

mebigfatguy commented 5 years ago

[WARNING] The requested profile "dev" could not be activated because it does not exist. [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project rocker-for-takes-maven-plugin: Fatal error compiling: invalid flag: --release -> [Help 1]

Serranya commented 5 years ago

@mebigfatguy hmm for some reason I can't reproduce it anymore. What about you @ThrawnCA