Skip to content
This repository has been archived by the owner on Oct 22, 2023. It is now read-only.

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

Open
smillies opened this issue Aug 23, 2018 · 13 comments

Comments

@smillies
Copy link

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:

  • 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.

@mebigfatguy
Copy link

mebigfatguy commented Aug 23, 2018 via email

@smillies
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link

mebigfatguy commented Aug 24, 2018

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
Copy link
Author

smillies commented Aug 25, 2018

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
Copy link

  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
Copy link
Author

smillies commented Aug 26, 2018

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
Copy link

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
Copy link
Author

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
Copy link
Author

smillies commented Aug 27, 2018

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
Copy link

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

@smillies
Copy link
Author

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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants