Issue #7920: Resolve Pitest Issues - RedundantImportCheck

[Gaurav-Punjabi]

Fixes issue #7920

Description

This mutation was surviving removed conditional - replaced equality check with true → KILLED at line no 120.

Solution

If pkgName is null it means this is an unnamed package and as an unnamed package cannot be imported we don't need to check if the pkgName is null as the second condition will return false.

[rnveach]

No regression was provided. Pitest report was not provided.

@Gaurav-Punjabi Please provide us regression and pitest report as described in main issue.

[rnveach]

#7929 (comment)

[Gaurav-Punjabi]

No regression was provided. Pitest report was not provided.

@Gaurav-Punjabi Please provide us regression and pitest report as described in main issue.

Added both the reports as a comment in the issue. Please review

[Gaurav-Punjabi]

@romani Why is this travis build failing? I don't understand.

[rnveach]

Since regression didn't return anything and you have decided to remove the code, we need an analysis of the code that we should absolutely remove it and cannot create a test to satisfy it.

as an unnamed package cannot be imported we don't need to check

Is this a general statement or can you show this isn't possible by the code and an AST provided to it.

if the pkgName is null as the second condition will return false.

Will it always return false in any circumstance and why?

[Gaurav-Punjabi]

Will it always return false in any circumstance and why?

Yes because isFromPackage() internally calls String's equals method and the package name is passed as an argument to it as we can see in the snippet below:
return front.equals(pkg);
So even if null is passed to this method it will return false, as String's equal method checks if the parameter is null or not. If it is null it returns false.

as an unnamed package cannot be imported we don't need to check

It's a general statement. Anyways even if pkgName is passed null, the second condition will still return false as described above.

[rnveach]

as an unnamed package cannot be imported we don't need to check

https://docs.oracle.com/javase/specs/jls/se13/html/jls-7.html#jls-7.5

A type in an unnamed package (§7.4.2) has no canonical name, so the requirement for a canonical name in every kind of import declaration implies that (a) types in an unnamed package cannot be imported, and (b) static members of types in an unnamed package cannot be imported. As such, §7.5.1, §7.5.2, §7.5.3, and §7.5.4 all require a compile-time error on any attempt to import a type (or static member thereof) in an unnamed package.

It is more than a general statement, but a fact. I could find no way to import a class from an unnamed package as it just results in errors. it is also documented in isFromPackage as well.

So I am on the verge with agreeing with you.

return front.equals(pkg);
So even if null is passed to this method it will return false, as String's equal method checks if the parameter is null or not.

So this is the whole crux of why the mutation survives is because this line is null safe from pkg. However, if we swap the code so it is pkg.equals(front) it no longer becomes null safe. It seems to me the reason for the extra null check outside the method is to prevent doing any extra calculation than we need to do, which is the execution of the lines in isFromPackage. Since it would no longer be null safe with the swap, the mutation should be killed when it gets this exception from the run.

Does it make sense that we try to keep the null check and try to minimize execution from doing any unnecessary steps?

[Gaurav-Punjabi]

Does it make sense that we try to keep the null check and try to minimize execution from doing any unnecessary steps?

Yes, I'm okay with this. It would surely reduce a function call and also kill the mutation.
So should I change this?

[rnveach]

I think we should as long as pitest passes and no other part of mvn verify or CI complains about the switch.
Please update the PR and I will approve.

[Gaurav-Punjabi]

Please update the PR and I will approve.

Done.

[rnveach]

@Gaurav-Punjabi CI failied because commit is based on too old a version of master. Please rebase.
https://travis-ci.org/github/checkstyle/checkstyle/jobs/666890436#L285

[Gaurav-Punjabi]

@Gaurav-Punjabi CI failied because commit is based on too old a version of master. Please rebase.
https://travis-ci.org/github/checkstyle/checkstyle/jobs/666890436#L285

Done rebased it to master.