feat(mutator): Add string method mutators

[ruudschouten]

Added mutators for string methods. Closes #2868

[dupdob]

Thanks for this PR. This is a great stab at this. I still have a few general remarks:

1. It seems to me that mutation logic could be made more generic, instead of having one method per method switch
2. Same with tests
3. Please use .Withxxx methods instead of creating a new syntax node from scratch as it secures future compatibility and preserves a lot of information. In this case, I guess you should be using node.WithName(SyntaxFactory.IdentifierName(...))
4. ElementAt and ElementAtOrDefault returns a char, hence should not be replaced by string.Empty.
5. Looking at the methods considered, I am not sure asserting the expression type is necessary. In any case, the mutator should accept not having semantic model available, as Stryker may not be able to always generate it properly.

[ruudschouten]

Thanks for the remarks! I resolved 1,2 and 4, and had some questions for the other remarks.

I'm not entirely sure what you mean with remark 3. Where should I be using this? Same goes partially for remark 5;

Looking at the methods considered, I am not sure asserting the expression type is necessary.

Ar for the rest of remark 5

In any case, the mutator should accept not having semantic model available, as Stryker may not be able to always generate it properly.

I used the semantic model, since that's a way to get the type of a member on which the method is being called. I thought I needed this since SharpLab showed me this syntaxtree;

I am unaware of any other ways do to this, but I did try go get to the type of the member by looking through different Expression types and couldn't find any other way.

[dupdob]

Thanks for the improved version. About 3, It turns out that most mutators were written before the switch to the .Withxxx(...) patterns, so I lack a good example. So here is the pattern you use

new()
  {
    OriginalNode = node,
      ReplacementNode = node.ReplaceNode(
                member.Name,
                SyntaxFactory.IdentifierName(replacement)
  )            

I think it would be improved this way:

new()
  {
    OriginalNode = node,
      ReplacementNode = node.WithName(
                SyntaxFactory.IdentifierName(replacement))
  )       

I see two benefits:

1. Explicit the mutation: use a new 'name'
2. Keep everything else from the original node, including source line information for example

About 5: The first thing to keep in mind is that Stryker has been designed to deal with mutations resulting in compilation errors. We try to limit these, but we know that it is impossible to ensure that no mutation will trigger a compilation error. So Stryker is able to remove (roll back) mutations until the mutated code compile. So , if you do not check for the string type, the mutator may mutate something that is not a string expression. For example, if a class implements a PadRight(...) method, your mutator will generate a mutation with a call to PadLeft(...). Then either this class does not implement a 'PadLeft(...)method (with the same signature); the mutation will then result in a compilation error that will result in the mutation being flagged as such. And if the class does implement aPadLeft(...)` method and the mutations will be tested.

Either way, this seems like a good mutation to try.

Is it clear?

[ruudschouten]

I got 3 now, I was a bit confused since I thought the WithName method would be on the node parameter, but it was on the member one. As for 5, no not really.

So , if you do not check for the string type, the mutator may mutate something that is not a string expression.

Does this not check if the expression was called on a string type?

private static bool IsOperationOnAString(ExpressionSyntax expression, SemanticModel model)
{
    var typeInfo = model.GetTypeInfo(expression);
    return typeInfo.Type?.SpecialType == SpecialType.System_String;
}

I did try what you said with the PadLeft method;

[Fact]
public void ShouldNotMutateMethodsWithSameName()
{
    var syntaxTree = CSharpSyntaxTree.ParseText(
        """
        class Padder {
            public string PadLeft(string org) => " " + org;
        }

        class TestClass {
            private Padder padder = new Padder();

            void TestMethod() {
                  var padded = padder.PadLeft("test");
            }
        }
        """);
    var compilation = CSharpCompilation.Create("TestAssembly")
        .WithOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary))
        .AddReferences(MetadataReference.CreateFromFile(typeof(object).Assembly.Location))
        .AddSyntaxTrees(syntaxTree);
    var semanticModel = compilation.GetSemanticModel(syntaxTree);
    var expression = syntaxTree.GetRoot().DescendantNodes().OfType<InvocationExpressionSyntax>().First();

    var target = new StringMethodMutator();
    var result = target.ApplyMutations(expression, semanticModel).ToList();

    result.ShouldBeEmpty();
}

And this test passes.

[richardwerkman]

I'd like to see at least a few test cases in CsharpMutantOrchestratorTests that show the mutation is placed as expected. Especially the Trim() and SubString() mutations could be placed in a weird way.

Our integration tests also won't pick this up.

[richardwerkman]

A good test would also be to add this line to the integration test project: "test".ToUpper().Trim().PadLeft(2).Substring(2).ElementAt(2); that would make sure everything compiles like expected.