New Implementation for ConstructorsDeclarationGroupingCheck

[Zopsss]

Issue: #14726

Follow up of #14749

New implementation of the Check based on @nrmancuso's suggestions at: #14749 (comment)

The Check now flags all the constructors which got separated from the grouped constructors.

Here is the new error message:
https://github.com/Zopsss/checkstyle/blob/64e425b6c68fe9e03832b55bcff0e69b8bad1544/src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties#L6

Example:

private class Example {
  Example() {}

  Example(String str) {}

  int x;

  Example(int x) {} // violation

  String xx;

  Example(double d) {} // violation

  Example(float f) {} // violation
}

The error message will look something like this:

#1      : 8:2: All constructors should be grouped together. The constructors got separated at line '6'.
#2      : 12:2: All constructors should be grouped together. The constructors got separated at line '6'.
#3      : 14:2: All constructors should be grouped together. The constructors got separated at line '6'.

Here line '6' means this line: int x;, at this line the constructors' separation started.

The message will specify the line from which the constructors got separated. This will help users to locate the first line from where the separation started. If you have any suggestions about improving the error message then please let me know.

---

I've kept 2 separate commits, the first commit is the initial implementation of the Check, the second commit is the new changes in the implementation so I can easily copy these changes back to the main PR in future.

---

New module config: https://gist.githubusercontent.com/Zopsss/b3dabe65814aae54ff98c62f9fe54d3e/raw/6372644a5aa81acba09f1ddc607a79cd0b56de58/ConstructorsDeclarationGrouping.xml

Diff Regression projects: https://gist.githubusercontent.com/Zopsss/22adadb570e4deb95296917244c580b3/raw/29ea756eada8915637b76678db4f46048c198808/projects-to-test-on-for-github-action.properties

[Zopsss]

made the implementation stateless as you suggested @nrmancuso

[Zopsss]

I was facing this similar error in my other PR too:

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25422/workflows/f50f34a7-449f-4a16-b8e6-9f0bdd0206c6/jobs/585837?invite=true#step-103-33210_175

any idea how about how to solve it?

Edit: I'm facing the same error on master branch too

[Zopsss]

I'm not able to think any way to kill this surviving mutation: https://github.com/checkstyle/checkstyle/actions/runs/8950200747/job/24585310558?pr=14844#step:7:22

Here is the if statement which is causing this mutation:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java

Lines 88 to 90 in bc12147

	if (TokenUtil.isOfType(parentType, tokenTypes)) {
	checkConstructorsGrouping(ast);
	}

I added this code based on OverloadMethodsDeclarationCheck's implementation. After digging into this Check, I found that it killed this mutation by using the following code in its input file:

checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/overloadmethodsdeclarationorder/InputOverloadMethodsDeclarationOrder.java

Lines 155 to 167 in e6051f2

	enum Foo2 {
	VALUE {
	public void value() {
	value("");
	}
	public void middle() {
	}
	public void value(String s) {
	}
	};
	}

I tried using something similar to kill the mutation but it didn't seem to work.

I initially added that if statement inspired by the OverloadMethodsDeclarationCheck but it is causing a surviving mutation.

So should I remove that if statement and directly call the checkConstructorsGrouping()? It does not seem to affect the working of the check. But I guess it'll degrade the time complexity of the check. I'm not able to think any other way to kill this mutation.

I'm hesitating to remove that if statement because I think it'll degrade the time complexity of the check, can you please guide me here? @nrmancuso

[rnveach]

@Zopsss Have you tried https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression ?

[Zopsss]

@Zopsss Have you tried https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression ?

No I didn't. Thanks for the suggestion, I will try this :)

[Zopsss]

@Zopsss Have you tried https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression ?

No I didn't. Thanks for the suggestion, I will try this :)

Generating the diff report here cuz I'm not able to run the command provided in the docs properly:

groovy diff.groovy --localGitRepo /home/johndoe/projects/checkstyle \
  --baseBranch i111-my-fix --patchBranch i111-my-fix-mutation \
  --config config.xml --listOfProjects projects-to-test-on.properties

[Zopsss]

GitHub, generate report

[rnveach]

https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression
Next you must create a new branch off the PR branch which must be modified to embed the applied mutation into the code.

Unless you crash your current PR, you can't do this here as you haven't applied the mutation to the code.

If you are having issues running regression locally, then I suggest starting a discussion and provide us details to assist you.

[github-actions]

Initial Report:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/bc12147_2024195447/reports/diff/index.html

[Zopsss]

Unless you crash your current PR, you can't do this here as you haven't applied the mutation to the code.

