ClassNotFoundException and other unexpected errors should come with a trail allowing the user to figure out where they came from

[hakanai]

I have a problem where I am getting ClassNotFoundException from forbidden-apis where it is trying to find up a class which was apparently referred to from some other class.

[linux: MinimalTestingBuild] 2018-06-26 09:53:19 > de.thetaphi.forbiddenapis.ForbiddenApiException: Check for forbidden API calls failed: java.lang.ClassNotFoundException: com.acme.elasticsearch.HasElasticSearchId

Problem is, there are no references to that class in the module where the error occurred. So it's referred to be some class in another module which is then indirectly referred to from the module I'm building, and the project is way too big to figure out where it might be.

• The java compiler, and even error-prone, can't see the problem at all.
• FindBugs has similar problems, but it isn't telling me either, and we're abandoning it for other reasons.
• IDEA doesn't appear to have any tools to tell me all the classes which indirectly depend on another class, though I initially thought that analysing backwards dependencies might work.

It would be nice if the error would be caught and rethrown with additional context about which class/method/parameter is being processed when it occurs. That would point directly at the problem.

[uschindler]

Hi,
This problem is on my to-do list. The reason is annotations which are resolved.
I think there is already an issue, but I wanted to fix that to ignore missing annotations.
The compiler does not check all annotations and does not go into superclasses of those annotations.
For the case of classes it also tried to go up up to Object to check if any superclass or interface is forbidden. I can improve the error message to show the original symbol.
I am about to release a new version to fix the Gradle bug und make the warnings of missing classes more silent. I will include this Bugfix.

[uschindler]

Looks like this is not an annotation. So I am working on improving the error message...

[uschindler]

It looks like this is not easy to solve... The calls to class lookups are often very deep inside other calls which are recursive. I think the easiest is to record some "current method/state" on the top level class scanner and when you get a CNFE, we can add the state to the message. Not sure yet how this works, but it's hard - unfortunately.

[hakanai]

I think just having it on the class scanner would be a good step forwards, because it would at least narrow down the manual search to one class method. In some cases, that might be enough. I don't actually know whether it would be in our case because we weren't even able to find the reference. We theorised that it was somehow used indirectly via a parameter to a method in some class but weren't able to narrow it down any further than that.

I was able to capture a full stack:

Caused by: de.thetaphi.forbiddenapis.ForbiddenApiException: Check for forbidden API calls failed: java.lang.ClassNotFoundException: com.acme.elasticsearch.HasElasticSearchId
        at de.thetaphi.forbiddenapis.Checker.run(Checker.java:644)
        at de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis.checkForbidden(CheckForbiddenApis.java:614)
        at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:73)
        ... 119 more
Caused by: java.lang.ClassNotFoundException: com.acme.elasticsearch.HasElasticSearchId
        at de.thetaphi.forbiddenapis.Checker.getClassFromClassLoader(Checker.java:322)
        at de.thetaphi.forbiddenapis.Checker.lookupRelatedClass(Checker.java:334)
        at de.thetaphi.forbiddenapis.ClassScanner.checkClassDefinition(ClassScanner.java:152)
        at de.thetaphi.forbiddenapis.ClassScanner.checkClassDefinition(ClassScanner.java:153)
        at de.thetaphi.forbiddenapis.ClassScanner.checkType(ClassScanner.java:172)
        at de.thetaphi.forbiddenapis.ClassScanner.checkType(ClassScanner.java:183)
        at de.thetaphi.forbiddenapis.ClassScanner.checkDescriptor(ClassScanner.java:210)
        at de.thetaphi.forbiddenapis.ClassScanner$2.<init>(ClassScanner.java:324)
        at de.thetaphi.forbiddenapis.ClassScanner.visitMethod(ClassScanner.java:316)
        at de.thetaphi.forbiddenapis.asm.ClassReader.readMethod(ClassReader.java:1062)
        at de.thetaphi.forbiddenapis.asm.ClassReader.accept(ClassReader.java:631)
        at de.thetaphi.forbiddenapis.asm.ClassReader.accept(ClassReader.java:355)
        at de.thetaphi.forbiddenapis.Checker.checkClass(Checker.java:622)
        at de.thetaphi.forbiddenapis.Checker.run(Checker.java:639)
        ... 121 more

ClassScanner.java:183 is checking arguments for a method, and then ClassScanner.java:153 is checking the superclasses/interfaces of the class. So I know that there is something which implements the interface which is causing the problem, which then appears as a parameter in a method somewhere. :)

A full solution seems like it would really want a context object passed around where you'd push/pop the current thing being looked at, and then when an error occurs, it would report the whole contents of the context stack which would point directly at the issue. So this context would have to be created somewhere around visitMethod and then get passed around all the way into checkClassUse, which might touch a lot of methods.

[uschindler]

Hi, the state for pushing popping was my idea, too. We don't need to pass that through the whole method calls, it can be a field on ClassScanner.
I will think about implementing it. It requires lots of push/pop, but should work. The good thing is, it can also assign the groupid of current method, so the rewriting done at end for handling lambdas correctly can also be applied on errors: As lambdas are compiled to special methods "lambda$XXXX()" there is already some state machine which keeps track of invokedynamics for lambdas to record the method, so the lambda methods can be rewritten to show the defining method in the error message (instead of the synthetic name).

[hakanai]

You could totally make a system where the context object does get passed from method to method and pushing onto it actually creates a new immutable context, which would avoid ever having to pop, but would also create an immense amount of garbage which may or may not slow down the processing.

[uschindler]

I have a plan already, it's not too much work. There is not much push/pop required, as you don't need to go deep inside. It's enough to push/pop on every field and method declaration. You would also get the line number inside methods. At least you don't need to really push its enough to have a "shared" state object that gets modified - very easy. Its currently done like that for the lambdas, it just needs to be extended. I will work on that!

[uschindler]

I did not implement a whole state machine, but in the next version this will be fixed like this:

• The exception is reported together with the scanned class (previously it was catching the exception globally, which made a relationship to the currently scanned class impossible).
• The class not found case is annotated with the type that was originally loaded, so you can easily look it up in the source code and/or public APIs. This means that it preserves the original type name that it parses (e.g., the class referred or the descriptor's owner of a method call) while diving up into superclasses and interfaces.