creation of mutable collection consider as can be avoided

[Kevin222004]

        public Set<DetailAST> getMethodCalls() {
            return Collections.unmodifiableSet(methodCalls);
        }

this code has been mutated as

        public Set<DetailAST> getMethodCalls() {
            return methodCalls;
        }

for

 <mutation unstable="false"> 
   <sourceFile>EqualsAvoidNullCheck.java</sourceFile> 
   <mutatedClass>com.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheck$FieldFrame</mutatedClass> 
   <mutatedMethod>getMethodCalls</mutatedMethod> 
   <mutator>org.pitest.mutationtest.engine.gregor.mutators.experimental.ArgumentPropagationMutator</mutator> 
   <description>replaced call to java/util/Collections::unmodifiableSet with argument</description> 
   <lineContent>return Collections.unmodifiableSet(methodCalls);</lineContent> 
 </mutation> 

Modifying the code with Pitest from return Collections.unmodifiableSet(methodCalls) to return new HashSet<>(methodCalls) eliminates the guarantee of returning an unmodifiable set, potentially leading to unintended modifications and inconsistent behavior.

we have faced this issue at https://github.com/checkstyle/checkstyle/pull/13127 and https://github.com/checkstyle/checkstyle/pull/13126

[romani]

Some internal immutable collection is good for engineers express intend, such details might not be easily testable from public methods. I think it is same level of restrictions that engineers use by placing final on variable, it is signal for compiler or other engineers that mutation is not recommended or not expected.

Avoiding reporting of survivals would be awesome, may be by options or specific mutator or ....

[hcoles]

This feature is implemented in #1310. In the next release it will be possible to filter these mutations out by adding the filter string "+funmodifiablecollection"

[romani]

@hcoles , please share link to some doc or please share how to use ? Or where to put "+funmodifiablecollection" ? We use maven.

Thanks a lot in advance. May be this https://pitest.org/quickstart/maven/#features ?

[romani]

@hcoles , one more fix is required Please look at https://github.com/checkstyle/checkstyle/pull/14484#discussion_r1490310155 Look like you covered the only when code is return ....;

[hcoles]

The filter is restricted to direct returns wrapping return values in this way is wide spread idiom where it might be reasonably argued that writing tests to confirm the behaviour isn't a productive use of time. A failure to wrap a return value can be detected by static an analysis rather than a test.

Filtering the calls in other places makes less sense as it means pitest would not highlight redundant code.

The example code in your link

    public List<Token> getHiddenBefore() {
        List<Token> returnList = null;
        if (hiddenBefore != null) {
            returnList = Collections.unmodifiableList(hiddenBefore);
        }
        return returnList;
    }

Could be more clearly expressed as

    public List<Token> getHiddenBefore() {
        if (hiddenBefore != null) {
            return Collections.unmodifiableList(hiddenBefore);
        }
        return hiddenBefore;
    }

Or

    public List<Token> getHiddenBefore() {
        if (hiddenBefore == null) {
            return null;
        }
        return Collections.unmodifiableList(hiddenBefore);
    }

To emphasize that the method may return null.

[romani]

there is another evil "single return from method" , but we refactored it to java8 style of code to reconcile. I pretty sure there will be numerous examples where usage of unmodifiableList without return is reasonable.

ok, what about code like (link):

    public void setHiddenBefore(List<Token> hiddenBefore) {
        this.hiddenBefore = Collections.unmodifiableList(hiddenBefore);
    }

[romani]

still mutation survival: https://github.com/checkstyle/checkstyle/actions/runs/7916871513/job/21611685470?pr=14484#step:6:458

Source File: "DetailAstImpl.java"
Class: "com.puppycrawl.tools.checkstyle.DetailAstImpl"
Method: "getHiddenAfter"
Line Contents: ".map(Collections::unmodifiableList)"
Mutator: "org.pitest.mutationtest.engine.gregor.mutators.experimental.NakedReceiverMutator"
Description: "replaced call to java/util/Optional::map with receiver"
Line Number: 514

java:

        return Optional.ofNullable(hiddenBefore)
            .map(Collections::unmodifiableList)
            .orElse(null);

[hcoles]

    public void setHiddenBefore(List<Token> hiddenBefore) {
        this.hiddenBefore = Collections.unmodifiableList(hiddenBefore);
    }

Wrapping on write is a common idiom, and I agree it would make sense to filter this.

Patterns such as creating an Optional in order to avoid writing an if statement, are not so common and it makes sense for pitest to continue to highlight code not justified by a test in these constructs.

If you wish to avoid mutating calls to unmodifiableList etc completely I think this would be better dealt with by a general purpose "don't mutate this" parameter similar to avoidCallTo.

[romani]

Do you already have parameter by which we config pitest to skip mutation over unmodifiableList at all ?

avoidCallTo do no mutation inside method, but still mutate call of method.

[hcoles]

avoidCallsTo would work, but it works at the class level, so you have to filter out calls to everything in java.util.Collections.

Filtering out calls to individual methods would need a new parameter.

[romani]

yes, you are right, we already use avoidCallsTo for classes. https://github.com/checkstyle/checkstyle/blob/c1ca1b903beaed94923f621fac2197394ae011dd/pom.xml#L3079-L3082

but did not want to exclude all methods from this class, as it has bunch of other methods like sort/...... that is better to question their necessity in main code. that is why we created new class as wrapper to suppress only unmodifiableXxxx. It would be awesome to let users suppress more granular for specific method of class.