New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Aug 6, 2018

Conversation

Projects
None yet
3 participants
@jsotuyod
Member

jsotuyod commented Jul 30, 2018

No description provided.

@jsotuyod jsotuyod added the a:bug label Jul 30, 2018

@jsotuyod jsotuyod added this to the 6.7.0 milestone Jul 30, 2018

@jsotuyod jsotuyod requested a review from oowekyala Jul 30, 2018

@oowekyala

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

This comment has been minimized.

Member

adangel commented Jul 30, 2018

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

This comment has been minimized.

Member

jsotuyod commented Jul 30, 2018

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

This comment has been minimized.

Member

adangel commented Jul 30, 2018

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

This comment has been minimized.

Member

jsotuyod commented Jul 30, 2018

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 referenced this pull request Aug 6, 2018

Open

[core] Render config errors #194

3 of 5 tasks complete

@adangel adangel changed the title from [java] Avoid NoClassDefFoundError with incomplete classpath to [java] MissingOverrideRule: Avoid NoClassDefFoundError with incomplete classpath Aug 6, 2018

@adangel

adangel approved these changes Aug 6, 2018

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

adangel added a commit that referenced this pull request Aug 6, 2018

@jsotuyod jsotuyod deleted the Monits:fix-noclassdef branch Aug 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment