Stryker comments inside multiline comments or at the ends of lines of code are ignored

[nwalker-et]

Describe the bug I have a method which returns null from an else-if block. Because Stryker has the method return default when the else-if block is removed, the behavior under that mutation is identical to the expected behavior (i.e., the method returns null), so the mutant can't be killed. This issue is described in #2000. In that issue @dupdob suggests using // Stryker disable once block to ignore the mutant. However, I ran into some challenges getting this to work with my company's coding style.

Because our style is to put curly braces on the same line as an if/else-keyword, I would like to be able to disable the Block mutation in this manner:

  public string? ReturnDefaultFromElseIf(bool? value) {
    if (value == true) {
      return TrueStr;
    } else if (value == null) /* Stryker disable once Block */ {
      return null;
    } else {
      return FalseStr;
    }
  }

However, Stryker comments that appear in multiline comments, or that appear after/amidst code on the same line, appear to be disregarded. Additionally, if a "disable once" comment appears anywhere before the if keyword, then the mutant is disabled for the last else block as well (since its syntax is a child of the if-statement), unless I have a second comment to restore it. I made various attempts to get Stryker to do what I wanted, which you can read through in the source code below.

DisableBlockIssueDemo.cs

```csharp namespace LibraryProject; public class DisableBlockIssueDemo { private const string TrueStr = "True"; private const string FalseStr = "False"; /* * Here is a method similar to the one I am trying to work with. */ public string? ReturnDefaultFromElseIf(bool? value) { if (value == true) { return TrueStr; } else if (value == null) { return null; } else { return FalseStr; } } /* * In general, for the examples below, my expectation is that Stryker would ignore * the Block mutation for the else-if block but not the else block. * (Though this expectation is stronger for some of these examples than others.) */ // Does not work - Comment is out of scope. public string? ReturnDefaultFromElseIf1(bool? value) { if (value == true) { return TrueStr; // Stryker disable once Block } else if (value == null) { return null; } else { return FalseStr; } } // Does not work - Stryker ignores multiline comments. public string? ReturnDefaultFromElseIf2(bool? value) { if (value == true) { return TrueStr; } /* Stryker disable once Block */ else if (value == null) { return null; } else { return FalseStr; } } // Does not work - Stryker ignores multiline comments. public string? ReturnDefaultFromElseIf3(bool? value) { if (value == true) { return TrueStr; } else /* Stryker disable once Block */ if (value == null) { return null; } else { return FalseStr; } } // Does not work - Stryker ignores multiline comments. // (This is the approach that I think would be the easiest to have Stryker work with, while being reasonably readable.) public string? ReturnDefaultFromElseIf4(bool? value) { if (value == true) { return TrueStr; } else if (value == null) /* Stryker disable once Block */ { return null; } else { return FalseStr; } } // Does not work - Comment appears after start of block. public string? ReturnDefaultFromElseIf5(bool? value) { if (value == true) { return TrueStr; } else if (value == null) { // Stryker disable once Block return null; } else { return FalseStr; } } // Does not work - Stryker ignores comments in trailing trivia (see Visual Studio's Syntax Visualizer). public string? ReturnDefaultFromElseIf6(bool? value) { if (value == true) { return TrueStr; } else if (value == null) // Stryker disable once Block { return null; } else { return FalseStr; } } // WORKS, but separates the curly brace from the if condition. public string? ReturnDefaultFromElseIf7(bool? value) { if (value == true) { return TrueStr; } else if (value == null) // Stryker disable once Block { return null; } else { return FalseStr; } } // Does not work - Stryker disables the mutant for both the else-if block and the else block. // (This is because it is a child of the if-statement; see Visual Studio's Syntax Visualizer) public string? ReturnDefaultFromElseIf8(bool? value) { if (value == true) { return TrueStr; } else // Stryker disable once Block if (value == null) { return null; } else { return FalseStr; } } // Does not work - Stryker ignores comments in trailing trivia. public string? ReturnDefaultFromElseIf9(bool? value) { if (value == true) { return TrueStr; } // Stryker disable once Block else if (value == null) { return null; } else { return FalseStr; } } // Does not work - Stryker disables the mutant for both the else-if block and the else block. public string? ReturnDefaultFromElseIf10(bool? value) { if (value == true) { return TrueStr; } // Stryker disable once Block else if (value == null) { return null; } else { return FalseStr; } } /* * The examples below try to mitigate the fact that Stryker still * disables the mutation for the else block if the disable comment * appears before the "if" keyword of the else-if portion. */ // Does not work - Restore comment is outside the scope of the else block. public string? ReturnDefaultFromElseIf11(bool? value) { if (value == true) { return TrueStr; } // Stryker disable once Block else if (value == null) { return null; // Stryker restore Block } else { return FalseStr; } } // Does not work - Stryker ignores comments in trailing trivia. public string? ReturnDefaultFromElseIf12(bool? value) { if (value == true) { return TrueStr; } // Stryker disable once Block else if (value == null) { return null; } // Stryker restore Block else { return FalseStr; } } // WORKS, but separates the else keywords from the curly braces. // (This is the least ugly solution I've found so far.) public string? ReturnDefaultFromElseIf13(bool? value) { if (value == true) { return TrueStr; } // Stryker disable once Block else if (value == null) { return null; } // Stryker restore Block else { return FalseStr; } } } ```

DisableBlockIssueDemoTests.cs

```csharp namespace LibraryProject.Test; public class DisableBlockIssueDemoTests { private DisableBlockIssueDemo exampleClass = new DisableBlockIssueDemo(); public static IEnumerable ReturnDefaultFromElseIfInputs => [ [true, "True"], //[false, "False"], // For demonstration purposes: Disabled so that mutations on else-branches survive. [null, null] ]; [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf1_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf1(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf2_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf2(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf3_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf3(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf4_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf4(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf5_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf5(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf6_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf6(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf7_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf7(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf8_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf8(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf9_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf9(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf10_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf10(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf11_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf11(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf12_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf12(input)); } [Theory, MemberData(nameof(ReturnDefaultFromElseIfInputs))] public void ReturnDefaultFromElseIf13_ReturnsCorrectValue(bool? input, string? expectedOutput) { Assert.Equal(expectedOutput, exampleClass.ReturnDefaultFromElseIf13(input)); } } ```

Logs log-20240808.txt

How Stryker treats a couple of the approaches shown in the source code above:

Expected behavior Stryker comments that appear in multiline comments, or that appear after some code on the same line, should be applied the same as comments that appear on a line of their own.

Desktop:

• OS: Windows
• Type of project: Microsoft.NET.Sdk
• Framework Version: net8.0
• Stryker Version: 4.1.0

Additional context Looking at the Stryker source code, the issue appears to be two-fold.

First, while multi-line comments are included in the comment search, it is impossible for them to match the regex pattern that is used to detect Stryker comments, since that pattern only accepts comments that start with //.

Second, only comments from leading trivia are considered. As such, any comments that appear as trailing trivia (e.g., amid code or at the end of a line after some code) will not be included. In theory, I think it should be possible to include the trailing trivia of the prior SyntaxToken in addition to the leading trivia of the SyntaxNode in question (see the implementation of SyntaxNode.GetLeadingTrivia, SyntaxNode.GetFirstToken, and SyntaxToken.GetPreviousToken).

It sounds like at some point there was a deliberate decision to not support multiline comments. However, it sounds like that decision was in part due to the trailing/leading trivia issue, which I believe is surmountable (see previous paragraph).

[dupdob]

Thanks for your detailed reporting and analysis. We elected not to support tailing comments because they would be the source of ambiguity.

if (condition)
{
...//block 1
} // Stryker disable once block
else
{
.. //block 2
} 

Should the comment apply to block 1 or 2? Same question here

if (condition)
{
...//block 1
} 
else // Stryker disable once block
{
.. //block 2
} 

Here, should it apply to block 2 only, or both?

if (condition)  // Stryker disable once block
{
...//block 1
} 
else
{
.. //block 2
} 

And there ?

if (condition)  /* Stryker disable once block */ {...block 1} else {.. block 2} 

This is the kind of questions we are happy to have avoided so far... 😄 .

As a conclusion, I understand your point of view, and look into what could be done. But I am not very optimistic about this.

[nwalker-et]

@dupdob In my opinion, it feels intuitive that Stryker comments would only apply to the code after them, so in the case of your first two examples, I would assume that just block 2 would be affected. As long as the documentation made it clear that Stryker comments only apply to the code after them, I don't think those two situations would be an issue. Your third and fourth examples, though, do introduce a bit more ambiguity. Still, I feel that similar ambiguity exists with the current setup due to the way Stryker comments are implemented. Namely, I would not have expected these two configurations to yield different results:

if (condition) {
...//block 1
}
// Stryker disable once block
else if (condition) {
...//block 2
} else {
...//block 3
}

if (condition) {
...//block 1
} else if (condition)
// Stryker disable once block
{
...//block 2
} else {
...//block 3
}

Yet, because of how comments are tied to the syntax tree, and how else blocks are children of if-statements, the first example ends up disabling the mutation for both blocks 2 and 3, while the second only applies to block 2.

The issue is that the second example does what I am trying to achieve, but under my company's coding style, it looks worse than the first example, while also not being obvious to those unfamiliar with Stryker's implementation why it would behave any different. Being able to use a multi-line comment before the curly brace would at least solve the ugliness issue, though I admit it doesn't solve the broader issue of these minor placement differences having such a substantial impact on behavior.

