BUG: CC0118 - Unnecessary '.ToString()' call in string concatenation

[kindermannhubert]

It is little bit more complicated while concatenating 'tostringed' objects only.

Example:

class C
{
    string M() => 1 + 2.ToString();
}

CC0118 is detected and code after fix looks like this:

class C
{
    string M() => 1 + 2;
}

which is incorrect.

Solution 1: Do not detect CC0118 in such situations.
Solution 2: Fix needs to be smarter.

This is problematic primarily when using solution-wide fix because of possibly large number of wrong fixes.

[AdamSpeight2008]

Would result in 3 being returned rather than 12

[giggio]

Yes, this is a bug. It is confirmed. We should not offer a diagnostic in this scenario, which is your option 1.

[MaStr11]

The underlying problem is the overload resolution of the + operator as described in the spec chapters Binary operator overload resolution and Addition operator.

The overload resolution is quite complicated and involves user defined operators, lifting, implicit conversions, delegate combination and enumeration addition.

The following code shows some of the problems:

using System;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Addition has precedence over string concatination:");
            Console.WriteLine($"1 + 2.ToString()                         : { 1 + 2.ToString() } ");
            Console.WriteLine($"1 + 2                                    : { 1 + 2} ");

            Console.WriteLine("\nDelegate combination has precedence over string concatination:");
            var ea1 = new EventHandler((o, e) => { });
            var ea2 = new EventHandler((o, e) => { });
            Console.WriteLine($"ea1 + ea2.ToString()                     : { ea1 + ea2.ToString() }");
            Console.WriteLine($"ea1 + ea2                                : { ea1 + ea2}");

            Console.WriteLine("\nUser defined operator has precedence over string concatination:");
            var p1 = new Point { X = 1, Y = 2 };
            var p2 = new Point { X = 100, Y = 200 };
            Console.WriteLine($"p1 + p2.ToString()                       : { p1 + p2.ToString() }");
            Console.WriteLine($"p1 + p2                                  : { p1 + p2 }");

            Console.WriteLine("\nCompilergenerated enum operator has precedence over string concatination:");
            Console.WriteLine($"AttributeTargets.Assembly + 1.ToString() : { AttributeTargets.Assembly + 1.ToString() }");
            Console.WriteLine($"AttributeTargets.Assembly + 1            : { AttributeTargets.Assembly + 1 }");
            Console.ReadLine();
        }
    }

    public class Point
    {
        public int X;
        public int Y;
        public override string ToString() => $"X={X}, Y={Y}";

        public static Point operator +(Point a, Point b) => new Point { X = a.X + b.X, Y = a.Y + b.Y };
    }
}

It produces the following output:

Addition has precedence over string concatination:                                                                                                                                                                                
1 + 2.ToString()                         : 12                                                                                                                                                                                     
1 + 2                                    : 3                                                                                                                                                                                      
                                                                                                                                                                                                                                  
Delegate combination has precedence over string concatination:                                                                                                                                                                  
ea1 + ea2.ToString()                     : System.EventHandlerSystem.EventHandler                                                                                                                                                 
ea1 + ea2                                : System.EventHandler                                                                                                                                                                    
                                                                                                                                                                                                                                  
User defined operator has precedence over string concatination:                                                                                                                                                                   
p1 + p2.ToString()                       : X=1, Y=2X=100, Y=200                                                                                                                                                                   
p1 + p2                                  : X=101, Y=202                                                                                                                                                                           
                                                                                                                                                                                                                                  
Compilergenerated enum operator has precedence over string concatination:                                                                                                                                                         
AttributeTargets.Assembly + 1.ToString() : Assembly1                                                                                                                                                                              
AttributeTargets.Assembly + 1            : Module                         

Look here to see in action.

It is hard to predict the consequences of the removal of the ToString() method in the analyzer.

In the CodeFix it is possible to check whether the refactoring changed the meaning of the code by checking the SymbolInfo of the AddExpression before and after the refactoring like this:

    var semModel = await document.GetSemanticModelAsync();
    // pick the symbolinfo of the addexpression. The symbol info points to 
    // Method System.String System.String.op_Addition(System.Object left, System.String right)
    var sInfo = semModel.GetSymbolInfo(addExpression);
    // refactor the code
    // recalcuate the semantic Model and get the symbolInfo of the addexpression for the refactored code
    var sInfoAfter = semModel.GetSymbolInfo(addExpressionAfter);
    // sInfoAfter should point als to one of the System.String.op_Addition overloads
    // In the cases above it points to
    // Method System.Int32 System.Int32.op_Addition(System.Int32 left, System.Int32 right)
    // Method System.EventHandler System.EventHandler.op_Addition(System.EventHandler left, System.EventHandler right)
    // Method ConsoleApplication1.Point ConsoleApplication1.Point.op_Addition(ConsoleApplication1.Point a, ConsoleApplication1.Point b)
    // Method System.AttributeTargets System.AttributeTargets.op_Addition(System.AttributeTargets left, System.Int32 right)

I think it is easier to go for option 2 and check whether the refactoring leads the use of an different + operator method.

@giggio If you like I could create a pull request with tests and changes in the UnnecessaryToStringInStringConcatenationCodeFixProvider.

[giggio]

@MaStr11 Your comment makes sense. But we cannot offer a diagnostic if it does not make sense, otherwise it will be hanging there forever, annoying the user. If the call to ToString actually is necessary, we should not offer an diagnostic. If we do, the code fix will need to fix it somehow. Or, in the worst scenario, not fix it, but let the user fix it and make it better.
We can do some semantic analysis on the analyzer, but we cannot project future semantics, which is an asynchronous call, that can only be done on the code fix.
Unless we can move this forward somehow, I am thinking we should maybe remove this analyzer.

[MaStr11]

I see three options here:

1. Detection in the Fix phase (as described above)

If a false positive is detected we could use a #pragma warning disable (and make an explanatory comment) to disable the diagnostic.

2. Detection during the Analyzer phase

We could try to predict the outcome of the Fix by looking up of the existing op_Addition overloads of the types on both sides of the + operator. Then we could try to predict whether an other overload might take precedence. I have no idea whether this will work but this might be worth a try.

3. Remove the Analyzer

[giggio]

@MaStr11 Option 2 will demand a careful implementation, but I think it might work, and is the best option by far (if it works). You want to try it out?

[giggio]

This is a fix in a new analyzer, so we I corrected the milestone.