Issue #14726: new check: ConstructorsDeclarationGroupingCheck #14749
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| /////////////////////////////////////////////////////////////////////////////////////////////// | ||
| // checkstyle: Checks Java source code and other text files for adherence to a set of rules. | ||
| // Copyright (C) 2001-2024 the original author or authors. | ||
| // | ||
| // This library is free software; you can redistribute it and/or | ||
| // modify it under the terms of the GNU Lesser General Public | ||
| // License as published by the Free Software Foundation; either | ||
| // version 2.1 of the License, or (at your option) any later version. | ||
| // | ||
| // This library is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| // Lesser General Public License for more details. | ||
| // | ||
| // You should have received a copy of the GNU Lesser General Public | ||
| // License along with this library; if not, write to the Free Software | ||
| // Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| /////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| package org.checkstyle.suppressionxpathfilter; | ||
|
|
||
| import java.io.File; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import com.puppycrawl.tools.checkstyle.DefaultConfiguration; | ||
| import com.puppycrawl.tools.checkstyle.checks.coding.ConstructorsDeclarationGroupingCheck; | ||
|
|
||
| public class XpathRegressionConstructorsDeclarationGroupingTest extends AbstractXpathTestSupport { | ||
|
|
||
| private final Class<ConstructorsDeclarationGroupingCheck> clazz = | ||
| ConstructorsDeclarationGroupingCheck.class; | ||
|
|
||
| @Override | ||
| protected String getCheckName() { | ||
| return clazz.getSimpleName(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testClass() throws Exception { | ||
| final File fileToProcess = new File( | ||
| getPath("InputXpathConstructorsDeclarationGroupingClass.java")); | ||
|
|
||
| final DefaultConfiguration moduleConfig = createModuleConfig(clazz); | ||
|
|
||
| final String[] expectedViolation = { | ||
| "10:5: " + getCheckMessage(clazz, | ||
| ConstructorsDeclarationGroupingCheck.MSG_KEY, 6), | ||
| }; | ||
|
|
||
| final List<String> expectedXpathQueries = Arrays.asList( | ||
| "/COMPILATION_UNIT/CLASS_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingClass']]" | ||
| + "/OBJBLOCK/CTOR_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingClass']]", | ||
| "/COMPILATION_UNIT/CLASS_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingClass']]" | ||
| + "/OBJBLOCK/CTOR_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingClass']]" | ||
| + "/MODIFIERS", | ||
| "/COMPILATION_UNIT/CLASS_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingClass']]" | ||
| + "/OBJBLOCK/CTOR_DEF/IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingClass']" | ||
|
|
||
| ); | ||
|
|
||
| runVerifications(moduleConfig, fileToProcess, expectedViolation, expectedXpathQueries); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEnum() throws Exception { | ||
| final File fileToProcess = new File( | ||
| getPath("InputXpathConstructorsDeclarationGroupingEnum.java")); | ||
|
|
||
| final DefaultConfiguration moduleConfig = createModuleConfig(clazz); | ||
|
|
||
| final String[] expectedViolation = { | ||
| "12:5: " + getCheckMessage(clazz, | ||
| ConstructorsDeclarationGroupingCheck.MSG_KEY, 8), | ||
| }; | ||
|
|
||
| final List<String> expectedXpathQueries = Arrays.asList( | ||
| "/COMPILATION_UNIT/ENUM_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingEnum']]" | ||
| + "/OBJBLOCK/CTOR_DEF" | ||
| + "[./IDENT[@text='InputXpathConstructorsDeclarationGroupingEnum']]", | ||
|
|
||
| "/COMPILATION_UNIT/ENUM_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingEnum']]" | ||
| + "/OBJBLOCK/CTOR_DEF" | ||
| + "[./IDENT[@text='InputXpathConstructorsDeclarationGroupingEnum']]" | ||
| + "/MODIFIERS", | ||
|
|
||
| "/COMPILATION_UNIT/ENUM_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingEnum']]" | ||
| + "/OBJBLOCK/CTOR_DEF/IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingEnum']" | ||
| ); | ||
|
|
||
| runVerifications(moduleConfig, fileToProcess, expectedViolation, expectedXpathQueries); | ||
| } | ||
|
|
||
| @Test | ||
| public void testRecords() throws Exception { | ||
| final File fileToProcess = new File( | ||
| getNonCompilablePath("InputXpathConstructorsDeclarationGroupingRecords.java")); | ||
|
|
||
| final DefaultConfiguration moduleConfig = createModuleConfig(clazz); | ||
|
|
||
| final String[] expectedViolation = { | ||
| "14:5: " + getCheckMessage(clazz, | ||
| ConstructorsDeclarationGroupingCheck.MSG_KEY, 8), | ||
| }; | ||
|
|
||
| final List<String> expectedXpathQueries = Arrays.asList( | ||
| "/COMPILATION_UNIT/CLASS_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingRecords']]" | ||
| + "/OBJBLOCK/RECORD_DEF[./IDENT[@text='MyRecord']]" | ||
| + "/OBJBLOCK/COMPACT_CTOR_DEF[./IDENT[@text='MyRecord']]", | ||
|
|
||
| "/COMPILATION_UNIT/CLASS_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingRecords']]" | ||
| + "/OBJBLOCK/RECORD_DEF[./IDENT[@text='MyRecord']]" | ||
| + "/OBJBLOCK/COMPACT_CTOR_DEF[./IDENT[@text='MyRecord']]/MODIFIERS", | ||
|
|
||
| "/COMPILATION_UNIT/CLASS_DEF[./IDENT" | ||
| + "[@text='InputXpathConstructorsDeclarationGroupingRecords']]" | ||
| + "/OBJBLOCK/RECORD_DEF[./IDENT[@text='MyRecord']]" | ||
| + "/OBJBLOCK/COMPACT_CTOR_DEF[./IDENT[@text='MyRecord']]" | ||
| + "/MODIFIERS/LITERAL_PUBLIC" | ||
| ); | ||
|
|
||
| runVerifications(moduleConfig, fileToProcess, expectedViolation, expectedXpathQueries); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //non-compiled with javac: Compilable with Java14 | ||
|
|
||
| package org.checkstyle.suppressionxpathfilter.constructorsdeclarationgrouping; | ||
|
|
||
| public class InputXpathConstructorsDeclarationGroupingRecords { | ||
| public record MyRecord(int x, int y) { | ||
|
|
||
| public MyRecord(int a) { | ||
| this(a,a); | ||
| } | ||
|
|
||
| void foo() {} | ||
|
|
||
| public MyRecord {} // warn | ||
|
|
||
| public MyRecord(int x, int y, int z) { | ||
| this(x+y, z); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package org.checkstyle.suppressionxpathfilter.constructorsdeclarationgrouping; | ||
|
|
||
| public class InputXpathConstructorsDeclarationGroupingClass { | ||
| InputXpathConstructorsDeclarationGroupingClass() {} | ||
|
|
||
| InputXpathConstructorsDeclarationGroupingClass(int a) {} | ||
|
|
||
| int x; | ||
|
|
||
| InputXpathConstructorsDeclarationGroupingClass(String str) {} // warn | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package org.checkstyle.suppressionxpathfilter.constructorsdeclarationgrouping; | ||
|
|
||
| public enum InputXpathConstructorsDeclarationGroupingEnum { | ||
| ONE; | ||
|
|
||
| InputXpathConstructorsDeclarationGroupingEnum() {} | ||
|
|
||
| InputXpathConstructorsDeclarationGroupingEnum(String str) {} | ||
|
|
||
| int f; | ||
|
|
||
| InputXpathConstructorsDeclarationGroupingEnum(int x) {} // warn | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /////////////////////////////////////////////////////////////////////////////////////////////// | ||
| // checkstyle: Checks Java source code and other text files for adherence to a set of rules. | ||
| // Copyright (C) 2001-2024 the original author or authors. | ||
| // | ||
| // This library is free software; you can redistribute it and/or | ||
| // modify it under the terms of the GNU Lesser General Public | ||
| // License as published by the Free Software Foundation; either | ||
| // version 2.1 of the License, or (at your option) any later version. | ||
| // | ||
| // This library is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| // Lesser General Public License for more details. | ||
| // | ||
| // You should have received a copy of the GNU Lesser General Public | ||
| // License along with this library; if not, write to the Free Software | ||
| // Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| /////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| package com.puppycrawl.tools.checkstyle.checks.coding; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import com.puppycrawl.tools.checkstyle.FileStatefulCheck; | ||
| import com.puppycrawl.tools.checkstyle.api.AbstractCheck; | ||
| import com.puppycrawl.tools.checkstyle.api.DetailAST; | ||
| import com.puppycrawl.tools.checkstyle.api.TokenTypes; | ||
|
|
||
| /** | ||
| * <p> | ||
| * Checks that all constructors are grouped together. | ||
| * If there is any code between the constructors | ||
| * then this check will give an error. | ||
| * Comments between constructors are ignored. | ||
| * </p> | ||
| * <p> | ||
| * Parent is {@code com.puppycrawl.tools.checkstyle.TreeWalker} | ||
| * </p> | ||
| * <p> | ||
| * Violation Message Keys: | ||
| * </p> | ||
| * <ul> | ||
| * <li> | ||
| * {@code constructors.declaration.grouping} | ||
| * </li> | ||
| * </ul> | ||
| * | ||
| * @since 10.16.0 | ||
| */ | ||
|
|
||
| @FileStatefulCheck | ||
| public class ConstructorsDeclarationGroupingCheck extends AbstractCheck { | ||
|
There was a problem hiding this comment. Can you make it stateless? There was a problem hiding this comment. Done. Firstly I tried making it stateless using other ways but the After adding the suppressions, Update: pitest coding-1 has still many surviving mutations. Should I suppress them? Because it is showing surviving mutators for those lines which are required to make the check work properly and mutating them would make test cases fail. There was a problem hiding this comment.
@Zopsss how is that? if the tests fail when hardcoding the mutation then how did they survive in the first place maybe you hardcode the mutation in the wrong way Update: Add the Test class to the TargetTests of the pitest-coding-1 profile in pom.xml There was a problem hiding this comment.
NVM it was showing that many surviving mutations because I didn't added the Test class in the TargetTests of pitest-coding-1. Thanks for helping me out. Now it is showing only 3 surviving mutations :) @romani Looking into the surviving mutations now 🫡 Update: all the surviving mutations has been killed. Now generating the diff report. There was a problem hiding this comment. I am "unresolving" this item to continue discussion. We should be able to make this check stateless by using all class definitions as required tokens (CLASS_DEF_, ENUM_DEF, etc...), then checking class members. This should allow us to make this check stateless, perhaps at the cost of a slightly larger memory footprint. @Zopsss @romani thoughts? There was a problem hiding this comment.
do you mean we should use an approach like this? The OverloadMethodsDeclarationOrderCheck uses this approach and it is stateless however the We're currently calling the Check for only We're also clearing the HashMap every time before calling the If we make the current implementation stateless then we cannot use this optimized approach. The check will take more time to execute. So I guess if we're able to make the Check optimized then we should be using the current approach. The Check implementation will also become complex if we use the OverloadMethodsDeclarationOrder's approach. Correct me if I'm wrong. There was a problem hiding this comment.
Can you prove this matters? There was a problem hiding this comment.
I gone through the page tried to understand how premature optimization should be avoided in some places if the optimization is not necessarily required. However I would like to give some light to the following paragraph:
In the last line, it says that premature optimization can introduce unexpected bugs and it can be a time waster. In our case the current implementation is bug free, easy to understand and does not contain complex code. The other approach you're suggesting might be difficult to implement and understand, it can also introduce new bugs which might take quite a time to solve. Here, Mr. Roman suggested that if we didn't find any way to make the Check stateless then it is okay to keep them stateful. I indeed agree that the Stateless approach you're suggesting will work but again it will be difficult to understand, might take some time to implement, might contain bugs and will perform lots of travelling in the Tree. But I'm not sure how much operating resources does a stateful check and a stateless check requires. I'm not sure how much they can affect on the performance of the Checkstyle. If you think I'm wrong somewhere, please let me know. I'm not experienced as you're, this is my first time having a conversation on optimizing the system performance. There was a problem hiding this comment. Sorry, I didn't read all this but just a few comments. We aren't multithreaded any time soon, so its not a big deal if we can't change the type of check here. Since we don't have an implementation of multithreading there is no telling how big a performance there may be doing either type. I don't see stateless being a big performance improvement. It just means we don't have to instantiate the check for each new file it processes. While new instances are cheap, it really depends on how we will do that instantiation and populate any property values which will decide which type of check is better for performance as well as the impact from the check not having any memory. There was a problem hiding this comment. After reading @rnveach's comment, do we still need to transition to the stateless check or is it okay to keep this check stateful? @nrmancuso @romani please tell me what do you think about this? |
||
|
|
||
| /** | ||
| * A key is pointing to the warning message text in "messages.properties" | ||
| * file. | ||
| */ | ||
| public static final String MSG_KEY = "constructors.declaration.grouping"; | ||
|
|
||
| /** | ||
| * Specifies different Object Blocks scope. | ||
| */ | ||
| private final Map<DetailAST, Integer> allObjBlocks = new HashMap<>(); | ||
|
There was a problem hiding this comment. This is a state. If you see no way to make it stateless, it is ok. You can just explain a reason on why stateless is not possible, and stay with stateful. There was a problem hiding this comment.
Done. Used the
I guess we can't make this check stateless because the check requires tracking the scope of the overloaded constructors, so we need a global variable to keep track of this. A local variable will not work in this situation. Here is the example of what I'm trying to say: |
||
|
|
||
| @Override | ||
| public int[] getDefaultTokens() { | ||
| return getRequiredTokens(); | ||
| } | ||
|
|
||
| @Override | ||
| public int[] getAcceptableTokens() { | ||
| return getRequiredTokens(); | ||
| } | ||
|
|
||
| @Override | ||
| public int[] getRequiredTokens() { | ||
| return new int[] { | ||
| TokenTypes.CTOR_DEF, | ||
| TokenTypes.COMPACT_CTOR_DEF, | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public void beginTree(DetailAST rootAst) { | ||
| allObjBlocks.clear(); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitToken(DetailAST ast) { | ||
| final DetailAST currObjBlock = ast.getParent(); | ||
| final Integer previousCtorLineNo = allObjBlocks.get(currObjBlock); | ||
|
There was a problem hiding this comment. Why we can not simply check previous sibling on each ctor? There was a problem hiding this comment. This will work in most of the situations. We can keep track if the current CTOR is first or not and check for its previous sibling, but if there is another block between the constructors then this approach will not work. For example: Here, the And we have provided the last occurred constructor's line number in the violation so user can know where the previous constructor is located. This is the very early implementation of this check: https://github.com/checkstyle/checkstyle/blob/feba1977f846e7cb4fbed8ee76239ca9ee7a5140/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java In this, I used a simple boolean variable to keep track of the previous CTOR, but this type of approach will fail for the example I mentioned above. Here is an example in the diff report: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7452e65_2024163113/reports/diff/spotbugs/index.html#A15 Our check would not able to detect this if we simply check the previous CTOR without tracking the block of the constructor. There was a problem hiding this comment. Looks like this approach might also mitigate the issue here. In this case we would always operate, statelessly, in the context of a single class. There was a problem hiding this comment. Yeah by using that approach we will be operating the check statelessly but that approach will be more time consuming, am I right? I've given my opinion here |
||
|
|
||
| if (previousCtorLineNo != null) { | ||
| final DetailAST previousSibling = ast.getPreviousSibling(); | ||
| final int siblingType = previousSibling.getType(); | ||
| final boolean isCtor = siblingType == TokenTypes.CTOR_DEF; | ||
| final boolean isCompactCtor = siblingType == TokenTypes.COMPACT_CTOR_DEF; | ||
|
|
||
| if (!isCtor && !isCompactCtor) { | ||
| log(ast, MSG_KEY, previousCtorLineNo); | ||
| } | ||
|
|
||
| allObjBlocks.put(currObjBlock, ast.getLineNo()); | ||
| } | ||
| else { | ||
| allObjBlocks.put(currObjBlock, ast.getLineNo()); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zopsss @romani it looks like we are emulating the behavior of OverloadMethodsDeclarationOrder where a user may need to run the check multiple times in order to make checkstyle happy. Is there some technical limitation that prevents us from using the following logic:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can do this, I agree with this. Waiting for @romani's opinion. If he agrees then I'll start working on the new implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nrmancuso @romani If this is the way to want to move, we should probably create an issue for the other check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nrmancuso I've changed the implementation according to your new idea. Should I push the new changes or wait for other maintainers approval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you’ve already done the work, you can checkout a new branch from what you’ve done, and open a draft PR. Then you can reset this branch from the origin. This way you have both versions preserved and we can see what the other looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Here is the draft PR: #14844
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new PR makes it even more obvious that we should do this, IMO.
This is my vision:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the implementation stateless on that PR, can you please check that out? and sorry for the late reply I wasn't active for few days