Add new rule set PA_PUBLIC_PRIMITIVE_ATTRIBUTE, PA_PUBLIC_ARRAY_ATTRIBUTE and PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE

[baloghadamsoftware]

SEI CERT rule OBJ01-J requires that accessibility of fields must be
limited. In general, requiring that no fields must be public is
overkill and unrealistic. Even the rule mentions that final fields
may be public, except if the type of the field is a mutable reference
type which can be modified despite the reference itself being final.
Beside final field, there may be other usages for public fields: some
public fields may serve as "flags" which affect the behavior of the
class. These fields are expected to be read by the containing class
itself and written by other classes. If a field is both written by
the methods of the containing class itself and from outside is
suspicious. This new check PA_PUBLIC_ATTRIBUTES warns for such
fields.

Make sure these boxes are checked before submitting your PR -- thank you!

• Added an entry into CHANGELOG.md if you have changed SpotBugs code

[baloghadamsoftware]

Hello! This is my first attempt to contribute to SpotBugs so please excuse me if I overlooked something. I am not sure whether I put it into the right place or I should make it as a plugin. (I see that some checkers are "core" checkers and some are implemented as a plugin checker.) We tested this checker on several open-source projects and our experience is that it only gives few new findings and all of them are true positives.

[baloghadamsoftware]

Thank you for your comments! I tried to address all of them.

[ThrawnCA]

Looks okay from a general code quality perspective, though I'm not an expert on the bug detection logic.

Please try to avoid force-pushing, as it messes up the history (eg following the Github notification leads to a page that says it can't find the commits).

[KengoTODA]

Do we need this limitation?
Actually, the page describing this rule has no limitation like this.

[baloghadamsoftware]

Do you mean that we only report public attributes which are written by the methods of the class? We introduced this limitation to reduce noise of the detector and make this rule more realistic to follow. Completely forbidding non-final public attributes seems overkill and results in many false positives. Our first try (which resulted in a much simpler detector but with more false positives) was that if the class is something like a C struct, thus it has no methods at all maybe except a constructor then we do not check it for public attributes. Even this did not seem enough because we found that some classes may have flags that can be set from outside and influence the behavior of the class. E.g. a data member called DEBUG may be set from outside to change logging level but methods of the class itself only read it. However, if the class itself writes an attributes that means that the class is responsible for that attribute and thus it should be a private (or protected member).

[baloghadamsoftware]

This detector basically checks for public attributes according to the SEI CERT rule. It is somewhat similar that the MS detector, but it checks non-static fields. Furthermore we tried to reduce the number of false positives by restricting the error messages to fields which are written by the class. If you think that unnecessary then I can remove this limitation but then we get much more false positives. (My first attempt was to only filter out public attributes in classes without methods [except constructors], but I found that the number of false positives is still too high.)

[baloghadamsoftware]

Dunno why building SpotbugsGui fails now. I did not modify that part. On my computer I have successful build.

[ThrawnCA]

Please don't force-push.

[baloghadamsoftware]

Please don't force-push.

Sorry, I did not force push the whole commit, just fixed the last one because I forgot to rebase from master before pushing.

[whisperity]

Style and phrasing related comments, mostly targeting the user-facing documentation.

[whisperity]

Wouldn't .contains('$') insteaf of .indexOf('$') >= 0 be more idiomatic?

[ThrawnCA]

No, indexOf is more efficient for single characters. contains searches for entire strings.

[baloghadamsoftware]

Sorry for force pushing but there was a git mess which I had to sort out.

[baloghadamsoftware]

Using our heuristics (only reporting public fields which are written by their owning class) SpotBugs found relatively few new bugs:
MATSim
SpotBugs itself

There were no new findings on the rest of the projects.

[ThrawnCA]

No, indexOf is more efficient for single characters. contains searches for entire strings.

[baloghadamsoftware]

@KengoTODA?

Long ago asked and answered question but no feed back. Answer seemed to explain it well. Only dismissing as i cannot accept that as resolved otherwise.