Resolve Pitest Issues - AvoidStarImportCheck (1)

[rnveach]

Child issue of #7797 ,

This issue specifically focuses on the line

checkstyle/.ci/pitest.sh

Line 74 in abf829f

"AvoidStarImportCheck.java.html:<td class='covered'><pre><span class='survived'> &#38;&#38; ast.getType() == TokenTypes.STATIC_IMPORT) {</span></pre></td></tr>"

[wilcoln]

I'm on it.

[wilcoln]

Pitest report

• Link: https://wilcoln.github.io/checkstyle-reports/pit-reports/202003130421/com.puppycrawl.tools.checkstyle.checks.imports/AvoidStarImportCheck.java.html
• Surviving mutations:
  • on line 188: removed conditional - replaced equality check with false → SURVIVED referenced by Resolve Pitest Issues - AvoidStarImportCheck (2) #7800, someone else's on it.
  • on line 224: removed conditional - replaced equality check with true → SURVIVED we're focusing on it.

Hardcoded mutation

• mutation cs branch: wilcoln@a0fcb5c
• comment:
  Looking at the pitest report, we can see that the surviving mutation is the one for which the equality check is set to true, this is equivalent as if it wasn't even there, so we can simply remove it. Which I did, leaving us with only the first condition inside the if statement.

Regression diff reports

Recall of check properties

                                                                                                                             

name	description	type	default value
excludes	Specify packages where star imports are allowed.	String Set	{}
allowClassImports	Control whether to allow starred class imports like   import java.util.*;.	Boolean	false
allowStaticMemberImports	Control whether to allow starred static member imports like   import static org.junit.Assert.*;.	Boolean	false

Default configuration

<module name="AvoidStarImport"/>

• Scope: All the projects.
• Link: https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf0/diff/index.html
• Bottom line: No difference found.

Non default configuration 1

<module name="AvoidStarImport">
  <property name="excludes" value="java.io,java.net,java.lang.Math"/>
</module>

• Scope: All the projects.
• Link: https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf1/diff/index.html
• Bottom line: No difference found.

Non default configuration 2

<module name="AvoidStarImport">
  <property name="allowClassImports" value="true"/>
</module>

• Scope: All the projects.
• Link: https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf2/diff/index.html
• Bottom line: No difference found.

Non default configuration 3

<module name="AvoidStarImport">
   <property name="allowStaticMemberImports" value="true"/>
</module>

• Scope: All the projects.
• Link: https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf3/diff/index.html
• Bottom line: No difference found.

Code analysis

The regression diff reports do not find any UT which kills the surviving mutation and I think this is because the condition is really not needed.
If we take a close look at the visitToken(final DetailAst ast) method of the AvoidStarImportCheck class, we can see that if we reach the else if statement, it implies one of two things:

1. The ast type is, indeed, STATIC_IMPORT
2. The ast type is IMPORT and allowClassImports is true

We're only interested in the 2nd case here, and in that case there's clearly no violation.

Bottom line : I propose to keep the mutation.

Related PR is #7843

NB For non default configurations I have only uploaded android-launcher diff report because my git repo is becoming way too big. Be assured that the results are the same for all other projects.

[rnveach]

The ast type is, indeed, STATIC_IMPORT
The ast type is IMPORT and allowClassImports is true
We're only interested in the 2nd case here, and in that case there's clearly no violation.

I agree that the 2nd one is the more important one. Regression was expanded to much more properties and permutations, so I am good regression won't help us.

Bottom line : I propose to keep the mutation.

Please convince me that the best choice is to remove the code, as there is no way this can produce any violation issues when released.

Why does, when the 2nd case's requirements are met, a violation is not produced? Do we have a duplicate check somewhere that checks for STATIC_IMPORT? What happens differently when ast is IMPORT versus STATIC_IMPORT that something else down the line won't produce a violation, no matter what, when the else if condition is met.

[wilcoln]

Let's assume the 2nd case's requirements are met for some import statement.

1. The ast type is IMPORT which means the corresponding statement is a class import as opposed to a static member import.
2. allowClassImports = true which means that, since our statement is a class import, we allow it.

That's why there is no violation.

[rnveach]

I agree with your number 1, but number 2 plays no role here. There are no further checks for allowClassImports inside the condition. That property only helped us get to the else if, nothing more. You can still have a class import with a start at this location.

I want to know what prevents a violation happening inside the condition under all circumstances.

[wilcoln]

There are no further checks for allowClassImports

Yeah but since we're in case 2 of #7799 (comment), we know for sure that that property's value is true

You can still have a class import with a start at this location

I agree with this completely, and what I am saying is that knowing allowClassImports = true, a class import with a star doesn't cause any violation.

At least that's what I thought from the property's description, but maybe I'm misunderstanding.

[wilcoln]

@rnveach I got proof!

I just ran java -jar checkstyle-8.29-all.jar -c config.xml Test.java on the following source file:

import java.util.*; // a class import with a star

class Foo{
  private int a;
  /* public comment */ // OK, comment is ignored
  public bar1() {} // violation
  public bar2() {} // violation
}

For each of the two following configs:

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
         <module name="AvoidStarImport"/>
    </module>
</module>

And

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
        <module name="AvoidStarImport">
             <property name="allowClassImports" value="true"/>
       </module>
</module>

the results are respectively:

Starting audit...
[ERROR] /var/tmp/Test.java:2: Using the '.*' form of import should be avoided - java.util.*. [AvoidStarImport]
Audit done.
Checkstyle ends with 1 errors.

And

Starting audit...
Audit done.

I believe this proves my point.

what I am saying is that knowing allowClassImports = true, a class import with a star doesn't cause any violation

[rnveach]

we know for sure that that property's value is true

Yes, but without it being used anymore it is not proof that it has any impact in the condition.

I got proof!

I don't see how this proves what is happening inside the code between 2 versions of checkstyle when you are only using 1 version in your example. Surely regression would have had this case already.

---

I suggest we walk through the code line by line inside the condition with a STATIC_IMPORT and IMPORT and see where the differences fall.

[wilcoln]

I suggest we walk through the code line by line inside the condition with a STATIC_IMPORT and IMPORT and see where the differences fall.

Ok let's do that.
Let's consider the mutation code (i.e there's no check of the ast type inside the else if condition).
Let's assume the 2nd case's requirements are met for some import statement.

• The ast type is IMPORT and allowClassImports is true.
• We get to the else if condition, once again , two cases are possible
  1. If allowStaticMembersImports is true we do not enter the else if body => no violation is reported
  2. otherwise we do enter the else if body
     At this point, since ast type is IMPORT then at line 255, startingDot value is actually SEMI (;) and not DOT (.)
     (See the tree below, from TokenTypes.java)

+--IMPORT (import)
    |
    +--DOT (.)
        |
        +--DOT (.)
            |
            +--IDENT (java)
            +--IDENT (io)
        +--IDENT (IOException)
    +--SEMI (;)

So at line 226 we call logsStarredImportViolation(startingDot) passing it a SEMI(;) instead of a DOT(.).
Given that, importText local variable can't possibly end with ".*". Hence we do not enter the if body at line 238 => no violation is reported.

[wilcoln]

@rnveach , please your thoughts

[rnveach]

This makes more sense now.

Given that, importText local variable can't possibly end with ".*". Hence we do not enter the if body at line 238

There was a small leap not saying how importText got its value but its directly from the AST brought in which is only a ; in the bad case. I was able to still follow.

I agree now, there is no way to satisfy the code as it is now except to change it somehow, either remove the mutated code or rewrite the code.

[rnveach]

Fix was merged