I guess the word "once" to me implies that the comment should only affect one block, not one syntax node and all its children. The structure of the syntax tree isn't always immediately obvious just looking at one's source code, so the current implementation leads to some strange behavior like in my example above. It doesn't help that the current documentation refers to Stryker comments applying to lines, whereas the actual behavior is less about lines and more about invisible syntax nodes.

I'm not sure what the fix for these broader issues would be, but being able to use inline comments would at least help with code style, and given Stryker's behavior with my examples here, I don't feel like it introduces much more ambiguity than we already have.

[dupdob]

Update: I am working on this. It proves as hard as expected. The main problem is that multiline comments are linked to the nearest token, while Stryker (sensibly) parses syntax nodes. This makes detecting them and associating them to a scope difficult, except in some basic situations (such as syntax blocks). Meaning, most of them are simply ignored.

I am looking for a better detection logic...

[dupdob]

I just opened PR #3028 that should cover this. I made some tests but did not test all provided construct.

Here is what you should get (using your original code sample)

namespace LibraryProject;

public class DisableBlockIssueDemo {
  private const string TrueStr = "True";
  private const string FalseStr = "False";

  /*
   * Here is a method similar to the one I am trying to work with.
   */

  public string? ReturnDefaultFromElseIf(bool? value) {
    if (value == true) {
      return TrueStr;
    } else if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Does not work - Comment is out of scope.
  public string? ReturnDefaultFromElseIf1(bool? value) {
    if (value == true) {
      return TrueStr;
    // Stryker disable once Block
    } else if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Will disable vlock mutation for the whole  if (value == null)... statement
  public string? ReturnDefaultFromElseIf2(bool? value) {
    if (value == true) {
      return TrueStr;
    } /* Stryker disable once Block */ else if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Should work: comment will be associated with `if` statement
  public string? ReturnDefaultFromElseIf3(bool? value) {
    if (value == true) {
      return TrueStr;
    } else /* Stryker disable once Block */ if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Will work: Stryker comment will apply to the whole block
    public string? ReturnDefaultFromElseIf4(bool? value) {
    if (value == true) {
      return TrueStr;
    } else if (value == null) /* Stryker disable once Block */ {
      return null;
    } else {
      return FalseStr;
    }
  }

 // Does not work as expected: comment will be associated to first statement within block, it will have no effect since there will be no block mutation
  public string? ReturnDefaultFromElseIf5(bool? value) {
    if (value == true) {
      return TrueStr;
    } else if (value == null) { // Stryker disable once Block
      return null;
    } else {
      return FalseStr;
    }
  }

  // Did not test explicitly. Should work now
  public string? ReturnDefaultFromElseIf6(bool? value) {
    if (value == true) {
      return TrueStr;
    } else if (value == null) // Stryker disable once Block
    {
      return null;
    } else {
      return FalseStr;
    }
  }

  // WORKS, but separates the curly brace from the if condition.
  public string? ReturnDefaultFromElseIf7(bool? value) {
    if (value == true) {
      return TrueStr;
    } else if (value == null)
    // Stryker disable once Block
    {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Works as documented: `once` apply to the whole `if` statement
  public string? ReturnDefaultFromElseIf8(bool? value) {
    if (value == true) {
      return TrueStr;
    } else
    // Stryker disable once Block
    if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Works: scope will be the whole if (value == null) statement
  public string? ReturnDefaultFromElseIf9(bool? value) {
    if (value == true) {
      return TrueStr;
    } // Stryker disable once Block
    else if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  // Works as documented
  public string? ReturnDefaultFromElseIf10(bool? value) {
    if (value == true) {
      return TrueStr;
    }
    // Stryker disable once Block
    else if (value == null) {
      return null;
    } else {
      return FalseStr;
    }
  }

  /*

  // will not work as intended: disable once apply to the whole if statement. Restore does not apply to anything
  public string? ReturnDefaultFromElseIf11(bool? value) {
    if (value == true) {
      return TrueStr;
    }
    // Stryker disable once Block
    else if (value == null) {
      return null;
    // Stryker restore Block
    } else {
      return FalseStr;
    }
  }

  // should work as expected, not tested
  public string? ReturnDefaultFromElseIf12(bool? value) {
    if (value == true) {
      return TrueStr;
    }
    // Stryker disable once Block
    else if (value == null) {
      return null;
    } // Stryker restore Block
    else {
      return FalseStr;
    }
  }

  // WORKS, but separates the else keywords from the curly braces.
  // (This is the least ugly solution I've found so far.)
  public string? ReturnDefaultFromElseIf13(bool? value) {
    if (value == true) {
      return TrueStr;
    }
    // Stryker disable once Block
    else if (value == null) {
      return null;
    }
    // Stryker restore Block
    else {
      return FalseStr;
    }
  }
}