Generated by 🚫 Danger

@adangel Apparently danger fails on the latest commits because of a missing master baseline zip:

https://travis-ci.org/github/pmd/pmd/jobs/736633219#L7492-L7502

Actually the diff is weird because the file names are truncated in the PR diff?

Eg this section has just the file name and not its full path

https://chunk.io/pmd/038abe34ece14f87822db43bb27fd398/diff2/checkstyle/index.html#A14249

Compare to the baseline for the same file

https://chunk.io/pmd/038abe34ece14f87822db43bb27fd398/diff2/checkstyle/index.html#A1

@adangel Apparently danger fails on the latest commits because of a missing master baseline zip:

https://travis-ci.org/github/pmd/pmd/jobs/736633219#L7492-L7502

I guess, this was a temporary hickup downloading the baseline from sourceforge. The wget command didn't fail (if it did, we would have seen a error message there already), it downloaded something. But maybe the file was not complete or corrupt (due to mirroring on sourceforge hosts?).

I see, that the regression tester runs again...

Actually the diff is weird because the file names are truncated in the PR diff?

Only the first part of the path should be truncated (e.g. the working directory), so that it doesn't matter, where exactly you run the regression tester.

The baseline looks Ok: checkstyle/src/it/java/com/google/checkstyle/test/base/AbstractIndentationTestSupport.java
But pmd7 doesn't: AbstractIndentationTestSupport.java

I feel like I remember having fixed this already.... hm.... I think, this was the path of processing errors, which was wrong (269f5bd). Maybe that's something new? I wonder, from where it came....

Looking at the comparison between this PR and pmd/7 - there are only trucated files... there is something broken on pmd/7.0.x....

@oowekyala The new regression report is available. We have many removed violations compared to master:

Compared to master:
This changeset introduces 4862 new violations, 9 new errors and 0 new configuration errors,
removes 24436 violations, 16 errors and 2 configuration errors.
https://chunk.io/pmd/e39b418f45cf42b683c24592dd69dc28/diff2/index.html

One example is this:
In OuterTypeFilenameTest.java we should report the missing comment for the 3 test methods, but we apparently don't. The first method is annotated with @Override, so we don't require comment for "getPackageLocation()", which is Ok. Maybe that leads to skipping the check for the other methods?

I'll look into this....

Ok, so this is what happens:

We see the annotation @Test, figure out, it is @org.junit.Test, but junit is not on the auxclasspath. So, this classref is unresolved. When comparing to @java.lang.Override, we consider it by default a subtype:

This results then in a false negative. It seems, that we have on master a different default behavior for incomplete auxclasspaths. Returning here simply Convertibility.NEVER would solve this special case, but maybe it's not too bad, and we should leave it. It would give us an argument: "you need to call PMD correctly to get accurate results".

I'll continue now on pmd/pmd-regression-tester#48, so that we call PMD correctly during regression testing.

Updated numbers for comparing this PR against master for checkstyle is available: https://chunk.pmd-code.org/pmd/f465ddd3-2188-4976-9f61-f16591faea2c/diff/index.html
The numbers look much better now....

• 1076 new violations, 420 removed violations
• 7 new errors, 21 removed errors

Here's the preview for spring-framework comparing this PR against master: https://chunk.pmd-code.org/pmd/37f43255-f654-4295-a72b-76e83ef2a06e/diff/index.html

The numbers look much better here as well:

• 4296 new violations, 3458 removed violations
• 3 new errors, 22 removed errors

This results then in a false negative. It seems, that we have on master a different default behavior for incomplete auxclasspaths. Returning here simply Convertibility.NEVER would solve this special case, but maybe it's not too bad, and we should leave it. It would give us an argument: "you need to call PMD correctly to get accurate results".

Thanks for diagnosing this. I think isConvertible needs to stay that way for type inference to work properly (ie, be liberal when inferring unresolved types). But I changed TypeTestUtil to use a better strategy (ie, always return false if we're not sure, which is the behavior of the rest of TypeTestUtil)

Quick review of the regression report:

This fixes many FNs of CommentRequired

• Annotation methods
• Misplaced comment on field, previously the comment that is on the previous enum constant was attributed to this field. Same here
• Misplaced comment on classes
• Misplaced comment on import declaration, though I'm not so sure about this one

Line numbers changed a bit, eg this violation is reported on the method identifier and not the return type, this is expected.

A few files are shown as if all violations are removed: those are the files excluded by the exclude pattern, refs 331f8f9

But notably, CommentRequired still does not report on annotation type declarations, and record declarations.

I think globally this is very positive, this new comment assignment method works better than the old.