Issue #17158: fix invalid javadoc position for compact constructor by mohitsatr · Pull Request #17249 · checkstyle/checkstyle

mohitsatr · 2025-06-20T10:55:05Z

mohitsatr · 2025-06-20T11:42:47Z

@romani I suppose these tests are old and I must write tests as we write them now for branch coverage purposes right?

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/utils/BlockCommentPositionTest.java

Lines 86 to 92 in 0b5361c

    
           @Test 
        
           public void testJavaDocsRecognitionNonCompilable() throws Exception { 
        
               final List<BlockCommentPositionTestMetadata> metadataList = Arrays.asList( 
        
                   new BlockCommentPositionTestMetadata("InputBlockCommentPositionOnRecord.java", 
        
                       BlockCommentPosition::isOnRecord, 3), 
        
                   new BlockCommentPositionTestMetadata("InputBlockCommentPositionOnCompactCtor.java", 
        
                       BlockCommentPosition::isOnCompactConstructor, 3)

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/utils/BlockCommentPositionTest.java

Lines 95 to 102 in 0b5361c

    
           for (BlockCommentPositionTestMetadata metadata : metadataList) { 
        
               final DetailAST ast = JavaParser.parseFile( 
        
                   new File(getPath(metadata.getFileName())), 
        
                       JavaParser.Options.WITH_COMMENTS); 
        
               final int matches = getJavadocsCount(ast, metadata.getAssertion()); 
        
               assertWithMessage("Invalid javadoc count") 
        
                       .that(matches) 
        
                       .isEqualTo(metadata.getMatchesNum());

romani · 2025-06-22T01:19:27Z

yes, we need coverage for new code in this PR thought Inputs and real Check usage.

pure junit testing as you see it, does not contribute much to quality, as they migh be unrealistic and just test code for main code. One day in future we will convert all such pure unit testing to ent-to-end (Input based testing)

mohitsatr · 2025-07-01T12:31:28Z

Github, generate report for InvalidJavadocPosition/all-examples-in-one

Zopsss · 2025-07-01T13:03:48Z

Just to be on the safe side, please also add this input case so we can ensure that no new false positive is introduced:

PS: If you use a canonical constructor instead of a compact constructor, the error goes away. Similarly, if you use public accessibility instead of package-protected, the error goes away.

github-actions · 2025-07-01T13:30:05Z

Report for InvalidJavadocPosition/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/16925546f4fbf145908c4961ad9102dc525abef4_2025132904/reports/diff/index.html

romani

items:

mohitsatr · 2025-07-07T13:01:04Z

Github, generate report for InvalidJavadocPosition/all-examples-in-one

github-actions · 2025-07-07T13:39:41Z

Report for InvalidJavadocPosition/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/eeebf7b20b6287534a1b23561d9d534ff34afbb8_2025133926/reports/diff/index.html

romani

Items

mohitsatr · 2025-07-14T07:02:22Z

Github, generate report for InvalidJavadocPosition/all-examples-in-one

github-actions · 2025-07-14T07:41:12Z

Report for InvalidJavadocPosition/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a57a335f5762ac2d81048f72d2cd3eb1afc51231_2025074056/reports/diff/index.html

Zopsss

please add below suggested cases to the Check's input files too.

Zopsss · 2025-07-20T16:57:56Z

+    Mapping(String from, String to) {
+      this.from = from;
+      this.to = to;
+    }


please also add a canonical constructor with public modifier

Zopsss · 2025-07-20T16:59:42Z

+    @SizeType(max = 1)
+    BindMount {}


please also use public keyword with compact constructor. Similarly for canonical constructor with annotation, create two cases - one with public modifier and other without any modifier.

Zopsss · 2025-07-20T17:01:54Z

can you also create a top-level record class? it is not necessary but it is always good to have all the cases covered

Zopsss · 2025-07-20T17:02:34Z

@mohitsatr let's resume working on this PR. It is almost ready to merge, only few changes are required :)

romani

last test extenstion

romani

Ok to merge

Zopsss · 2025-07-22T05:20:27Z

+  public record Mapping2(String to) {
+
+    /**
+     * The constructor for Mapping.
+     *
+     * @param to The source
+     */
+    @SizeType(max = 3)
+    public Mapping2(String to) {
+      this.to = to;
+    }
+  }


The input file is for compact constructors but we have used canonical constructor here. I would suggest to split examples into 2 files - one for compact constructors and second for canonical constructor.

Sorry for asking this but it will make the inputs more manageable and it will also simplify the process of adding more cases in future for other contributors.

Sorry for asking this but it will make the inputs more manageable

no it does make sense.

…or compact constructor

mohitsatr mentioned this pull request Jun 26, 2025

supplemental: adding inputs for 4.5.1 where to break for record-class #17189

Merged

mohitsatr force-pushed the invalidjavadocpos-record branch 2 times, most recently from 428a453 to 1692554 Compare July 1, 2025 12:19

Zopsss suggested changes Jul 1, 2025

View reviewed changes

mohitsatr force-pushed the invalidjavadocpos-record branch from 1692554 to b4fcf7a Compare July 2, 2025 07:41

romani requested changes Jul 6, 2025

View reviewed changes

Comment thread src/it/java/com/google/checkstyle/test/chapter7javadoc/rule711generalform/GeneralFormTest.java Outdated

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/utils/BlockCommentPosition.java Outdated

mohitsatr force-pushed the invalidjavadocpos-record branch 2 times, most recently from 412876d to eeebf7b Compare July 7, 2025 11:26

romani requested changes Jul 12, 2025

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/utils/BlockCommentPosition.java Outdated

mohitsatr force-pushed the invalidjavadocpos-record branch from eeebf7b to a57a335 Compare July 14, 2025 04:57

Zopsss suggested changes Jul 20, 2025

View reviewed changes

Zopsss reviewed Jul 20, 2025

View reviewed changes

mohitsatr force-pushed the invalidjavadocpos-record branch from a57a335 to 4d44910 Compare July 21, 2025 09:05

romani requested changes Jul 21, 2025

View reviewed changes

Comment thread .../checks/javadoc/invalidjavadocposition/InputInvalidJavadocPositionOnCompactConstructors.java

mohitsatr force-pushed the invalidjavadocpos-record branch from 4d44910 to 3bd4c48 Compare July 21, 2025 13:51

romani approved these changes Jul 21, 2025

View reviewed changes

Zopsss reviewed Jul 22, 2025

View reviewed changes

mohitsatr force-pushed the invalidjavadocpos-record branch from 3bd4c48 to c48f2e5 Compare July 24, 2025 04:33

Issue checkstyle#17158: fix false-positive invalid javadoc position f…

7781f15

…or compact constructor

mohitsatr force-pushed the invalidjavadocpos-record branch from c48f2e5 to 7781f15 Compare July 24, 2025 05:16

Zopsss approved these changes Jul 26, 2025

View reviewed changes

romani merged commit 34a0073 into checkstyle:master Jul 27, 2025
118 checks passed

mohitsatr deleted the invalidjavadocpos-record branch July 27, 2025 03:13

Uh oh!

Conversation

mohitsatr commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohitsatr commented Jun 20, 2025

Uh oh!

romani commented Jun 22, 2025

Uh oh!

mohitsatr commented Jul 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 1, 2025

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mohitsatr commented Jul 7, 2025

Uh oh!

github-actions Bot commented Jul 7, 2025

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mohitsatr commented Jul 14, 2025

Uh oh!

github-actions Bot commented Jul 14, 2025

Uh oh!

Zopsss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zopsss commented Jul 20, 2025

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mohitsatr commented Jun 20, 2025 •

edited

Loading