[Proposal] Change order of evaluation for string concatenation to follow C# specification

[sharwell]

During evaluation for #415, a situation was identified where the C# compiler does not strictly adhere to the C# language specification. I am proposing altering the behavior of the Roslyn C# compiler in this area to instead follow the C# specification. It is not clear to me when the bug was originally introduced; it may go back all the way to the C# 1.0 compiler.

Affected source code

The behavior under consideration here is string concatenation which meets both of the following conditions:

1. The string concatenation operation involves the concatenation of three or more expressions.
2. The static type of at least one of the terms in the concatenation expression is not implicitly convertible to string.

   Description of deviation from specification

The C# specification describes the behavior of string concatenation in terms of a left-associative sequence of calls to the following methods:

string operator +(string x, string y);
string operator +(string x, object y);
string operator +(object x, string y);

For example, in the underlying bytecode, the expression "x" + y + z should appear as the following:

// semantics according to specification:
op_Addition(op_Addition("x", y), z)

Based on the defined order of evaluation for calls to methods in C# and semantics for string concatenation, it is clear that the expression y above is evaluated all the way to the final string result prior to any evaluation of z. This order of evaluation is:

1. "x" is evaluated to a string
2. y is evaluated to an object
3. If the result of (2) is not null, its ToString method is called
4. z is evaluated to an object
5. If the result of (4) is not null, its ToString method is called

The C# compiler currently deviates from this behavior. Rather than making two calls to op_Addition, the generated bytecode contains a single call to string.Concat:

// semantics according to compiler:
string.Concat("x", y, z)

It is critical to note that the C# compiler does not call ToString on y or z prior to passing them as arguments to Concat. The semantics are now:

1. "x" is evaluated to a string
2. y is evaluated to an object
3. z is evaluated to an object
4. If the result of (2) is not null, its ToString method is called
5. If the result of (3) is not null, its ToString method is called

The deviation from the specification lies in the reordering of the expression evaluation and the calls to ToString. This reordering can produce observable behavior that does not adhere to the C# specification in the following situations (ordered from highest to lowest likelihood of incidence, based on nothing but my opinion):

1. The evaluation of ToString on the result of evaluating y throws an exception, and the evaluation of z produces side effects (i.e. the evaluation of z shouldn't have occurred, but it does occur here due to the reordering).
2. The evaluation of expression z has side effects that affect the ToString operation in step 4 (e.g. evaluation of z changes the current culture, which affects the ToString operation).
3. The evaluation of ToString on the result of evaluating y has side effects which affect the evaluation of z (i.e. the side effects should have occurred prior to evaluating z, but instead they occur afterwards).

   Argument against altering the current behavior

Especially due to the amount of time this bug has been present in the compiler, it is possible that real-world code exists which relies (intentionally or unintentionally) on the current behavior.

Argument in favor of altering the behavior

• The semantics defined by the C# specification allow the compiler to avoid boxing and certain other memory allocations at runtime (see #415). These boxing operations have already been identified in several key code paths in the .NET Core and Roslyn projects, and providing functionality in the compiler to avoid the boxing will provide an automatic performance benefit to many .NET applications.
• The behavior change is certainly less likely to manifest itself in real code that other accepted changes like avoiding placing certain local variables in the state machines generated for iterators and async methods.
• The "most likely situation" identified above is not likely to appear in the "correct" execution path of a program, due to throwing an exception. Furthermore, even in this case it's unlikely that a program would rely on the side effect(s) of evaluating z.

[VladimirReshetnikov]

I think the condition

The static type of at least one of the terms in the concatenation expression is not string.

should be changed to

The static type of at least one of the terms in the concatenation expression is not implicitly convertible to string.

I also think the task #439 should be completed before any work on this bug.

[sharwell]

@VladimirReshetnikov I updated that condition. I'm also surprised you only found one issue with this. Perhaps you are just getting started? :smile_cat:

[gafter]

tl;dr version:

The compiler currently optimizes string concatenation in a way that doesn't follow the language spec. This issue proposes to follow the spec, thereby breaking compatibility. Code that would be broken by this change probably does not exist in the wild.

[theoy]

We are reluctant to make a change without fully understanding the compat story.

[canton7]

TL;DR: I searched GitHub, found one file that would be affected (excluding homework).

I ran a very crude query in Google BigData:

SELECT  
  sample_path, sample_repo_name, CONCAT('https://github.com/', sample_repo_name, '/blob/master/', sample_path) AS url, content
FROM `fh-bigquery.github_extracts.contents_net_cs`
WHERE 
  STRPOS(sample_repo_name, 'corefx') = 0
  AND STRPOS(sample_repo_name, 'roslyn') = 0
  AND STRPOS(sample_repo_name, 'coreclr') = 0
  AND REGEXP_CONTAINS(content, '(?s)override\\s+string\\s+ToString\\s*().+Current(UI)?Culture\\s*=')
LIMIT 500

That brought in a modest 133 matches.

I downloaded the results, and threw them at Roslyn:

public class Model
{
    public string Url { get; set; }
    public string Content { get; set; }
}

...

int i = 0;
foreach (var line in File.ReadLines(@"<path>.json"))
{
    var result = JsonConvert.DeserializeObject<Model>(line);
    var tree = CSharpSyntaxTree.ParseText(result.Content);
    var root = (CompilationUnitSyntax)tree.GetRoot();
    var toStringMethods = root.DescendantNodes().OfType<MethodDeclarationSyntax>()
        .Where(x => x.Identifier.ValueText == "ToString");

    foreach (var toStringMethod in toStringMethods)
    {
        bool setsCulture = toStringMethod.DescendantNodes().OfType<AssignmentExpressionSyntax>()
            .Where(x => x.Left is MemberAccessExpressionSyntax access && (access.Name.Identifier.Text == "CurrentCulture" || access.Name.Identifier.Text == "CurrentUICulture"))
            .Any();

        if (setsCulture)
        {
            Console.WriteLine(new string('=', 40));
            Console.WriteLine(result.Url);
            Console.WriteLine(toStringMethod.ToString());
        }
    }

    i++;
}

With the results:

================================================================================
https://github.com/nevadascout/dash-core/blob/master/FastColoredTextBox/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/vip32/Xtricate/blob/master/src/Xtricate.Templ/Template.cs
public override string ToString()
        {
            if (Culture == null || Thread.CurrentThread.CurrentCulture.Equals(Culture))
                return TransformText(null);
            var culture = Thread.CurrentThread.CurrentCulture;
            var uiculture = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentCulture = Culture;
            Thread.CurrentThread.CurrentUICulture = Culture;
            var result = TransformText(null);
            Thread.CurrentThread.CurrentCulture = culture;
            Thread.CurrentThread.CurrentUICulture = uiculture;
            return Regex.Replace(result, @"^\s+$[\r\n]*", "", RegexOptions.Multiline); // remove empty lines
        }
================================================================================
https://github.com/xieguigang/MathLab/blob/master/components/TextEditor/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/baijioss/BaijiGenerator.Net/blob/master/Lib/FastColoredTextBox/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/samarjeet27/ynoteclassic3/blob/master/Code/FastColoredTextBox/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/samarjeet27/ynoteclassic/blob/master/Code/FastColoredTextBox/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            var sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}\r\n ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/vip32/Xtricate/blob/master/src/Xtricate.Core.Templ/Template.cs
public override string ToString()
        {
#if NET45
            if (Culture == null || Thread.CurrentThread.CurrentCulture.Equals(Culture))
                return TransformText(null);
#elif NET46
            if (Culture == null || Thread.CurrentThread.CurrentCulture.Equals(Culture))
                return TransformText(null);
#else
            if (Culture == null || CultureInfo.CurrentCulture.Equals(Culture))
                return TransformText(null);
#endif

#if NET45
            var culture = Thread.CurrentThread.CurrentCulture;
            var uiculture = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentCulture = Culture;
            Thread.CurrentThread.CurrentUICulture = Culture;
#elif NET46
            var culture = Thread.CurrentThread.CurrentCulture;
            var uiculture = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentCulture = Culture;
            Thread.CurrentThread.CurrentUICulture = Culture;
#else
            var culture = CultureInfo.CurrentCulture;
            var uiculture = CultureInfo.CurrentUICulture;
            CultureInfo.CurrentCulture = Culture;
            CultureInfo.CurrentUICulture = Culture;
#endif

            var result = TransformText(null);
#if NET45
            Thread.CurrentThread.CurrentCulture = culture;
            Thread.CurrentThread.CurrentUICulture = uiculture;
#elif NET46
            Thread.CurrentThread.CurrentCulture = culture;
            Thread.CurrentThread.CurrentUICulture = uiculture;
#else
            CultureInfo.CurrentCulture = culture;
            CultureInfo.CurrentUICulture = uiculture;
#endif
            return Regex.Replace(result, @"^\s+$[\r\n]*", "", RegexOptions.Multiline); // remove empty lines
        }
================================================================================
https://github.com/JannikArndt/Canal/blob/master/FastColoredTextBox/HotkeysMapping.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/furesoft/Creek/blob/master/Creek.UI/FastColoredTextBox/Hotkeys.cs
public override string ToString()
        {
            CultureInfo cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            var sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/andibadra/ANDT/blob/master/Andi.Libs/FastColoredTextBox/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/RyuaNerin/Azpe/blob/master/Azpe/jsontoolkit/JsonToolKit 4.3.832.cs
public string ToString(bool niceFormat) {
            CultureInfo culture = System.Threading.Thread.CurrentThread.CurrentCulture;
            System.Threading.Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;

            StringBuilder sb = new StringBuilder();
            Stringifier.stringify(this, sb, 0, niceFormat);

            System.Threading.Thread.CurrentThread.CurrentCulture = culture;

            return sb.ToString();
        }
================================================================================
https://github.com/RyuaNerin/Azpe/blob/master/Azpe/jsontoolkit/JsonToolKit 4.3.832.cs
public string ToString(bool niceFormat) {
            CultureInfo culture = System.Threading.Thread.CurrentThread.CurrentCulture;
            System.Threading.Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;

            StringBuilder sb = new StringBuilder();
            Stringifier.stringify(this, sb, 0, niceFormat);

            System.Threading.Thread.CurrentThread.CurrentCulture = culture;

            return sb.ToString();
        }
================================================================================
https://github.com/MSAlih1/ScriptCommunityPack-Editor/blob/master/REditor/FastColoredTextBoxNS/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/RyuaNerin/ImageAPIs/blob/master/ImageAPIs/Library/JsonToolKit 4.1.736.cs
public string ToString(bool niceFormat)
                {
                        CultureInfo culture = System.Threading.Thread.CurrentThread.CurrentCulture;
                        System.Threading.Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;

                        StringBuilder sb = new StringBuilder();
                        Stringifier.stringify(this, sb, 0, niceFormat);

                        System.Threading.Thread.CurrentThread.CurrentCulture = culture;

                        return sb.ToString();
                }
================================================================================
https://github.com/RyuaNerin/ImageAPIs/blob/master/ImageAPIs/Library/JsonToolKit 4.1.736.cs
public string ToString(bool niceFormat)
                {
                        CultureInfo culture = System.Threading.Thread.CurrentThread.CurrentCulture;
                        System.Threading.Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;

                        StringBuilder sb = new StringBuilder();
                        Stringifier.stringify(this, sb, 0, niceFormat);

                        System.Threading.Thread.CurrentThread.CurrentCulture = culture;

                        return sb.ToString();
                }
================================================================================
https://github.com/xieguigang/Reference_SharedLib/blob/master/UX.Framework/Pavel.CodeEditor/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/oxysoft/PokeSharp/blob/master/src/MapEditor.UI/FastColoredTextbox/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/jcaillon/3P/blob/master/YamuiFramework/Controls/FastColoredTextBox/Core/Hotkeys.cs
public override string ToString()
        {
            var cult = Thread.CurrentThread.CurrentUICulture;
            Thread.CurrentThread.CurrentUICulture = CultureInfo.InvariantCulture;
            StringBuilder sb = new StringBuilder();
            var kc = new KeysConverter();
            foreach (var pair in this)
            {
                sb.AppendFormat("{0}={1}, ", kc.ConvertToString(pair.Key), pair.Value);
            }

            if (sb.Length > 1)
                sb.Remove(sb.Length - 2, 2);
            Thread.CurrentThread.CurrentUICulture = cult;

            return sb.ToString();
        }
================================================================================
https://github.com/StackableRegiments/metl2011/blob/master/MeTLMeeting/MeTLLib/Providers/GlobalCultureCompatibility.cs
public override string ToString()
        {
            Thread.CurrentThread.CurrentCulture = enAUCulture;
            return base.ToString();
        }
================================================================================
https://github.com/StackableRegiments/metl2011/blob/master/MeTLMeeting/SandRibbonObjects/GlobalCultureCompatibility.cs
public override string ToString()
        {
            System.Threading.Thread.CurrentThread.CurrentCulture = currentCulture;
            return base.ToString();
        }
================================================================================
https://github.com/ivayloivanof/OOP.CSharp/blob/master/01.DefiningClasses/OtherHomeworks/DefClass/03. PC Catalog/Computer.cs
public override string ToString()
        {
            Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
            StringBuilder result = new StringBuilder();
            result.AppendLine(string.Format("Computer name: {0}", this.Name));
            foreach (var component in components)
            {
                result.AppendLine(string.Format("{0}{2} {1:c2}", component.Name, component.Price, string.IsNullOrWhiteSpace(component.Details) ? ":" : ":" + " " + component.Details + ":"));
            }
            result.AppendLine(string.Format("Total price: {0:c2}", this.Price));
            return result.ToString();
        }
================================================================================
https://github.com/ivayloivanof/OOP.CSharp/blob/master/01.DefiningClasses/OtherHomeworks/01.DefineClasses/03.PCCatalog/Computer.cs
public override string ToString()
    {
        Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
        StringBuilder result = new StringBuilder();
        result.AppendLine(string.Format("Computer name: {0}", this.Name));
        foreach (var component in components)
        {
            result.AppendLine(string.Format("{0}{2} {1:c2}", component.Name, component.Price, string.IsNullOrWhiteSpace(component.Details) ? ":" : ":" + " " + component.Details + ":"));
        }
        result.AppendLine(string.Format("Total price: {0:c2}", this.Price));
        return result.ToString();
    }
================================================================================
https://github.com/beBoss/SoftUni/blob/master/Object-Oriented Programming/Defining Classes/Problem-3-PcCatalog/Computer.cs
public override string ToString()
        {
            Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
            var result = new StringBuilder();
            result.AppendLine($"Computer name: {this.Name}");

            foreach (var component in this.components)
            {
                result.AppendLine(value:
                    string.Format("{0}{2} {1:c2}",
                    component.Name,
                    component.Price,
                    string.IsNullOrWhiteSpace(component.Details) ? ":" : ":" + " " + component.Details + ":"));
            }

            result.AppendLine($"Total price: {this.Price:c2}");
            return result.ToString();
        }
================================================================================
https://github.com/ivayloivanof/OOP.CSharp/blob/master/04.EncapsulationAndPolymorphism/OtherHomeworks/OOP-master/01. OOP-Defining-Classes-Homework/03.PcCatalog/Computer.cs
public override string ToString()
    {
        Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
        StringBuilder result = new StringBuilder();
        result.AppendLine(string.Format("Computer name: {0}", this.Name));
        foreach (var component in components)
        {
            result.AppendLine(string.Format("{0}{2} {1:c2}",component.Name, component.Price, string.IsNullOrWhiteSpace(component.Details) ?  ":": ":" +" "+ component.Details+":"));
        }
        result.AppendLine(string.Format("Total price: {0:c2}" , this.Price));
        return result.ToString();
    }

Of those, the ones that don't set the culture back are:

https://github.com/StackableRegiments/metl2011/blob/master/MeTLMeeting/MeTLLib/Providers/GlobalCultureCompatibility.cs
public override string ToString()
        {
            Thread.CurrentThread.CurrentCulture = enAUCulture;
            return base.ToString();
        }
================================================================================
https://github.com/ivayloivanof/OOP.CSharp/blob/master/01.DefiningClasses/OtherHomeworks/DefClass/03. PC Catalog/Computer.cs
public override string ToString()
        {
            Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
            StringBuilder result = new StringBuilder();
            result.AppendLine(string.Format("Computer name: {0}", this.Name));
            foreach (var component in components)
            {
                result.AppendLine(string.Format("{0}{2} {1:c2}", component.Name, component.Price, string.IsNullOrWhiteSpace(component.Details) ? ":" : ":" + " " + component.Details + ":"));
            }
            result.AppendLine(string.Format("Total price: {0:c2}", this.Price));
            return result.ToString();
        }
================================================================================
https://github.com/beBoss/SoftUni/blob/master/Object-Oriented Programming/Defining Classes/Problem-3-PcCatalog/Computer.cs
public override string ToString()
        {
            Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
            var result = new StringBuilder();
            result.AppendLine($"Computer name: {this.Name}");

            foreach (var component in this.components)
            {
                result.AppendLine(value:
                    string.Format("{0}{2} {1:c2}",
                    component.Name,
                    component.Price,
                    string.IsNullOrWhiteSpace(component.Details) ? ":" : ":" + " " + component.Details + ":"));
            }

            result.AppendLine($"Total price: {this.Price:c2}");
            return result.ToString();
        }
================================================================================
https://github.com/ivayloivanof/OOP.CSharp/blob/master/04.EncapsulationAndPolymorphism/OtherHomeworks/OOP-master/01. OOP-Defining-Classes-Homework/03.PcCatalog/Computer.cs
public override string ToString()
    {
        Thread.CurrentThread.CurrentCulture = new CultureInfo("bg");
        StringBuilder result = new StringBuilder();
        result.AppendLine(string.Format("Computer name: {0}", this.Name));
        foreach (var component in components)
        {
            result.AppendLine(string.Format("{0}{2} {1:c2}",component.Name, component.Price, string.IsNullOrWhiteSpace(component.Details) ?  ":": ":" +" "+ component.Details+":"));
        }
        result.AppendLine(string.Format("Total price: {0:c2}" , this.Price));
        return result.ToString();
    }

3 of those are obviously homework, and I have no idea what the final one is trying to achieve: link.

I haven't used Roslyn for parsing code before - if I've done something dumb, please let me know.

[gafter]

@canton7 We've marked this help wanted because we are now willing to take a fix for this.

[canton7]

Right, I was working off the last comment of

We are reluctant to make a change without fully understanding the compat story.

Just to make sure I understand the desired behaviour:

1. We should not call string.Concat overloads which take object. Instead, any arguments should be converted to a string (either by being evaluated as a string, or by being evaluated as object and then having ?.ToString() called on them), in the order left-to-right.
2. In order to preserve backwards compatibility, value types which have a ToString method which may mutate the type should have ToString() called on a defensive copy. Currently this happens because ToString() is called on a boxed copy (discussed in #415).

For example:

• s is a string
• cs is a type convertible to a string
• r is a reference type (which isn't convertible to string)
• mv is a mutable value type
• iv is an immutable value type
• g is a generic type, not constrained to class
• gr is a generic type, constrained to class

Then the expression s + cs + r + mv + iv + g + gr will have the same behaviour as:

var mvCopy = mv;
var gCopy = g;
string.Concat(
    s,
    (string)cs
    r?.ToString(),
    mvCopy.ToString(),
    iv.ToString(),
    gCopy?.ToString(),
    gr?.ToString());

(Of course, the copying of value types would happen inside the expression, but I can't express that in C#).

Is this correct?

The immutability of a value type could be determined by the readonly modifier, plus checks for specific special types in order to support .NET Framework (where special types don't have readonly applied to them).

Presumably we would also apply this to unary concatenation? i.e. mv + "" is currently turned into string.Concat(mv) ?? "", would we instead generate mvCopy?.ToString() ?? ""?

---

There is also a slight niggle in the form of #7079.

A particular problem here was that calls via object are emitted as constrained calls causing compat issues on runtimes that do not have support for generics.

So there are compat issues with platforms which don't support generics if we emit constrained virtual calls to ToString(). However, with this change we are going to be emitting a lot more constrained virtual calls, for all value types that we don't special-case (currently bool, char, IntPtr, UIntPtr). Is that a problem? Do we want to special-case more types?

[dgrunwald]

I am playing around with this change. Unless I'm mistaken, neither the old nor the new compiler behavior matches the specification???

If I understood the specification correctly, the + operator first evaluates both inputs, then converts the non-string input to string, then concatenates. So for

new C(0) + Space() + new C(1)

The + operator groups left-associative, so we're evaluating (new C(0) + Space()) + new C(1). The outer + first evaluates the LHS, so from the spec I would expect the evaluation order:

• new C(0)
• Space()
• C(0).ToString()
• new C(1)
• C(1).ToString() But that's not what the Roslyn master currently does; it instead results in:

  new C(0)
  C(0).ToString()
  Space()
  new C(1)
  C(1).ToString()

  Effectively, Roslyn master immediately converts the non-string to string, before even evaluating the other argument to string.op_Addition(object, string). Full example on SharpLab

It gets even more interesting if we use parentheses to re-group the concatenation:

Console.WriteLine(new C(0) + (Space() + new C(1)));

Now from the spec I would expect this evaluate as in op_Addition(object, op_Addition(string, object)), so:

• new C(0)
• Space()
• new C(1)
• C(0).ToString()
• C(1).ToString() The old compiler actually got this right (but for entirely the wrong reason), but master changed the evaluation order to:

  new C(0)
  C(0).ToString()
  Space()
  new C(1)
  C(1).ToString()

So my takeaway is: The old compiler was optimizing string concatenation in a way that doesn't follow the language spec; the new compiler is optimizing string concatenation further in a way that still doesn't follow language spec. Though arguably this time you could argue that it's the spec that is broken; as the new compiler behavior intuitively makes sense: every object is immediately converted to string, before evaluating any further operands.

[gafter]

It gets even more interesting if we use parentheses to re-group the concatenation:

Console.WriteLine(new C(0) + (Space() + new C(1)));

By my reading we should evaluate C(1).ToString() before C(0).ToString(), because "the + operator first evaluates both inputs, then converts the non-string input to string, then concatenates". Evaluating the right-hand operand requires computing C(1).ToString().

[gafter]

@dgrunwald I would recommend opening a new issue for your observations. Please tag me and @canton7 .