Sorry I didn't saw your comment before... I pushed the changes with mutated code, I was thinking of re-generating the regression report with this mutated code and find if is there any difference between this mutated code report and the original code report....

If you are having issues running regression locally, then I suggest starting a discussion and provide us details to assist you.

Sure, here is the error I'm getting:

ERROR: No java.exe found at: C:\Program Files\Java\jdk-21\bin\bin\java.exe
Please set the JAVA_HOME variable in your environment
to match the location of your Java installation.

I'm using jdk 21, groovy 4 and windows 11.
I have made sure that the JAVA_HOME is defined perfectly, here its value: C:\Program Files\Java\jdk-21\bin

the error is showing java.exe not found at C:\Program Files\Java\jdk-21\bin\bin\java.exe, I'm not sure why it is showing \bin\bin when the JAVA_HOME has only one \bin in its value. I also tried using jdk-19 but the results were same.

The where groovy command is giving this result:

C:\Users\maury>where groovy
C:\Program Files (x86)\Groovy\bin\groovy
C:\Program Files (x86)\Groovy\bin\groovy.bat

This was my command to generate the regression report:

groovy diff.groovy --localGitRepo ./checkstyle --baseBranch new-impl-ctor --patchBranch new-impl-ctor-mutation --config config.xml --listOfProjects projects-to-test-on.properties

I modified this command and made it a single line otherwise it was giving me this error when I did not modified it and used the original one provided in the doc:

At line:2 char:5
+   --baseBranch new-impl-ctor --patchBranch new-impl-ctor-mutation \
+     ~
Missing expression after unary operator '--'.
At line:2 char:5
+   --baseBranch new-impl-ctor --patchBranch new-impl-ctor-mutation \
+     ~~~~~~~~~~
Unexpected token 'baseBranch' in expression or statement.
At line:3 char:5
+   --config config.xml --listOfProjects projects-to-test-on.properties
+     ~
Missing expression after unary operator '--'.
At line:3 char:5
+   --config config.xml --listOfProjects projects-to-test-on.properties
+     ~~~~~~
Unexpected token 'config' in expression or statement.
    + CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : MissingExpressionAfterOperator

[Zopsss]

GitHub, generate report

[Zopsss]

Re-generating the report with mutated code as mentioned here:

I was thinking of re-generating the regression report with this mutated code and find if is there any difference between this mutated code report and the original code report

Will also generate the report locally once the error is fixed. I thought it would be better to compare these 2 regression reports until the error I was getting locally is solved.

[github-actions]

Mutated Code Report:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/30250dd_2024213808/reports/diff/index.html

[rnveach]

I meant a GH discussion. Not in the PR thread.

Please set the JAVA_HOME variable in your environment
I have made sure that the JAVA_HOME is defined perfectly, here its value: C:\Program Files\Java\jdk-21\bin

Java home isn't the bin directory. It is the one above it.

[Zopsss]

I meant a GH discussion. Not in the PR thread.

Done. Here is the link: #14861.

Please set the JAVA_HOME variable in your environment
I have made sure that the JAVA_HOME is defined perfectly, here its value: C:\Program Files\Java\jdk-21\bin

Java home isn't the bin directory. It is the one above it.

Fixed. thanks for the help. I was able to run that command but was getting some error. I've mentioned that in the discussion linked above.

[Zopsss]

I analyzed both of these reports: #14844 (comment) #14844 (comment). One report was with the original code I pushed and other report was with the mutated code. They don't have any differences, they both have same violations. They both are same so there is no after affects of removing that if statement, the only bad side is that it'll increase the time complexity of the Check. So it is okay to remove that if statement. I also tried generating the regression report locally but I was facing some error which I mentioned in the above comment.

Should I still generate the regression report locally or can I just follow pitest and remove the mutated code? @rnveach

[rnveach]

Should I still generate the regression report locally or can I just follow pitest and remove the mutated code?

I would still like to see it to ensure it is being generated correctly.

To be clear. Base branch is suppose to be what you want merged in this PR. Patch branch is suppose to be base branch plus the mutation applied. The only differences that show up should be only related to the mutation, nothing related to what you want merged in this PR normally.

[nrmancuso]

I'm hesitating to remove that if statement because I think it'll degrade the time complexity of the check, can you please guide me here? @nrmancuso

I guided you at #14749 (comment). I can see why we might want to also include OBJBLOCK in the tokens for this check (anonymous classes), but we should filter out the OBJBLOCKs that are not in anonymous classes.

You will never pass pitest with the code at

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java

Lines 88 to 90 in bc12147

	if (TokenUtil.isOfType(parentType, tokenTypes)) {
	checkConstructorsGrouping(ast);
	}

