Issue #13328: Kill mutation for UnusedLocalVariableCheck 8 by Kevin222004 · Pull Request #13151 · checkstyle/checkstyle

Kevin222004 · 2023-06-03T20:15:38Z

Issue #13328: Kill mutation for UnusedLocalVariableCheck 8

Check :-

https://checkstyle.org/checks/coding/unusedlocalvariable.html#UnusedLocalVariable

Mutation

checkstyle/config/pitest-suppressions/pitest-coding-2-suppressions.xml

Lines 75 to 82 in 554a87e

    
           <mutation unstable="false"> 
        
             <sourceFile>UnusedLocalVariableCheck.java</sourceFile> 
        
             <mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.UnusedLocalVariableCheck$TypeDeclDesc</mutatedClass> 
        
             <mutatedMethod>getUpdatedCopyOfVarStack</mutatedMethod> 
        
             <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.NakedReceiverMutator</mutator> 
        
             <description>replaced call to com/puppycrawl/tools/checkstyle/api/DetailAST::getLastChild with receiver</description> 
        
             <lineContent>final DetailAST updatedScope = literalNewAst.getLastChild();</lineContent> 
        
           </mutation>

Explaination

the updated scope is changes as only literalNewAst Actually it is not affecting any thing if we make it as literalNewast or objBlock as its last child

Regression :-

Diff Regression config: https://gist.githubusercontent.com/Kevin222004/0dbdfb487f8e9a050ed0e6356f1a35b4/raw/31f7927f400e11e4285e86fd086b373095227848/unusedLocal.xml
Diff Regression projects: https://gist.githubusercontent.com/Kevin222004/21e3934e85f802e2fbd48af06d122364/raw/604256badd733d8568064f371d55657c04b00dfd/test-projects-2.properties
Report label: Final-Regression-1

Kevin222004 · 2023-06-03T20:15:53Z

Github, generate report

github-actions · 2023-06-03T22:11:53Z

Regression-1:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/34c1110_2023221043/reports/diff/index.html

Kevin222004 · 2023-07-01T03:28:34Z

Github, generate report

romani

https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LITERAL_NEW
Strange that we used last child, not clear why

Items

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

romani

Ok to merge if reports are clean

Kevin222004 · 2023-07-01T05:34:02Z

Please ignore survivng mutation it will be covering at #13331

github-actions · 2023-07-01T05:57:52Z

Final-Regression-1:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/47f994b_2023053216/reports/diff/index.html

rdiachenko

lgtm

Vyom-Yadav · 2023-07-02T08:21:14Z

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

