can't we box other types e.g. int and pass to Concat(object, object) etc? #ByDesign

@alrz I think the purpose of this PR is to get the camel's nose into the tent. #Resolved

#Resolved

I suppose that's a no -- due to custom formatting semantics. camel's nose shall not get into the tent. #Resolved

What I meant was doing this for strings is the camel's nose in the tent. Once this optimization is done, if we can demonstrate that it would be correct to do the optimization for other types in the future, the optimization could be extended in the future. #Resolved

Tagging @AlekseyTs as FYI. I'm not sure if it was the case before, but interpolated strings can now introduce some small control flow (null-coalescing operator introduced during lowering). #Resolved

} [](start = 4, length = 1)

Consider testing nested string interpolation $"{$"...."}" too. #Closed

@jcouv Done #Closed

builder.Builder [](start = 16, length = 15)

Consider extracting local for builder.Builder. #Closed

Should that be done in the existing MakeInterpolatedStringFormat too? #Closed

Yes, that'd be good. Thanks

In reply to: 190506216 [](ancestors = 190506216)

{ [](start = 20, length = 1)

Consider asserting that the part is a BoundLiteral and ConstantValue is not null (ie. a string literal) #Closed

I agree... please do that.

In reply to: 190382117 [](ancestors = 190382117)

The existing lowering to string.Format doesn't have that assert. Should it be added there too? #Closed

Yes, that would be good. Thanks

In reply to: 190494041 [](ancestors = 190494041)

== [](start = 63, length = 2)

Perhaps use is null (no need to call string.operator==) #Closed

Doesn't the compiler generate the exact same code in both cases, i.e. a brtrue.s instruction? #Closed

You're correct. Never mind then :-)

In reply to: 190485587 [](ancestors = 190485587)

_factory.StringLiteral("") [](start = 39, length = 26)

It seems like _factory.StringLiteral("") always creates instances. @gafter Do you think it's worth it to use a static singleton here? #Resolved

No, this is fine. It is easier to debug if the bound nodes have the right syntax node associated with them, which the factory helps with.

In reply to: 190383830 [](ancestors = 190383830)

I didn't understand why the treatment is different in the case where length != 1 than it was in the case where length == 1.

I'd expect we can extract the logic from the length == 1 into a method, then we can call the method in both cases and just check what's returned (if it's just a StringLiteral, in the case where length == 1 then we can return).
If that works out, then I suspect the check on length == 1 will become a very local choice, rather than a containing if statement.

Something like:

foreach (var part in node.Parts)
{
    var loweredPart = LowerPart(part);
    if (length == 1 && /* loweredPart is a string literal */) { return loweredPart; }
    result = ...
}
``` #Resolved

The reason why the length == 1 case is different, is that it requires additional processing (e.g. lowering to a null coalescing expression) to ensure the resulting string is not null. Such processing is not required when length > 1, because string concatenation allready ensures it. I will add a comment to clarify that.

I did consider merging the loops.

Because of the additonal requirements for the length == 1 case, such a LowerPart method would need two parameters: LowerPart(BoundExpression part, bool isOnlyPart). While the loop duplication is not ideal, such a flag parameter isn't either. The parameter isOnlyPart is indeed tested locally in LowerPart, but has no local meaning. It becomes magic. And if you create two LowerPart variants, you might as well have the two loops.

As for the loop itself:

bool isOnlyPart = length == 1;
foreach (var part in node.Parts)
{
    var loweredPart = LowerPart(part, isOnlyPart);
    if (isOnlyPart && loweredPart is BoundLiteral)
    {
        Debug.Assert(/* loweredPart is a non-null string*/);
        return loweredPart;
    }

    result = ...
}

We can of course skip the if statement, and have the string literal go through another lowering pass like any other result. From a functional point of view, that doesn't hurt.

With or without the if statement, having a specialized loop for length == 1 is more efficient.
#Resolved

To clarify: the additional processing to ensure that the result is not null is required when length == 1, but forbidden when length > 1. The reason for that is that string concatenation does not flatten ((a + b) ?? "") + c to a + b + c, and the same is true for a + ((b + c) ?? "").

However, it just occurred to me that this problem can be fixed quite easily :-). I'll see what I can do. #Resolved

Done. #Resolved

I suspect this method can be re-used in VisitInterpolatedString (LocalRewriter_StringInterpolation.cs line 78) #Closed

No it cannot. String.Format requires '{' to be escaped as "{{" and '}' to be escaped as "}}". #Closed

I'm sorry, I don't see the difference between this method and the code I referred to.

                var builder = PooledStringBuilder.GetInstance();
                ...
                int formatLength = formatText.Length;
                for (int i = 0; i < formatLength; i++)
                {
                    char c = formatText[i];
                    builder.Builder.Append(c);
                    if ((c == '{' || c == '}') && (i + 1) < formatLength && formatText[i + 1] == c)
                    {
                        i++;
                    }
                }
                ... builder.ToStringAndFree() ...

In reply to: 190505462 [](ancestors = 190505462)

I'm confused. What code are you referring too?

Or are you saying that line 86:

 formatString.Builder.Append(part.ConstantValue.StringValue);

does the same thing as:

        int formatLength = s.Length;
        for (int i = 0; i < formatLength; i++)
        {
            char c = s[i];
            stringBuilder.Append(c);
            if ((c == '{' || c == '}') && (i + 1) < formatLength && s[i + 1] == c)
            {
                i++;
            }
        }

?

It's not the same. When lowering an interpolation to string.Format, any "{{" must remain "{{", because string.Format needs them to be escaped. When lowering to string concatenation, they need to be unescaped to "{". #Closed

My bad, I was looking at a local copy of the file before your change. Sorry for the confusion.

In reply to: 191042562 [](ancestors = 191042562)

I don't think we should do this unless there was no string concatenation. #Resolved

It is not functionally wrong to do this, even if there was a concatenation, because we remove it again in the lowering of the coalescing operator. I noticed this technique is used in lot's of places. (For example, the lowering of a non-binary string concatenation builds lot's of intermediary structures that are broken down again when lowering a higher level.) So I used the same technique here to clean up the code considerably. Also, notice that the unit tests all still pass, nothing has changed there.

But you are correct, a simple if (length == 1) avoids it in most cases.
#Resolved

Is this still needed? #Resolved

Yes, for the case $"{a + b}" (where a + b is a string). #Resolved