Issue #734: New check 'Jsr305AnnotationsCheck'

[mbert]

This PR is a port of the existing independent JSR305CheckstylePlugin into the sevntu ecosystem.

Documentation and discussion can be found in issue #734.

[rnveach]

PMD failures should be fixed.
All sevntu checks should be used in contribution failure can be ignored for now.

Once PMD is fixed, we can start looking over the check.

[mbert]

@rnveach I have commented on that in #734. Could you please read and respond?

[mbert]

@rnveach continuing here as you proposed.

I have now moved the test input files to src/test/resources. There are now those PMD failures left:

[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:50 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:62 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:77 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:93 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:104 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:117 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:134 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:145 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:153 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:161 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:169 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:177 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().
[INFO] PMD Failure: com.github.sevntu.checkstyle.checks.coding.Jsr305AnnotationsCheckTest:191 Rule:JUnitTestsShouldIncludeAssert Priority:3 JUnit tests should include assert() or fail().

These warnings result from the construction used to run the check on input files:

    @Test
    public void testParameters() throws CheckstyleException {
        Jsr305AnnotionsTestUtil.check(
                new ExpectedWarning("InputJsr305AnnotationsCheckWithParameter.java", 37, 44),
                new ExpectedWarning("InputJsr305AnnotationsCheckWithParameter.java", 42, 45),
                new ExpectedWarning("InputJsr305AnnotationsCheckWithParameter.java", 42, 64),
                new ExpectedWarning("InputJsr305AnnotationsCheckWithParameter.java", 75, 41),
                new ExpectedWarning("InputJsr305AnnotationsCheckWithParameter.java", 81, 27),
                new ExpectedWarning("InputJsr305AnnotationsCheckWithParameter.java", 99, 35)
        );
    }

The Jsr305AnnotationsTestUtil contains all the org.junit.Assert. The original motivation had been to avoid unnecessary boilerplate code in the unit test.

How do you propose to deal with this?

[rnveach]

here's what's in the diff folder: diff.zip

Yes, thats the correct folder and output we are looking for.
As mentioned at https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#testing-sevntu-checks , you are running full difference regression and sevntu doesn't support it. Only use the options --patchBranch and --patchConfig and --mode single. There is an example of this on that same readme.
Please also uncomment all projects at https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties to make sure you generate a report for everything.

We will need execution of your Check on some projects that use Jsr305 annotations

This. If you know such a project, you can add it to projects-to-test-on.properties to produce a report for it. If you don't know how to, give me the location for the project and I will show you how to add it.

[rnveach]

I won't be reviewing your code until the CI is green. If you need any help on anything please just ask.

[coveralls]

Coverage increased (+0.2%) to 96.307% when pulling 4dec3b0 on mbert:issue-734 into 984c6fe on sevntu-checkstyle:master.

[rnveach]

@mbert I need time to look over your check. I have marked some minor things but they don't address the implementation of your check and it's properties.
I still ask if there is some way to make the check simplier to read as the single file is more than 1000 lines.

[mbert]

Please also uncomment all projects at https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties to make sure you generate a report for everything.

Done that. Here's the report: https://mbert.github.io/sevntu.checkstyle/reports/diff/index.html

This. If you know such a project, you can add it to projects-to-test-on.properties to produce a report for it. If you don't know how to, give me the location for the project and I will show you how to add it.

I don't know of any public project using the plugin. That's why I provided a zip archive with an example project. For your convenience, here it is again: example-webservice.zip

[rnveach]

For your convenience, here it is again: example-webservice.zip

Thanks I missed it before. Please run this through the report generation as I don't see it listed in https://mbert.github.io/sevntu.checkstyle/reports/diff/index.html . We want to see the results of your check on this project that uses the annotation to further validate your changes.

You should be able to add a line to the projects properties like: my_custom_project|local|/home/username/java/my_custom_project||
to run against this webservice. You would need to unzip it first.

Here's the report: https://mbert.github.io/sevntu.checkstyle/reports/diff/index.html

The report looks generated correctly. I will use it as I start to review your check.

[mbert]

Thanks I missed it before. Please run this through the report generation as I don't see it listed in https://mbert.github.io/sevntu.checkstyle/reports/diff/index.html . We want to see the results of your check on this project that uses the annotation to further validate your changes.

You should be able to add a line to the projects properties like: my_custom_project|local|/home/username/java/my_custom_project||
to run against this webservice. You would need to unzip it first.

Done. Here's the report: example-webservice-diff.zip

[mbert]

I won't be reviewing your code until the CI is green. If you need any help on anything please just ask.

Thank you, I am running into two issues here:

1. If I interpret Job 1712.6 correctly, it fails because a DTD cannot be loaded (see first screenshot below). This is not related to my PR. Obviously the DTD has been removed from that URL in the time between the last check and my change. What's your policy here? If you want, I can fix that bit in my PR, but it would probably be "cleaner" to file an issue and fix it separately?

2. Although I am using the commands from the travis.sh script, I am getting results different from travis on the code coverage. On my system, the coverage of NumericLiteralNeedsUnderscoreCheck$NumericType is sometimes OK and sometimes below 100% (at the moment it fails on my machine, see the second screenshot below). Conversely, my check passes with respective branch rates of 99% and 94% on my box as set in pom.xml, but it fails with 98.3% and 96.5% in Job 1712.5 on travis (see third screenshot below). Are such differences between different build environments something you experience from time to time? I have no problem adapting the limits in pom.xml, but to me this behaviour seems worrying.

[edit] Partly replying to myself here: in order to speed things up I have now made the changes that hopefully bring the CI build to "green".

First screenshot: missing DTD:

Second screenshot: output of mvn cobertura:check on my machine:

Third screenshot: code coverage on travis:

[mbert]

OK, so now we have one issue left in Job 1714.6:

There are two problems mentioned here:

1. The file checks-sevntu-error.xml does not seem to exist in this repo, hence nothing that I can fix.
2. The URL https://checkstyle.org/dtds/configuration_1_3.dtd is downloadable on both my workstation and my mobile phone. No idea what problem travis has with it.

Any idea?

[rnveach]

@mbert I am already started to review your PR.

The file checks-sevntu-error.xml does not seem to exist in this repo

Last CI item can't be fixed until your check is added to an external file in another repo, but adding it before this PR is finished will cause issues since it is technically a check that doesn't exist.
Sorry for the confusion on this.

The URL https://checkstyle.org/dtds/configuration_1_3.dtd is downloadable on both my workstation and my mobile phone

It is a false error in CI. I believe it is the command that we are using that requires something more for the DTD. We need to add an option to the command to ignore DTD to prevent this error from printing.

[mbert]

@romani moving your questions here.

Please recheck that you shared all files.
https://mbert.github.io/sevntu.checkstyle/reports/example-webservice-diff/example-webservice/xref/home/md/src/contribution/checkstyle-tester/repositories/example-webservice/src/main/java/org/example/webservice/service/impl/PingServiceImpl.java.html#L79 gives error.

I have rechecked. There isn't any such file generated under diff. I ran the tool with this command line:

groovy diff.groovy --localGitRepo /home/md/src/checkstyle --patchBranch master --patchConfig my_check.xml --listOfProjects example-webservice.properties --mode single

And my example-webservice.properties file looks like this (as proposed by @rnveach):

example-webservice|local|/home/md/src/example-webservice||

Please let me know if there's anything I need to change.

I noticed only one project, please run check on all projects that we have in config, it will not find violation but at we can be somehow sure that Check is not crashing.

See above: #742 (comment)

Do you have some open source projects in mind that use such annotations? Maybe this https://checkerframework.org ? Probably search at GitHub by annotation name can lead you some open source project

I tried some search the other day. The problem from my side is that all the code I know that uses these annotations is closed source. I can try again.

[romani]

you can find jsr305 annotations usage by smth like https://github.com/search?l=Java&q=ParametersAreNullableByDefault&type=Code

please squash all commit to single commit.

have rechecked. There isn't any such file generated under diff. I ran the tool with this command line

@rnveach , we need your assistance.

items to improve:

[romani]

Few more items:

[romani]

probably last items to improve:

[mbert]

Here's an extended Checkstyle Tester run result with 8 projects: https://mbert.github.io/sevntu-checkstyle/checkstyle-tester/index.html

[romani]

thanks a lot for your time to finish it.

Here's an extended Checkstyle Tester run result with 8 projects

I have problem to open source code from report
https://mbert.github.io/sevntu-checkstyle/checkstyle-tester/diff/example-webservice/xref/home/md/src/contribution/checkstyle-tester/repositories/example-webservice/src/main/java/org/example/webservice/service/impl/PingServiceImpl.java.html#L79
please fix.

please squash all new Check related commits in one (you can keep https update in separate commit of this PR).
I always do review from scratch as I never did it before, so accumulation of commits does not help me, if you squash all github still be able to show diff between you previous remote commit and current.

generated javadoc:

items to improve:

[romani]

item to improve:

[romani]

Please replace all cases of "protected" with private or public. Inner classes should be ready to simply moved to outside or we should treat them same as outside classes

[mbert]

Please replace all cases of "protected" with private or public. Inner classes should be ready to simply moved to outside or we should treat them same as outside classes

Done that for a number of occurrences.

Since the inner classes for handling the different kinds of definitions are based on abstract base classes, there are some cases of protected left: abstract methods and methods used by subclasses. Please check whether that's OK for you.

[romani]

@rnveach , plase finalize.
Last item to improve:

[romani]

@rnveach , I am done with review, please finish second review and merge this.

@mbert , thanks a lot for hard work in this PR.

[rnveach]

@mbert There are PMD failures. See https://travis-ci.org/sevntu-checkstyle/sevntu.checkstyle/jobs/538137903#L917 .
Please make a PR to change https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-sevntu-error.xml to add your check.

I will try to begin reviewing this PR.

[rnveach]

I have problem to open source code from report
https://mbert.github.io/sevntu-checkstyle/checkstyle-tester/diff/example-webservice/xref/home/md/src/contribution/checkstyle-tester/repositories/example-webservice/src/main/java/org/example/webservice/service/impl/PingServiceImpl.java.html#L79

This problem still exists. Once new items are done, report can be regenerated to fix this.

[mbert]

@mbert There are PMD failures. See https://travis-ci.org/sevntu-checkstyle/sevntu.checkstyle/jobs/538137903#L917 .

Yep, sorry, those slipped my attention.

It looks like the PMD settings conflict with the CheckStyle settings when it comes to calling constructors of private inner classes.

Consider line 491:

1. PMD complains if the constructor ClassJsr305Handler(ast) is private - https://pmd.github.io/pmd-6.15.0/pmd_rules_java_bestpractices.html#accessorclassgeneration .
2. If I make it public, CheckStyle will report a violation because the public modifier is "redundant".
3. If I make it default scope, then PMD will require an extra comment (and, really, default scope would not be correct here).
4. I had set this constructor to protected before which kept both, PMD and CheckStyle happy, but @romani asked me not to use protected here.

I have now set it to protected again as this seems the only solution that passes static code checking.

[rnveach]

@mbert I assume you missed the review items listed inside the "Load more..." section.

[romani]

@mbert , please resolve all other points from @rnveach .
I keep your PR in my TODO, I will reply soon, sorry for delay.

[mbert]

@mbert , please resolve all other points from @rnveach .
I keep your PR in my TODO, I will reply soon, sorry for delay.

I think I've done that. Is there anything I missed?

[romani]

If I make it public, CheckStyle will report a violation because the public modifier is "redundant".

checkstyle/checkstyle#6813

I have now set it to protected again as this seems the only solution that passes static code checking.

I am fine with this for now.
@rnveach , I am still ok to merge. Please finish your review.

[rnveach]

this is the last change, sorry for the delay.

[rnveach]

New check will have to be added to https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-sevntu-error.xml before we can merge this PR.

[mbert]

New check will have to be added to https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-sevntu-error.xml before we can merge this PR.

Do I need to open a PR for this? Or are you guys taking care of it?

[rnveach]

@mbert please open a PR for it.

[mbert]

@mbert please open a PR for it.

Done. See PR #380.

[romani]

@mbert , thank you very much for your huge effort to make it happen.