, because the tokens that the if statement is guarding against don't have constructors anyway (INTERFACE_DEF).

Additionally, as I mentioned at #14749 (comment), we can clean the implementation in this PR up (simplify logic, remove mutable variables) by creating a list of child ASTs first.

[Zopsss]

I can see why we might want to also include OBJBLOCK in the tokens for this check (anonymous classes), but we should filter out the OBJBLOCKs that are not in anonymous classes.

Anonymous classes can't have constructors. Here is the oracle doc's reference:

https://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html

Accessing Local Variables of the Enclosing Scope, and Declaring and Accessing Members of the Anonymous Class section's last line:

However, you cannot declare constructors in an anonymous class.

---

Additionally, as I mentioned at #14749 (comment), we can clean the implementation in this PR up (simplify logic, remove mutable variables) by creating a list of child ASTs first.

list of child ASTs? Do you mean specifying the CLASS_DEF, ENUM_DEF, RECORD_DEF in the getRequiredTokens()?

Here is your full comment:

1. Create a list of all children of the class/enum/whatever def
2. Create sublist from first constructor , taking while the AST is a constructor
3. Take the difference of the original list and the sublist
4. Filter out non-constructors
5. log all remaining items

I understood the first point but didn't quite got the other points, I have made the changes according to first point:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java

Lines 70 to 77 in e91d31c

	@Override
	public int[] getRequiredTokens() {
	return new int[] {
	TokenTypes.CLASS_DEF,
	TokenTypes.ENUM_DEF,
	TokenTypes.RECORD_DEF,
	};
	}

Here is how the check works currently:

The check firstly gets the OBJBLOCK of the whatever DEF it is called for. Then checks if the current token is constructor or not, if it is then it checks if the previous sibling is CTOR or not, it checks if the current CTOR is first one to occur or not? If the current CTOR is not the first one to occur and the previous sibling is also not a CTOR then the isViolation is set to true and also firstNonCtorSiblingLineNo is set to the first non constructor sibling indicating the line from which the separation started.

If the isViolation is set to true then it means that we should log all the constructors which occurred after the first non constructor sibling saying that they're separated and not grouped together.

The error message indicates the line number from where the separation started:

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties

Line 6 in e91d31c

constructors.declaration.grouping=All constructors should be grouped together. The constructors got separated at line ''{0}''.

Example:

class Example {
  Example() {}

  Example(String str) {}

  int x;

  Example(int x) {} // violation

  String xx;

  Example(double d) {} // violation

  Example(float f) {} // violation
}

Error messages:

8:4: All constructors should be grouped together. The constructors got separated at line '6'.
12:4: All constructors should be grouped together. The constructors got separated at line '6'.
14:4: All constructors should be grouped together. The constructors got separated at line '6'.

Here the error message indicates line 6 which is int x; from where the separation started.

---

The implementation looks good to me, the time complexity might have got improved after adding the only required DEFs in the getRequiredToken(). The Check is stateless and as you requested ( #14749 (comment) ), it flags all the constructors after the separation has occurred. The user does not need to run Checkstyle multiple times, all the separated constructors will be logged and the separation line number will also be specified after running Checkstyle only once.

[Zopsss]

To be clear. Base branch is suppose to be what you want merged in this PR. Patch branch is suppose to be base branch plus the mutation applied. The only differences that show up should be only related to the mutation, nothing related to what you want merged in this PR normally.

I have changed the Check's implementation a little bit as mentioned here: #14844 (comment)

But I generated the diff report before that, both branches had same violations. Here is the report if you want to take a look at it: https://zopsss.github.io/Diff-Report/

The current implementation and the implementation I used in the patchBranch are little bit different but both of the implementations works in the same way, the new implementation is just little bit optimized.

Edit: I've excluded elasticsearch & openjdk17 in the .properties file. I was not able to clone them because of my unstable internet connection

[Zopsss]

GitHub, generate report

[github-actions]

Mutated Code Report:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/e91d31c_2024183902/reports/diff/index.html

[nrmancuso]

I am good to abandon the other PR and take this out of draft mode.

Items:

[Zopsss]

I am good to abandon the other PR and take this out of draft mode.

Okay I'll close that PR so. And can you please assign this issue to yourself?

[nrmancuso]

Item:

[Zopsss]

The javac11 is failing, I'm getting this error in my other PR too

[romani]

@Zopsss , please rebase, it is fixed in master

[nrmancuso]

Please try this:

[nrmancuso]

Can we map to DetailAST.class::cast to avoid checker issue?

[romani]

Please remove second commit or squash commits together to single commit.
I see that module becomes stateless, that is good , we will go this way for sure.

Initial implementation will be stored in old PR forever, no need to keep it here.