-            final DetailAST updatedScope = literalNewAst.getLastChild();
            final Deque<VariableDesc> instAndClassVarDeque = new ArrayDeque<>();
            instanceAndClassVarStack.forEach(instVar -> {
                final VariableDesc variableDesc = new VariableDesc(instVar.getName(),
-                        instVar.getTypeAst(), updatedScope);
+                        instVar.getTypeAst(), literalNewAst);


I am not ok with pitest complaining here. The logic is correct but it breaks the integrity of the check. We use last-child because we have a set definition of scope.

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

Lines 152 to 159 in 3142055

/**

* An array of scope tokens.

*/

private static final int[] SCOPES = {

TokenTypes.SLIST,

TokenTypes.LITERAL_FOR,

TokenTypes.OBJBLOCK,

};

The check by design uses { or for for determining scope. Extending this to literalNew is error-prone as it serves no value. The check is designed to have only the above-mentioned token as scope and this change can make extending this check difficult because one more case has to be handled.

I vote for suppressing this pitest mutation.

@romani @rdiachenko Please share your thoughts.

If this value does not matter in current context, let's put null there or use some ctor without this value.

The value of scope matters, it is used to pop out the variables at the right time (from the proper scope). I am saying this should stay:

final DetailAST updatedScope = literalNewAst.getLastChild();

And pitest should be suppressed in this case. This is an intentional check design otherwise every other parent-level token can be considered as scope but we have limited it due to intentional design and ease of maintenance.

I do not see this.

there are only 2 usages of this class with scope defined in constructor: modified case and case

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

Lines 494 to 495 in d4b1aec

final VariableDesc desc = new VariableDesc(ident.getText(),

varDefAst.findFirstToken(TokenTypes.TYPE), findScopeOfVariable(varDefAst));

but this method also use parent, not a last child:

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

Lines 764 to 773 in d4b1aec

private static DetailAST findScopeOfVariable(DetailAST variableDef) {

final DetailAST result;

final DetailAST parentAst = variableDef.getParent();

if (TokenUtil.isOfType(parentAst, TokenTypes.SLIST, TokenTypes.OBJBLOCK)) {

result = parentAst;

}

else {

result = parentAst.getParent();

}

return result;

but you might mean that we do validation based on scope token type

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

Lines 310 to 312 in d4b1aec

public void leaveToken(DetailAST ast) {

if (TokenUtil.isOfType(ast, SCOPES)) {

logViolations(ast, variables);

so any not SCOPES will be skiped here

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

Lines 423 to 424 in d4b1aec

private void logViolations(DetailAST scopeAst, Deque<VariableDesc> variablesStack) {

while (!variablesStack.isEmpty() && variablesStack.peek().getScope() == scopeAst) {

Once again after evaluating and following pitest of pr #13152 pitest) it looks to me that the design needs to be changed or the logic looks very clean to me so let it be suppressed

I lost track of this PR, sorry for no response for long, I will do deep dive one more time to make decision.

@Vyom-Yadav

Why complicate the check more by adding a new scope when the current definition of scope makes everything work.

We are not extending SCOPES with a new scope.

So it does not matter if we do checking at OBJBLOCK or LITERAL_NEW, the result would be the same.

I didn't get it, why the result would be the same? If it's the same and we are not extending SCOPES and there is no input test, why shouldn't we accept Kevin's change?

We are not extending SCOPES with a new scope.

We are not adding anything to SCOPES static field but now a variable has LITERAL_NEW as scope which is not what the check promises. Sort of violates the contract.

I didn't get it, why the result would be the same? If it's the same and we are not extending SCOPES and there is no input test, why shouldn't we accept Kevin's change?

The result would be the same because no instead of both OBJBLOCK and LITERAL_NEW don't contribute anything to the violations (as they are non-local). On top of that, it won't affect the stack order because they are popped off at the right time. (With this change, at LITERAL_NEW instead of OBJBLOCK)

I tried to play with code preserving OBJBLOCK and resolving the mutation - https://github.com/checkstyle/checkstyle/pull/13546/files.

@Vyom-Yadav what do you think about such a change, assuming it passes all needed ci checks?

romani · 2023-07-04T03:13:10Z

@Kevin222004 , please rebase to resolve conflict.

romani · 2023-08-21T03:39:49Z

@Kevin222004 , conflict again

Kevin222004 · 2023-08-23T18:14:07Z

#13151 (comment) Resolved

rdiachenko

lgtm

I tried a different approach here #13546 (comment) but it is less safe and may break existing logic. I'd rather go with the current approach with is simpler and safer.

rdiachenko · 2023-10-26T21:05:16Z

@Kevin222004 please resolve conflicts

romani

I am ok to merge.

Absence of any mutation will not next contributor to extend list of situations.

Vyom-Yadav

Though this is not what I wanted, but I can see why this is the better solution.

While designing this check, I didn't keep local classes in mind ('cause colliding variable names in local classes is really rare, and then their nesting, it can get reallyyy complex).

Pitest is indirectly indicating that only, this breaks the contract of the check but the logic is working fine. I will come back to this check sometime with hopefully a better design.

LGTM!

romani · 2023-10-29T16:57:43Z

By merging this we close a door to all other leaks for survival, one day we will see how to make design better, we already spent too much time on this, we need a break.

Kevin222004 changed the title ~~Issue #13109: Kill mutation for UnusedLocalVariableCheck 8~~ Issue #13328: Kill mutation for UnusedLocalVariableCheck 8 Jul 1, 2023

Kevin222004 force-pushed the ul8 branch from 34c1110 to 47f994b Compare July 1, 2023 03:28

Kevin222004 mentioned this pull request Jul 1, 2023

Targeted Pitest Mutation in July 2023 #13328

Closed

12 tasks

Kevin222004 marked this pull request as ready for review July 1, 2023 03:45

Kevin222004 force-pushed the ul8 branch from 47f994b to f6ffe95 Compare July 1, 2023 03:50

romani requested changes Jul 1, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/UnusedLocalVariableCheck.java Show resolved Hide resolved

Kevin222004 force-pushed the ul8 branch from f6ffe95 to bf5bbee Compare July 1, 2023 04:36

romani approved these changes Jul 1, 2023

View reviewed changes

romani requested review from Vyom-Yadav and rdiachenko July 1, 2023 04:46

romani assigned rdiachenko Jul 1, 2023

This was referenced Jul 1, 2023

Issue #13501: Kill mutation for UnusedLocalVariableCheck 9 #13152

Merged

Issue #13328: Kill mutation for UnusedLocalVariableCheck 10 #13331

Merged

rdiachenko approved these changes Jul 1, 2023

View reviewed changes

rdiachenko assigned Vyom-Yadav and unassigned rdiachenko Jul 1, 2023

Vyom-Yadav suggested changes Jul 2, 2023

View reviewed changes

Kevin222004 force-pushed the ul8 branch from bf5bbee to 31b50c6 Compare July 4, 2023 03:40

romani assigned romani and unassigned Vyom-Yadav Jul 31, 2023

romani self-requested a review July 31, 2023 05:58

This was referenced Aug 2, 2023

storage Kevin222004/GitHub-action#3

Closed

Resolve pitest survivals in modules - 3 #13501

Closed

Issue #13328: remove OBJBLOCK scope from UnusedLocalVariableCheck #13513

Closed

rdiachenko mentioned this pull request Aug 10, 2023

Issue #13328: resolve mutation for UnusedLocalVariableCheck #13546

Closed

romani assigned rdiachenko and unassigned romani Aug 11, 2023

romani requested a review from rdiachenko August 11, 2023 02:57

Kevin222004 force-pushed the ul8 branch from 31b50c6 to e915bd6 Compare August 23, 2023 18:13

Kevin222004 mentioned this pull request Aug 31, 2023

Resolve pitest survivals in modules - 4 #13672

Closed

29 tasks

rnveach assigned Vyom-Yadav and unassigned rdiachenko Aug 31, 2023

Kevin222004 mentioned this pull request Oct 1, 2023

Resolve pitest survivals in modules - 5 #13809

Closed

33 tasks

rdiachenko approved these changes Oct 26, 2023

View reviewed changes

Issue checkstyle#13328: Kill mutation for UnusedLocalVariableCheck 8

2a3965f

Kevin222004 force-pushed the ul8 branch from e915bd6 to 2a3965f Compare October 26, 2023 21:20

romani approved these changes Oct 26, 2023

View reviewed changes

Vyom-Yadav approved these changes Oct 29, 2023

View reviewed changes

Vyom-Yadav assigned romani and unassigned Vyom-Yadav Oct 29, 2023

romani merged commit eacee58 into checkstyle:master Oct 29, 2023

	<mutation unstable="false">
	<sourceFile>UnusedLocalVariableCheck.java</sourceFile>
	<mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.UnusedLocalVariableCheck$TypeDeclDesc</mutatedClass>
	<mutatedMethod>getUpdatedCopyOfVarStack</mutatedMethod>
	<mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.NakedReceiverMutator</mutator>
	<description>replaced call to com/puppycrawl/tools/checkstyle/api/DetailAST::getLastChild with receiver</description>
	<lineContent>final DetailAST updatedScope = literalNewAst.getLastChild();</lineContent>
	</mutation>

	/**
	* An array of scope tokens.
	*/
	private static final int[] SCOPES = {
	TokenTypes.SLIST,
	TokenTypes.LITERAL_FOR,
	TokenTypes.OBJBLOCK,
	};

	final VariableDesc desc = new VariableDesc(ident.getText(),
	varDefAst.findFirstToken(TokenTypes.TYPE), findScopeOfVariable(varDefAst));

	private static DetailAST findScopeOfVariable(DetailAST variableDef) {
	final DetailAST result;
	final DetailAST parentAst = variableDef.getParent();
	if (TokenUtil.isOfType(parentAst, TokenTypes.SLIST, TokenTypes.OBJBLOCK)) {
	result = parentAst;
	}
	else {
	result = parentAst.getParent();
	}
	return result;

	public void leaveToken(DetailAST ast) {
	if (TokenUtil.isOfType(ast, SCOPES)) {
	logViolations(ast, variables);

	private void logViolations(DetailAST scopeAst, Deque<VariableDesc> variablesStack) {
	while (!variablesStack.isEmpty() && variablesStack.peek().getScope() == scopeAst) {

Uh oh!

Conversation

Kevin222004 commented Jun 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Check :-

Mutation

Explaination

Regression :-

Uh oh!

Kevin222004 commented Jun 3, 2023

Uh oh!

github-actions bot commented Jun 3, 2023

Uh oh!

Kevin222004 commented Jul 1, 2023

Uh oh!

romani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Kevin222004 commented Jul 1, 2023

Uh oh!

github-actions bot commented Jul 1, 2023

Uh oh!

rdiachenko 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!

romani Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

romani commented Jul 4, 2023

Uh oh!

romani commented Aug 21, 2023

Uh oh!

Kevin222004 commented Aug 23, 2023

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

rdiachenko commented Oct 26, 2023

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Vyom-Yadav left a comment

Choose a reason for hiding this comment

Uh oh!

romani commented Oct 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Kevin222004 commented Jun 3, 2023 •

edited

Loading

romani left a comment •

edited

Loading

romani Jul 31, 2023 •

edited

Loading