Issue #13809: Kill mutation in Tree-walker by Kevin222004 · Pull Request #13928 · checkstyle/checkstyle

Kevin222004 · 2023-10-22T09:42:27Z

Issue #13809: Kill mutation in Tree-walker

nrmancuso · 2023-10-22T13:37:05Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

        return Stream.concat(filters.stream(),
                Stream.concat(ordinaryChecks.stream(), commentChecks.stream()))
            .filter(ExternalResourceHolder.class::isInstance)
-            .map(ExternalResourceHolder.class::cast)


@Kevin222004 I am curious; does this suffer the same surviving mutation?

.map(r -> (ExternalResourceHolder) r)

@nrmancuso Yes, it will survive the same.

https://kevin222004.github.io/PitestReport/com.puppycrawl.tools.checkstyle/TreeWalker.java.html

nrmancuso · 2023-10-22T13:39:32Z

config/pitest-suppressions/pitest-tree-walker-suppressions.xml

-    <mutatedClass>com.puppycrawl.tools.checkstyle.TreeWalker</mutatedClass>
-    <mutatedMethod>getExternalResourceLocations</mutatedMethod>
-    <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.NakedReceiverMutator</mutator>
-    <description>replaced call to java/util/stream/Stream::map with receiver</description>


@Kevin222004 doesn't this effectively mean that we completely remove the call to map from this chain of method calls? This seems like a pitest bug to me, I would not expect such code to compile.

there is compilation error:

[INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ [INFO] Total time: 8.178 s [INFO] Finished at: 2023-10-22T06:44:59-07:00 [INFO] ------------------------------------------------------------------------ [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project checkstyle: Compilation failure: Compilation failure: [ERROR] /home/rivanov/java/github/romani/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java:[391,42] cannot find symbol [ERROR] symbol: method getExternalResourceLocations() [ERROR] location: variable resource of type java.lang.Object [ERROR] /home/rivanov/java/github/romani/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java:[392,21] incompatible types: inference variable T has incompatible bounds [ERROR] equality constraints: java.lang.String [ERROR] lower bounds: java.lang.Object [ERROR] -> [Help 1] [ERROR] [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch. [ERROR] Re-run Maven using the -X switch to enable full debug logging. [ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles: [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException $ git diff diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java b/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java index ae9a0e6..9c3d5c8 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java @@ -388,7 +388,6 @@ public final class TreeWalker extends AbstractFileSetCheck implements ExternalRe return Stream.concat(filters.stream(), Stream.concat(ordinaryChecks.stream(), commentChecks.stream())) .filter(ExternalResourceHolder.class::isInstance) - .map(ExternalResourceHolder.class::cast) .flatMap(resource -> resource.getExternalResourceLocations().stream()) .collect(Collectors.toSet()); }

I think it is reasonable to contact pitest.

@Vyom-Yadav , did you experience the same ?

I think mutations are by design create code that is not compile able.
Compiler smart and can block nonsense code writing, but jvm can execute any bitecode.
So we can not create issue on pitest, it is by design.

@nrmancuso .

Pitest is maniupulation of the byte code. Compilation is turning source code to byte code. There is no "compilable" code as byte code pitest modifies is already compiled from source. Pitest is basically viewing and modifying assembly code at this point.

In this instance, it probably means you did not successfully recreate what pitest is doing by modifying the source instead. It also could be that pitest is not implementing some safeties in its mutations that the compiler checks for and adds. You will need to look at the original bytecode and the modified bytecode to see what exactly pitest is doing to even know if it is a bug in pitest or a misunderstanding by the user. We had to do this quite a bit for this project, which hopefully we aren't lowering our standards.

You may want to submit this issue to pitest as it seems it will always happily ignore type casts while still being able to call methods that require the cast in the first place. This seems like pitest is bypassing what the compiler checks and the JVM assumes the compiler already did all the proper checks in the first place to not care at run time.

As few people are unsure, let's create issue in pitest to get official position on what is correct.

But in same time let's proceed with PR acceptance.

Fact: we still don’t know what the actual mutation is. That is enough to block this PR.

It is ok that pitest mutate code to non compilable code, mutation already imply that code becomes weird and error prone. Java compiler catch most basic and obvious nonsense code.

romani

Ok to merge

rdiachenko

lgtm

rnveach · 2023-10-24T14:34:24Z

#13928 (comment)

Vyom-Yadav · 2023-10-25T18:53:27Z

I don't think it is straight away removing call to map, I pointed out exactly the same type of mutation in #13913 (review).

That comment contains the bytecode generated by PIT after inserting the mutation. Decompiling does not guarantee compilable code, so I wasn't sure whether it was PIT or decompilers limitation.

rnveach · 2023-10-25T19:06:19Z

https://github.com/checkstyle/checkstyle/wiki/How-to-Generate-and-Analyze-Pitest-Reports#decompiler-usage-to-see-bytecode

It may be misleading to say that it is a decompiler.
"The decompiler translates a compiled code or an executable file into high-level code."
"A decompiler is a computer program that translates an executable file to high-level source code."

You need to look at the mutated bytecode, and then create an understanding from that what it is doing. I am sure it is possible to have bytecode that will not translate into any standard Java, that doesn't mean the bytecode isn't valid and executable.

Vyom-Yadav · 2023-10-25T19:07:25Z

Here is the mutated bytecode for this particular mutation:
mutant1.class.gz

Continuing my comment at #13928 (comment), even the non-mutated code, decompiled by fernflower isn't compilable.

Here is what fernflower gave me:

public Set<String> getExternalResourceLocations() {                                                                                              
    Stream var10000 = Stream.concat(this.filters.stream(), Stream.concat(this.ordinaryChecks.stream(), this.commentChecks.stream()));            
    Objects.requireNonNull(ExternalResourceHolder.class);                                                                                        
    var10000 = var10000.filter(ExternalResourceHolder.class::isInstance);                                                                        
    Objects.requireNonNull(ExternalResourceHolder.class);                                                                                        
    return (Set)var10000.map(ExternalResourceHolder.class::cast).flatMap((resource) -> {                                                         
        return resource.getExternalResourceLocations().stream();                                                                                 
    }).collect(Collectors.toSet());                                                                                                              
}

[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/vyom/IdeaProjects/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java:[394,28] cannot find symbol
  symbol:   method getExternalResourceLocations()
  location: variable resource of type java.lang.Object
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:27 min
[INFO] Finished at: 2023-10-26T00:34:48+05:30
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project checkstyle: Compilation failure
[ERROR] /home/vyom/IdeaProjects/checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java:[394,28] cannot find symbol
[ERROR]   symbol:   method getExternalResourceLocations()
[ERROR]   location: variable resource of type java.lang.Object
[ERROR] 
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

vyom at fedora in ~/IdeaProjects/checkstyle (fixLinkks●) 
$ git diff          
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java b/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java
index ae9a0e646..31175d9c3 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java
@@ -27,6 +27,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
@@ -385,12 +386,13 @@ public final class TreeWalker extends AbstractFileSetCheck implements ExternalRe
 
     @Override
     public Set<String> getExternalResourceLocations() {
-        return Stream.concat(filters.stream(),
-                Stream.concat(ordinaryChecks.stream(), commentChecks.stream()))
-            .filter(ExternalResourceHolder.class::isInstance)
-            .map(ExternalResourceHolder.class::cast)
-            .flatMap(resource -> resource.getExternalResourceLocations().stream())
-            .collect(Collectors.toSet());
+        Stream var10000 = Stream.concat(this.filters.stream(), Stream.concat(this.ordinaryChecks.stream(), this.commentChecks.stream()));
+        Objects.requireNonNull(ExternalResourceHolder.class);
+        var10000 = var10000.filter(ExternalResourceHolder.class::isInstance);
+        Objects.requireNonNull(ExternalResourceHolder.class);
+        return (Set)var10000.map(ExternalResourceHolder.class::cast).flatMap((resource) -> {
+            return resource.getExternalResourceLocations().stream();
+        }).collect(Collectors.toSet());
     }
 
     /**

So I think we should at least bring this to pitest maintainers attention.

rnveach · 2023-10-25T19:08:02Z

Provide us the difference from JavaP between the mutated and non-mutated byte code.

even the non-mutated code, decompiled by fernflower isn't compilable.

Again, you can bring this to pitest, but they will probably tell you what I have been saying. Pitest doesn't work on the source. It is not its job to create mutations that can be represented in Java code.

Vyom-Yadav · 2023-10-25T19:18:49Z

Provide us the difference from JavaP between the mutated and non-mutated byte code.

https://www.diffchecker.com/LhJhXQVV/ line 881

- invokeinterface #114,  2          // InterfaceMethod java/util/stream/Stream.map:(Ljava/util/function/Function;)Ljava/util/stream/Stream;
+ pop

Looks like it simply removed the call.

rnveach · 2023-10-25T19:28:30Z

Thank you.

This makes sense. This is what Naked Receiver does.

The only difference is pitest can modify the bytecode and can bypass the type check. This is what I surmised at #13928 (comment) .

I still think we should report this case to pitest because according to the bytecode it creates an equivalent mutation when it doesn't when compared to the source compiler. We should avoid these cases and this PR in hopes that pitest acknowledges and provides a fix.

romani · 2023-10-25T20:07:21Z

Type checking is compiler job, jvm bite code executes just commands, and if some casting problems happens ( if compiler missed it) it will be run time exception.

But we do filter(ExternalResourceHolder.class::isInstance) so we by all means avoid exceptions, and have our code double safe, and pitest does not like double safety by design.

rnveach · 2023-10-25T20:18:32Z

An exception would kill the mutation. Since the mutation survives, I assume there is no runtime exception happening.

romani · 2023-10-25T20:21:23Z

Yes, because code generate proper types of collection elements, and only single mutation happening at the time.

romani · 2023-10-27T13:59:34Z

@Vyom-Yadav, how you get mutated bytecode ?

Vyom-Yadav · 2023-10-28T04:46:28Z

I don't think pitest can solve this. If anything, if a method was invoked at A then that method invokes B and so on...

Somewhere down in the tree there is a cast. Pitest would possibly have to ignore all the methods in the tree to get rid of this mutation. IT MAY NOT BE a direct child parent relationship with these type of method calls.

Our best option rn is to work around like we are doing rn. This mutation only survives because this is only required by compiler. Maybe a pitest limitation.

But let's create an issue to discuss this with the maintainers.

romani · 2023-10-28T15:05:36Z

@Kevin222004 , please create issue on pitest.

Vyom-Yadav · 2023-10-29T04:04:33Z

@Vyom-Yadav, how you get mutated bytecode ?

I have made some modifications to the test suite of pitest, so when a class is mutated, I save the mutated bytecode. See hcoles/pitest@master...Vyom-Yadav:pitest:minorMod

rnveach · 2023-10-29T04:07:17Z

There is also hcoles/pitest#391 (comment)
https://github.com/pitest/export-plugin

Vyom-Yadav · 2023-10-29T17:04:31Z

Nice find, hcoles/pitest#432, it was moved to the main repo.

Vyom-Yadav

LGTM! Mutant has been discussed. This is the best way to proceed IMO.

romani · 2023-10-29T17:44:43Z

I stored both link to https://github.com/checkstyle/checkstyle/wiki/How-to-Generate-and-Analyze-Pitest-Reports

and clear answer from author that pitest does not try code to be mathching to compiler expectations.

nrmancuso · 2023-10-30T04:02:10Z

I am prepared to approve this PR once we have issue opened in pitest repo.

romani · 2023-10-30T12:55:49Z

issue is opened hcoles/pitest#1270

Issue checkstyle#13809: Kill mutation in Tree-walker

7233cf4

nrmancuso reviewed Oct 22, 2023

View reviewed changes

romani self-assigned this Oct 22, 2023

Kevin222004 mentioned this pull request Oct 23, 2023

Resolve pitest survivals in modules - 5 #13809

Closed

33 tasks

romani approved these changes Oct 24, 2023

View reviewed changes

romani assigned rdiachenko and unassigned romani Oct 24, 2023

romani requested review from Vyom-Yadav and rdiachenko October 24, 2023 02:17

rdiachenko approved these changes Oct 24, 2023

View reviewed changes

rdiachenko assigned Vyom-Yadav and unassigned rdiachenko Oct 24, 2023

rnveach added the blocked label Oct 24, 2023

Vyom-Yadav approved these changes Oct 29, 2023

View reviewed changes

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

Vyom-Yadav removed the blocked label Oct 29, 2023

romani mentioned this pull request Oct 30, 2023

NakedReceiverMutator demands removal of map(MyClass.class::cast) but java compiler requires it hcoles/pitest#1270

Open

nrmancuso approved these changes Oct 30, 2023

View reviewed changes

nrmancuso merged commit 9956228 into checkstyle:master Oct 30, 2023

Uh oh!

Conversation

Kevin222004 commented Oct 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrmancuso Oct 22, 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!

rnveach Oct 24, 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!

Choose a reason for hiding this comment

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

rnveach commented Oct 24, 2023

Uh oh!

Vyom-Yadav commented Oct 25, 2023

Uh oh!

rnveach commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vyom-Yadav commented Oct 25, 2023

Uh oh!

rnveach commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vyom-Yadav commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Oct 25, 2023

Uh oh!

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

Uh oh!

Uh oh!

rnveach commented Oct 25, 2023

Uh oh!

romani commented Oct 25, 2023

Uh oh!

romani commented Oct 27, 2023

Uh oh!

Vyom-Yadav commented Oct 28, 2023

Uh oh!

romani commented Oct 28, 2023

Uh oh!

Vyom-Yadav commented Oct 29, 2023

Uh oh!

rnveach commented Oct 29, 2023

Uh oh!

Vyom-Yadav commented Oct 29, 2023

Uh oh!

Vyom-Yadav left a comment

Choose a reason for hiding this comment

Uh oh!

romani commented Oct 29, 2023

Uh oh!

nrmancuso commented Oct 30, 2023

Uh oh!

romani commented Oct 30, 2023

Uh oh!

Reviewers

nrmancuso Oct 22, 2023 •

edited

Loading

rnveach Oct 24, 2023 •

edited

Loading

rnveach commented Oct 25, 2023 •

edited

Loading

rnveach commented Oct 25, 2023 •

edited

Loading

Vyom-Yadav commented Oct 25, 2023 •

edited

Loading

romani commented Oct 25, 2023 •

edited

Loading