spotbugs / discuss

SpotBugs mailing list
6 stars 1 forks source link

Can Spotbugs enforce private visibility and single-read access for variables of a certain type? #54

Open smillies opened 6 years ago

smillies commented 6 years ago

I'd like to enforce best practices for the use of the SafeDCLFactory from Alex Shipilev’s paper on safe public construction.

In order to do that, I’d like to have Spotbugs (or Findbugs, for our legacy codelines) issue a bug report when one of the following happens:

Is that possible? If so, how?

PS: I also think best practices are more of a style thing than a bug thing, but none of the style checkers such as CheckStyle that I have looked at could do this analysis.

mebigfatguy commented 6 years ago

You'd have to write your own plugin of course, but should be relatively trivial.

On 08/23/2018 06:01 AM, smillies wrote:

I'd like to enforce best practices for the use of the SafeDCLFactory from Alex Shipilev’s paper http://shipilev.net/blog/2014/safe-public-construction on safe public construction.

In order to do that, I’d like to have Spotbugs (or Findbugs, for our legacy codelines) issue a bug report when one of the following happens:

  • a class member of type |SafeDCLFactory| is not private and final
  • there is more than one read access to some member of type |SafeDCLFactory|

Is that possible? If so, how?

PS: I also think best practices are more of a style thing than a bug thing, but none of the style checkers such as CheckStyle that I have looked at could do this analysis.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/spotbugs/discuss/issues/54, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKYsS3qz3B3mGbX6A_aX493uNOi52D8ks5uTn19gaJpZM4WJOA6.

smillies commented 6 years ago

I feared as much :-( Trivial, eh? Is there a good tutorial, documentation on how to go about it? The official website just has a pointer to the Javadocs, and that's a bit thin for a complete beginner like myself.

mebigfatguy commented 6 years ago

Guides for making detectors:

https://www.ibm.com/developerworks/library/j-findbug2/
http://kjlubick.github.io/blog/post/1?building-my-first-findbugs-detector

Java bytecode

https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html
smillies commented 6 years ago

Thank you, that has been very helpful.

It almost works. I am still struggling with one thing, and I cannot find BCEL documentation that helps. Perhaps you can point me in the right direction. I am looking at invocations of the GET_INSTANCE_METHOD like this:

@Override
public void sawOpcode(int seen) {
    if (seen == Const.INVOKEVIRTUAL) {
        FQMethod fqm = new FQMethod(getClassConstantOperand(), getNameConstantOperand(), getSigConstantOperand());
        if (GET_INSTANCE_METHOD.equals(fqm)) {
            ...
        }
    }
}

What I need to do is to partition the method calls by the variables (fields) on which they occur. Do you know how I could do that? (or read up on it?)

mebigfatguy commented 6 years ago

i assume you are looking at fb-contrib as an example which is fine, if that's the case, take a look at uses of OpcodeStack. That basically holds all the things that get pushed onto the stack as the code is 'executed'. So when you get to a invokevirtual, the opcode stack will have all the parameters and also the 'this' object on which the object was called. So look at the signature of the method, get the number of args, and go down the stack that many to get the OpcodeStack.Item that is the 'this' object. You will be able to get the field name of that object from the item.

smillies commented 6 years ago

Thanks again, that worked out quite well! (Except for local variables, but that is a use case I don't really need to cover.)

I have a couple of questions about classpath and packaging, though. The User Guide is silent on this.

  1. When running the JUnit tests for my custom detector inside Eclipse, how do I set the auxClasspath to avoid warnings about missing application classes?
  2. As you have noticed, the plugin depends on external libraries, such as fb-contrib. How do I pull those in at runtime, e. g. when calling Spobugs through Ant or in the IDE plugin (Eclipse/IntelliJ)? I would have expected the Maven archetype to contain a suitable configuration to create a jar with such classpath entries in the manifest, or some such. I'm not really familar with this. What I finally did was use the Maven Shade plugin to create an uber.jar, but that blows up my detector plugin to about 6 MB.
mebigfatguy commented 6 years ago

1) Are you using a project file to define what you are scanning in the unit test?

2) That is a limitation of the spotbugs plugin system. The plugin itself can't have it's own classpath, it only can rely on things that spotbugs itself pulls in, or what's in it's own jar.

smillies commented 6 years ago

Thanks for the info. As for using a project file, I guess not. I just followed the Spotbugs manual to create a project, imported that project into Eclipse and ran the JUnit test with the standard run configuration created by Eclipse. Basically the only thing I changed in the template code was the name of the detector and the number of bugs expected in BadCase.

mebigfatguy commented 6 years ago

A long time ago i recommended to the defunct findbugs project that FindBugs read the Class-Path: attribute in the manifest of the plugin (if it exists) to find aux jars needed by the plugin. I don't think it ever was implemented. Now that i think of it, probably would be good for SpotBugs to do.

smillies commented 6 years ago

Yes, that would be a good thing. For now, I've decided to get rid of the fb-contrib dependency by simply copying the FQMethod code to my detector. The missing auxClasspath for my application files in the Eclipse runtime config is only a very minor nuisance, because I'll always be using spotbugs from Ant.

Thanks for your support!

smillies commented 6 years ago

Oops, I've downloaded newest fb-contrib-7.4.3.jar and am using it with Spotbugs 3.1.6. I'm getting stacktraces about incompatible BCEL versions:

[spotbugs] Exception analyzing com.idsscheer.ppm.server.ws.impl.ZWSServerFavoriteResolver using detector com.mebigfatguy.fbcontrib.detect.OverlyPermissiveMethod [spotbugs] java.lang.RuntimeException: Incompatible bcel version, apparently bcel has been upgraded to not use 'Unknown' for 'BootstrapMethods', but uses: BootstrapMethods [spotbugs] At com.mebigfatguy.fbcontrib.detect.OverlyPermissiveMethod.getBootstrapMethod(OverlyPermissiveMethod.java:451) ... [spotbugs] Exception analyzing com.idsscheer.ppm.server.ws.impl.ZWSServerFavoriteResolver using detector com.mebigfatguy.fbcontrib.detect.FunctionalInterfaceIssues [spotbugs] java.lang.ClassCastException: org.apache.bcel.classfile.BootstrapMethods cannot be cast to org.apache.bcel.classfile.Unknown [spotbugs] At com.mebigfatguy.fbcontrib.detect.FunctionalInterfaceIssues.getMethodHandle(FunctionalInterfaceIssues.java:345) ...

What is the latest pair of compatible versions? Is there a compatibility matrix somewhere?

(Addendum: fb-contrib-6.8.0 seems to work, which is the version currently included in the IntelliJ IDEA FindBugs plugin.)

mebigfatguy commented 6 years ago

Use a version with .sb in it, like 7.4.3.sb

smillies commented 6 years ago

Thanks. I have another feature idea: It seems that it is possible to omit visitors only based on their class names, at least in the Ant task, which is impractical. I only want correctness warnings, and would like to exclude visitors by category. Putting the exclusion in the filter file does not prevent those zillions of unwanted detectors from running, which significantly adds to the cost of a check (my code base has >10.000 classes).