#51 FCBL_FIELD_COULD_BE_LOCAL: false positive

[Tobias Baum]

Hi,

I would like to use the FCBL_FIELD_COULD_BE_LOCAL-Check in a project, but it seems to have some false positives. They all resemble the following (heavily stripped down) case:

public final class Testclass {

private boolean flag;
private final Testclass t;

public Testclass(Testclass t) {
this.t = t;
}

public void setFlag() {
this.flag = true;
}

public void perform() {
this.flag = false;
do {
this.t.setFlag();
} while(!this.flag);
}

}

The check says that "flag" could be local. It is certainly true that the first action in every method is a store to "flag", but that's not sufficient in this case. (The algorithm doesn't make much sense in the stripped down example, but we have some real code that work's in a similar way).

[Dave Brosius]

Thanks for the report. I'm sure you have a real situation, it's just i'm not understanding what it is, and i don't think the example shows the problem you are having. how would this sample behave differently if it was a local?

[Tobias Baum]

You're right, I botched the example up a little bit because there is no way to instantiate Testclass without running into a NullPointerException in perform... So there has to be a setter for t or a second constructor.

So consider "t" has been set to "this" in one of these ways. With "flag" being an attribute, the code in "perform" will enter the loop only once. If "flag" is changed into two local variables, "setFlag" will become a no-op and "perform" will loop infinitely.

[thinkpuppet]

I believe we have observed a variation of this in 4.6.0. We get what looks like a false positive when one instance of a class accesses private fields of another instance of the same class in the constructor. If you would like sample code I can whip something up. BTW awesome lib.

[Dave Brosius]

an example would be great, thanks!

[Dave Brosius]

actually, can you create a new bug please? i believe i've fixed this one, now.

https://github.com/mebigfatguy/fb-contrib/commit/68edbd0dfb99ab9529193843e313a7a647812f66

[thinkpuppet]

I sure can; I will try to get that sample class and file in the next day or so.

[thinkpuppet]

I was about to file a new bug but now I am not so sure. The code which is failing FCBL does something with a dubious code smell. The problem revolves around 'copy constructors' where the constructor of a class copies fields from a template instance of the same class.

In the examples we have that violate FCBL, a certain field is retained purely for use in the copy constructor (e.g. for performing a lookup). The trivial example will look a bit dubious as a result, and for sure we could re-factor to avoid this.

The question becomes whether this is really a bug in FCBL, or a dubious piece of code we should just add to our exclude file. In any case here's the example, if you still think it is a bug in FCBL, I can file it separately.

The field bar_ in the class Foo below violates FCBL in 4.6.0. Because it is used in the copy constructor, it must be a field rather than local so it is available to the next copy constructor.

(Again, this is a somewhat contrived / over-simplified example ;)

public final class Foo
{
private final String id_;
private final String bar_;
private final int baz_;

public Foo( String id, String bar, int baz )
{
    id_ = id;
    bar_ = bar;
    baz_ = baz;
}

public Foo( String id, Foo template, Provider provider )
{
    id_ = id;
    bar_ = template.bar_;

    baz_ = provider.doSomethingExpensive( id_, bar_ );
}

public String getId()
{
    return id_;
}

public int getBaz()
{
    return baz_;
}

}

[Dave Brosius]

as you state, as posted there's no point for bar_ to be a field, so there is no FP here. Of course, i certainly understand how in 'real code' there is possibly an issue that you can't boil down here, but still, there's really nothing i can do about it here, without further info.