Block mutator shouldn't remove blocks that only return default

[sbergen]

Describe the bug

Given the following code and tests, the behaviour mentioned in the comments will be encountered.

Class being tested:

namespace StrykerPlayground.Core;

public static class ExampleClass
{
    public static bool IsPositive1(int i)
    {
        if (i > 0)
        {
            return true;
        }
        else
        {
            // This will report a false positive, because it's equivalent to 'return default;'
            return false;
        }
    }

    public static bool IsPositive2(int i)
    {
        if (i > 0)
        {
            return true;
        }

        // This will not report a false positive, as it's not a block.
        return false;
    }

    public static bool IsNotPositive(int i)
    {
        if (i > 0)
        {
            // This will also report a false positive, as replacing it with an empty block
            // will still end up returning default (which is equivalent to false).
            return false;
        }
        else
        {
            return true;
        }
    }
}

Tests:

using StrykerPlayground.Core;
using Xunit;

namespace StrykerPlayground;

public class ExampleTest
{
    [Theory]
    [InlineData(1, true)]
    [InlineData(0, false)]
    public void Test1(int input, bool expected) =>
        Assert.Equal(expected, ExampleClass.IsPositive1(input));

    [Theory]
    [InlineData(1, true)]
    [InlineData(0, false)]
    public void Test2(int input, bool expected) =>
        Assert.Equal(expected, ExampleClass.IsPositive2(input));

    [Theory]
    [InlineData(1, false)]
    [InlineData(0, true)]
    public void Test3(int input, bool expected) =>
        Assert.Equal(expected, ExampleClass.IsNotPositive(input));
}

Expected behavior

I would expect blocks only returning a default value to not be mutated. However, I realise that not all default values (e.g. in case of enums or structs) can be detected on the source code level.

[sbergen]

As some extra context: I like to write those else clauses explicitly, because it helps the compiler be more efficient. Consider the following scenario.

I have the following code:

bool SomeMethod()
{
  if (something)
  {
    DoSomething();
    return false; // <-- and I remove this line *accidentally* while I'm working on the code
  }
  else
  {
    return true;
  }
}

in this case it will produce a compiler error.

If I had

bool SomeMethod()
{
  if (something)
  {
    DoSomething();
    return false; // <-- and I remove this line *accidentally* while I'm working on the code
  }

  return true;
}

then the compiler will not complain and the method will return true, which was not intentional.

[dupdob]

Thanks for taking such care for this issue. TLDR; I don't see any short term possible improvement for this. But there is an alternative solution. Clarifications:

• These are indeed examples of semantically equivalent mutants and those arise in many other situations. Semantically equivalent mutant are mutations that do not alter the behavior of the mutated code.
• Stryker makes no claim of eliminating those.
• Stryker tries to eliminate redondant mutations (i.e. mutator resulting in same mutation)

You provide a logical analysis here, but this is not how the mutation engine works:

• The block mutator provides a mutated (and empty) block whenever there is a non empty block in the code.
• The overall mutation orchestration engine ensures the mutated code compiles dealing with normal flow controlled elicitation (e.g. no return after a while loop without any break in it, ensuring that out variables are defined in every possible paths...).
• To do that, it adds out variables initialization (to default) at the beginning of the method and a return (default) at the end of the method (if there is none and the method is non void).

There is an official way to deal with semantically equivalent mutant: Stryker comments. You need to add: Stryker disable once block before

// Stryker disable once block
{
    return false;
}

And voilà

[sbergen]

• These are indeed examples of semantically equivalent mutants and those arise in many other situations. Semantically equivalent mutant are mutations that do not alter the behavior of the mutated code.
• Stryker makes no claim of eliminating those.
• Stryker tries to eliminate redondant mutations (i.e. mutator resulting in same mutation)

If Stryker doesn't claim to eliminate equivalent mutants, then this is not a bug, that's a fair point!

Also, I understand your point about semantically equivalent mutants being a different category than redundant mutations. However, aren't semantically equivalent mutants actually worse from the end user's perspective? With e.g. two redundant mutants, you'd have a longer runtime, and either 0 or 2 survived mutants, where as with a semantically equivalent mutant, you have a longer runtime and also always one false positive. Of course if you had multiple redundant mutants, then the runtime impact would be larger.

You provide a logical analysis here, but this is not how the mutation engine works:

• The block mutator provides a mutated (and empty) block whenever there is a non empty block in the code.
• The overall mutation orchestration engine ensures the mutated code compiles dealing with normal flow controlled elicitation (e.g. no return after a while loop without any break in it, ensuring that out variables are defined in every possible paths...).
• To do that, it adds out variables initialization (to default) at the beginning of the method and a return (default) at the end of the method (if there is none and the method is non void).

I'm actually pretty familiar with how the mutator works, as I fixed some issues in an early version of it :) My comments were just the high level conclusions, considering the current state of the whole system, not just the mutator itself.

I get the division of concerns here from an architectural perspective, but if we want to build the best possible tool, I think we'll have to do some assumptions about how orchestration works in the mutators also (even though in an ideal world/architecture you wouldn't).

There is an official way to deal with semantically equivalent mutant: Stryker comments. You need to add: `Stryker disable once block' before

// Stryker disable once block
{
    return false;
}

And voilà

I still consider this a workaround. Couldn't we do better?

Like I mentioned in my starting post, it would be impossible to detect e.g. default enum and struct values, and implicit conversions also make things complicated. But couldn't we still eliminate block mutations that only contain

• return default
• return null or
• return 0 if the return type is a numeric type?

[dupdob]

Again, semantically equivalent mutants are unavoidable, as long as there are several ways to write the same piece of code. Granted, it would be nice to avoid the most obvious ones; but this also means that Stryker comments are needed (albeit sadly, I concur); so this is not a workaround, but a needed features for users who want/need 100% killing.

Yes we can filter (top level) blocks that contains only some form of return default, but bear in mind this will also result in discarding valid mutations:

bool SomeMethod()
{
  if (!something)
  {
    return false;
  }  
  DoSomething();
  return true;
}

So this is a fair feature request, but I feel this is difficult to get right for a sufficient number of situations, at least with the current state of the engine. It requires effort to have some basic path/scope analysis; I have been toying with is idea for some time now, as it would improve block mutation and open doors to other interesting mutations, for sure; but it also looks like a can of worms.

Just to be crystal clear: this is a good feature request and if someone proposes a PR implementing this (eliminating some/all semantically equivalent mutations without dropping too many valid ones), it would be progress.