spotify / missinglink

Build time tool for detecting link problems in java projects
Apache License 2.0
146 stars 27 forks source link

add explicitly loaded classes to reachability analysis #27

Closed pettermahlen closed 9 years ago

pettermahlen commented 9 years ago

Fixes #25

pettermahlen commented 9 years ago

@mattnworb @spkrka what do you think?

mattnworb commented 9 years ago

I'm confused - is the ldc instruction similar to a class implementing an interface or extending a superclass?

pettermahlen commented 9 years ago

The relevant part of the specification is here: http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.ldc. Basically, if ldc is invoked with a symbolic reference to a class, that class is resolved. This PR ensures that missinglink can detect failures to resolve classes that way. An example in code of how that happens is shown in #25, and in the system test that is added by the second commit. missinglink 0.1.1 fails to find the error in that test, but with this PR, it does find it.

mattnworb commented 9 years ago

Thanks for the explanation. (I noticed for some reason Github doesn't send emails to notify me of new issues or PRs until you mention me , that is annoying). This is a good catch of something we missed.

I'm not sure though if I agree or understand the change in ConflictChecker that is a result of treating "parent classes" and "loaded classes" as the same thing - that bit of code is trying to find which class defines a method that is called on SomeSubClass and walks the inheritance chain of that class. But a class loaded by ldc is not necessarily a superclass of the class, is it? This seems like it might introduce false negatives where previously we were detecting a conflict - for example:

It seems like ConflictChecker would no longer detect this. Or am I misunderstanding something?

pettermahlen commented 9 years ago

I think you're on to something, I didn't notice that the parents were traversed looking for method declarations, so I thought the concept was more general. Good point, I'll fix!

pettermahlen commented 9 years ago

I've 'fixed it a bit', but would like to add some more tests around this feature before merging. Not sure when I will be able to find the time for that, though.

pettermahlen commented 9 years ago

@mattnworb @spkrka how does this look? I'm personally unhappy with how hard it is to test in particular the ConflictChecker and ClassLoader. I'd like to do some refactoring or add some testing harness to simplify that at some point.

mattnworb commented 9 years ago

Separating out parents and loadedClasses looks good to me.

Regarding testing, I wonder if at a certain point it would be easier to generate classfiles by hand or some other process other than javac to get the sort of "invalid" instructions we want to exit in the test.