Skip to content

[java] MissingOverrideRule: Avoid NoClassDefFoundError with incomplete classpath#1267

Merged
adangel merged 1 commit into
pmd:masterfrom
jsotuyod:fix-noclassdef
Aug 6, 2018
Merged

[java] MissingOverrideRule: Avoid NoClassDefFoundError with incomplete classpath#1267
adangel merged 1 commit into
pmd:masterfrom
jsotuyod:fix-noclassdef

Conversation

@jsotuyod

Copy link
Copy Markdown
Member

No description provided.

@jsotuyod jsotuyod added the a:bug PMD crashes or fails to analyse a file. label Jul 30, 2018
@jsotuyod jsotuyod added this to the 6.7.0 milestone Jul 30, 2018
@jsotuyod jsotuyod requested a review from oowekyala July 30, 2018 00:03

@oowekyala oowekyala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok, but I wonder if the rule should not report a misconfiguration/ throw an exception in case of incomplete auxclasspath instead of going silent. The rule definitely needs a complete auxclasspath to run correctly

@adangel

adangel commented Jul 30, 2018

Copy link
Copy Markdown
Member

Yes, reporting the incomplete auxclasspath (or even the missing classes) would be nice.
I think, this would be part of #194

However, I did a quick try, and we could already add config errors like this:

RuleContext context = (RuleContext) data;
context.getReport().addConfigError(new ConfigurationError(this, "missing class/method.."));

It would be then for now just in this rule, that we would report this, but it would be a start...
With #194, we could add a convenience method in AbstractRule (similar to addViolation) and also add support for asserting such config errors in the test framework.
We probably need a different data structure though, in order to avoid reporting the same missing class multiple times...

@jsotuyod

Copy link
Copy Markdown
Member Author

Yes, that is part of #194.

The hard part however is telling the user which classes are missing, so they can easily fix it. For ClassNotFoundException that is very straightforward. For these exceptions however, we don't really know...

Also of note, we should avoid duplicate reports to avoid being noisy, so on this case we would have to know we reported the issue on a static and thread-safe manner (to avoid issues on multi-threaded analysis).

I feel that's way beyond the scope of this PR, and definitely part of a larger task as described by #194

@adangel

adangel commented Jul 30, 2018

Copy link
Copy Markdown
Member

Yes, the "full" solution will definitely be #194 with a separate PR.
Do you think, just adding a "log.warn" with "incomplete auxclasspath" would be too noisy already (since we don't have de-duplication...)?
For detecting the actual missing class - in this concrete example, the missing/invalid classes are related to exploredType - so we could report this class and the exception message (which hopefully gives a hint, which class exactly was missing).

@jsotuyod

Copy link
Copy Markdown
Member Author

Considering how we systematically find people complaining about warn logs they can't get rid of (Gradle users mostly), I'd be hesitant on doing that...

We would have to be very careful with the wording... the missing class is related to exploredType (used in one of the declared methods in this case), but it's definitely not exploredType itself...

@jsotuyod jsotuyod mentioned this pull request Aug 6, 2018
5 tasks
@adangel adangel changed the title [java] Avoid NoClassDefFoundError with incomplete classpath [java] MissingOverrideRule: Avoid NoClassDefFoundError with incomplete classpath Aug 6, 2018

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this now. I've added this case to #194, that we report the missing classes, once we are able to.

@adangel adangel merged commit c7ac7cc into pmd:master Aug 6, 2018
adangel added a commit that referenced this pull request Aug 6, 2018
@jsotuyod jsotuyod deleted the fix-noclassdef branch August 6, 2018 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:bug PMD crashes or fails to analyse a file.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants