Issue #13672: Kill mutation for FileImportControl

[Kevin222004]

Issue #13672: Kill mutation for FileImportControl

Mutation

checkstyle/config/pitest-suppressions/pitest-imports-suppressions.xml

Lines 12 to 19 in ccc12eb

	<mutation unstable="false">
	<sourceFile>FileImportControl.java</sourceFile>
	<mutatedClass>com.puppycrawl.tools.checkstyle.checks.imports.FileImportControl</mutatedClass>
	<mutatedMethod>&lt;init&gt;</mutatedMethod>
	<mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.ArgumentPropagationMutator</mutator>
	<description>replaced call to com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl::encloseInGroup with argument</description>
	<lineContent>this.name = encloseInGroup(name);</lineContent>
	</mutation>

Explaination

This constructor

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Lines 44 to 49 in ccc12eb

	/* package */ FileImportControl(PkgImportControl parent, String name, boolean regex) {
	super(parent, MismatchStrategy.DELEGATE_TO_PARENT);
	this.regex = regex;
	if (regex) {
	this.name = encloseInGroup(name);

is used called only at line 185

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlLoader.java

Lines 181 to 186 in ccc12eb

	else if (FILE_ELEMENT_NAME.equals(qName)) {
	final String name = safeGet(attributes, NAME_ATTRIBUTE_NAME);
	final boolean regex = containsRegexAttribute(attributes);
	final PkgImportControl parentImportControl = (PkgImportControl) stack.peek();
	final AbstractImportControl importControl = new FileImportControl(parentImportControl,
	name, regex);

so this elseif statment will execute in case

 <file name="InputImportControl.*" regex="true">
    <disallow class="java.awt.Image"/>
  </file>

where we have file know encloseInGroup is used to non-capture the group I have done some research at
https://stackoverflow.com/questions/3512471/what-is-a-non-capturing-group-in-regular-expressions

[Kevin222004]

In explanation, I have only mentioned where it is used. And tried to understand the significance of it but i don't understand why the reason to do this. Please let me know what value this will create

[romani]

hmm, example of "file" tag https://checkstyle.org/checks/imports/importcontrol.html#ImportControl
search <file name=".*(Panel|View|Dialog)" regex="true">

this commit when it was added and example:
e018b2d#diff-b5d1e8bded0094bbf387cf531f3785057bf87530c8f73a637214fecf1a30a50cR170

So pitest does not see coverage for "non-capturing group"

https://stackoverflow.com/a/3513858/1015848

But we put whole regexp in non captured, this strange as it iliminates a purpose of it, as we need sibling group to make meaningful.

[romani]

@rnveach , do you remember why you used non captured group as wrapper?

I don't see that wrapping non-captured whole content make all nested groups as non-captured
https://regex101.com/r/aL1e8T/1 , see details panel.

[romani]

pitest blocks failed due to crash of pitest execution.

imports pitest has:

 <suppressedMutations>
+  <mutation unstable="false">
+    <sourceFile>FileImportControl.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.checks.imports.FileImportControl</mutatedClass>
+    <mutatedMethod>&lt;init&gt;</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_IF</mutator>
+    <description>removed conditional - replaced equality check with true</description>
+    <lineContent>if (regex) {</lineContent>
+  </mutation>

we need test where <file name="Dialog.java" regex="false">, this is highly unlikely users will use as hardcoding absolute path is very non-portable solution. if there will be problems to kill mutation by such test, I am ok with pure unit test for this edge case.

[rnveach]

@rnveach , do you remember why you used non captured group as wrapper?

What code are you talking about? I am not following.

[romani]

@rnveach , This code.

If we don't remember why we did this, we will remove it

[rnveach]

The comment above its call says ensure that fullPackage is a self-contained regular expression.

You had to search a bit more in the commit you found.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/PkgImportControl.java#L155-L156 was renamed from ImportControl, and then from PkgControl before that, and that is where the code originally came from. I did not add it.

It appears the code really came from 63e8c1d#diff-f68bc4c425df6a25439881dbcd97576b489f7d71fc7706a450e11a101f0d3e77R148-R149 .
#2999

Some things of interest:
#3438 (comment)
#3438 (comment)
#3438 (comment)

[romani]

Ok, thanks a lot, after reading all of this, there is no special reason of such code.

Last reason could be performance, as regexp does not need do creation of group. But as we wrap whole regexp it is does not guarantee that all nested groups will be downgraded to non-captured groups.

I am ok to remove this method.

[rnveach]

@romani

(?:.*(Panel|View|Dialog))

What happens when the regexp changes to .*(Panel|View|Dialog)? Since this now a capturing group, will it cause any issues?

[romani]

If no code deals with group, no changes.
We do not use group, we just match, true or false.

[rnveach]

@romani To me the following lines explain why we need encloseInGroup.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/PkgImportControl.java

Lines 128 to 130 in 4d113f9

	* "org.uncommon"}. Without the grouping it would be {@code "com|org.common|uncommon"} which
	* would match {@code "com"}, {@code "org.common"}, and {@code "uncommon"}, which clearly is
	* undesirable. Adding the group fixes this.

Apparently we concat 2 regexps together and without the enclosing () we would create a very different and wrong regexp than with it. We can't rely on the users adding () themselves and we enclose it in a () in case they don't.

Unless you are saying the javadoc is out of date, and we would need proof of that.

Combination code is at:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/PkgImportControl.java

Lines 98 to 101 in 4d113f9

	final String parentRegex = ensureSelfContainedRegex(parent.fullPackageName,
	parent.regex);
	final String thisRegex = ensureSelfContainedRegex(subPackageName, regex);
	fullPackageName = parentRegex + DOT_REGEX + thisRegex;

Edit: I was concentrating too much on PkgImportControl.

[rnveach]

Since this PR is only for FileImportControl, it doesn't do any combination of 2 regexps. The field it assigns is private, so we won't be passing this field to anywhere outside this class.

So yes, it is ok that we remove this.

[rnveach]

If no code deals with group, no changes.
We do not use group

Or if we do not combine 2 regexps.

[romani]

@Kevin222004 , please investigate. Looks like we found a way to make it to work not as expected if we remove such code.

[rnveach]

@romani #13760 (comment) . I also approved this PR as I think the code removal is fine.

[romani]

Ok to merge

[romani]

@Kevin222004 , please resolve pitest failure:
https://github.com/checkstyle/checkstyle/actions/runs/6266904570/job/17100922604?pr=13760

 <suppressedMutations>
+  <mutation unstable="false">
+    <sourceFile>FileImportControl.java</sourceFile>
+    <mutatedClass>com.puppycrawl.tools.checkstyle.checks.imports.FileImportControl</mutatedClass>
+    <mutatedMethod>&lt;init&gt;</mutatedMethod>
+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.RemoveConditionalMutator_EQUAL_IF</mutator>
+    <description>removed conditional - replaced equality check with true</description>
+    <lineContent>if (regex) {</lineContent>
+  </mutation>
+

Please prepare PR for merge.

[romani]

See reason above

[rnveach]

It seems to me we could remove name completely, and transfer everything to patternForExactMatch. We would just have to sanitize name that we put into patternForExactMatch if it is not regexp.

[romani]

@Kevin222004 , please resolve a conflict.

Let us know if you donot know how to resolve this survival, so mentors will look closer.

name is used at:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Line 89 in bbd110b

result = name.equals(fileName);

I do not think we can easily remove it.

to kill mutation on if regex, we need to have:
<allow class="java\..*\.File" regex="true"/>
and
<allow class="java\..*\.File" regex="false"/>

[Kevin222004]

This is the constructor. so after loading all the configuration and setup when visitToken in ImportControl

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheck.java

Line 237 in 83788d6

currentImportControl = root.locateFinest(packageName, fileName);

after executing this statement

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Line 80 in 83788d6

public AbstractImportControl locateFinest(String forPkg, String forFileName) {

after this method

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Lines 90 to 102 in 83788d6

	protected boolean matchesExactly(String pkg, String fileName) {
	final boolean result;
	if (fileName == null) {
	result = false;
	}
	else if (regex) {
	result = patternForExactMatch.matcher(fileName).matches();
	}
	else {
	result = name.equals(fileName);
	}
	return result;
	}

where we already check that if regex is true or not so that's why replacing this

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Lines 48 to 55 in 83788d6

	if (regex) {
	this.name = encloseInGroup(name);
	patternForExactMatch = createPatternForExactMatch(this.name);
	}
	else {
	this.name = name;
	patternForExactMatch = null;
	}

if as true will not make any affect.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Line 33 in 83788d6

private final Pattern patternForExactMatch;

The only usage of this field is at

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/FileImportControl.java

Lines 95 to 97 in 83788d6

	else if (regex) {
	result = patternForExactMatch.matcher(fileName).matches();
	}

so we can remove this field and directly initialize that in method it will not make any issue

[romani]

Items

[romani]

Ok to merge