Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer Concat on interpolated strings with 4 or less string parts #54726

Merged
merged 6 commits into from
Jul 12, 2021
Merged

Prefer Concat on interpolated strings with 4 or less string parts #54726

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee6294e
Prefer Concat on interpolated strings with 4 or less string parts
333fred Jul 9, 2021
441db55
Remove extra either
333fred Jul 9, 2021
5447050
Make Stannis happy.
333fred Jul 9, 2021
75f7a3d
Single line formatting.
333fred Jul 9, 2021
9eadc80
Fix comment
333fred Jul 12, 2021
df66bb6
Really fix comment.
333fred Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,21 +132,24 @@ private BoundInterpolatedString BindUnconvertedInterpolatedStringToString(BoundU
{
// We have 4 possible lowering strategies, dependent on the contents of the string, in this order:
// 1. The string is a constant value. We can just use the final value.
// 2. The WellKnownType DefaultInterpolatedStringHandler is available, and none of the interpolation holes contain an await expression.
// 2. The string is composed of 4 or fewer components that are all strings, we can lower to a call to string.Concat without a
// params array. This is very efficient as the runtime can allocate a buffer for the string with exactly the correct length and
// make no intermediate allocations.
// 3. The WellKnownType DefaultInterpolatedStringHandler is available, and none of the interpolation holes contain an await expression.
// The builder is a ref struct, and we can guarantee the lifetime won't outlive the stack if the string doesn't contain any
// awaits, but if it does we cannot use it. This builder is the only way that ref structs can be directly used as interpolation
// hole components, which means that ref structs components and await expressions cannot be combined. It is already illegal for
// the user to use ref structs in an async method today, but if that were to ever change, this would still need to be respected.
// We also cannot use this method if the interpolated string appears within a catch filter, as the builder is disposable and we
// cannot put a try/finally inside a filter block.
// 3. The string is composed entirely of components that are strings themselves. We can turn this into a single call to string.Concat.
// We prefer the builder over this because the builder can use pooling to avoid new allocations, while this call will potentially
// need to allocate a param array.
// 4. The string has heterogeneous data and either InterpolatedStringHandler is unavailable, or one of the holes contains an await
// 4. The string is composed of more than 4 components that are all strings themselves. We can turn this into a single
// call to string.Concat. We prefer the builder over this because the builder can use pooling to avoid new allocations, while this
// call will need to allocate a param array.
// 5. The string has heterogeneous data and either InterpolatedStringHandler is unavailable, or one of the holes contains an await
// expression. This is turned into a call to string.Format.
//
// We need to do the determination of 1, 2, or 3/4 up front, rather than in lowering, as it affects diagnostics (ref structs not being
// able to be used, for example). However, between 3 and 4, we don't need to know at this point, so that logic is deferred for lowering.
// We need to do the determination of 1, 2, 3, or 4/5 up front, rather than in lowering, as it affects diagnostics (ref structs not being
// able to be used, for example). However, between 4 and 5, we don't need to know at this point, so that logic is deferred for lowering.

if (unconvertedInterpolatedString.ConstantValue is not null)
{
Expand All @@ -155,13 +158,22 @@ private BoundInterpolatedString BindUnconvertedInterpolatedStringToString(BoundU
return constructWithData(BindInterpolatedStringParts(unconvertedInterpolatedString, diagnostics), data: null);
}

// Case 2. Attempt to see if all parts are either strings or convertible to ROS<char>. The types must be homogeneous, as there
// are no overloads that mix and match.
if (unconvertedInterpolatedString.Parts.Length <= 4 &&
unconvertedInterpolatedString.Parts.All(p => p is BoundLiteral
or BoundStringInsert { Value: { Type: { SpecialType: SpecialType.System_String } }, Alignment: null, Format: null }))
{
return constructWithData(BindInterpolatedStringParts(unconvertedInterpolatedString, diagnostics), data: null);
}

if (tryBindAsHandlerType(out var result))
{
// Case 2
// Case 3
return result;
}

// The specifics of 3 vs 4 aren't necessary for this stage of binding. The only thing that matters is that every part needs to be convertible
// The specifics of 4 vs 5 aren't necessary for this stage of binding. The only thing that matters is that every part needs to be convertible
// object.
return constructWithData(BindInterpolatedStringParts(unconvertedInterpolatedString, diagnostics), data: null);

Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,10 @@ public override BoundNode VisitInterpolatedString(BoundInterpolatedString node)
_factory.Binary(BinaryOperatorKind.StringConcatenation, node.Type, result, part);
}

if (length == 1)
// We need to ensure that the result of the interpolated string is not null. If the single part has a non-null constant value
// or is itself an interpolated string (which by proxy cannot be null), then there's nothing else that needs to be done. Otherwise,
// we need to test for null and ensure "" if it is.
if (length == 1 && result is not ({ Kind: BoundKind.InterpolatedString } or { ConstantValue: { IsString: true } }))
{
result = _factory.Coalesce(result!, _factory.StringLiteral(""));
}
Expand Down
242 changes: 223 additions & 19 deletions src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,61 @@ static void Main(string[] args)
CompileAndVerify(source, expectedOutput: expectedOutput);
}

[Fact]
public void OneLiteral()
{
string source =
@"using System;
class Program
{
static void Main(string[] args)
{
Console.WriteLine( $""Hello"" );
}
}";
string expectedOutput = @"Hello";
var verifier = CompileAndVerify(source, expectedOutput: expectedOutput);
verifier.VerifyIL("Program.Main", @"
{
// Code size 11 (0xb)
.maxstack 1
IL_0000: ldstr ""Hello""
IL_0005: call ""void System.Console.WriteLine(string)""
IL_000a: ret
}
");
}

[Fact]
public void OneInsert()
{
string source =
@"using System;
class Program
{
static void Main(string[] args)
{
var hello = $""Hello"";
Console.WriteLine( $""{hello}"" );
}
}";
string expectedOutput = @"Hello";
var verifier = CompileAndVerify(source, expectedOutput: expectedOutput);
verifier.VerifyIL("Program.Main", @"
{
// Code size 20 (0x14)
.maxstack 2
IL_0000: ldstr ""Hello""
IL_0005: dup
IL_0006: brtrue.s IL_000e
IL_0008: pop
IL_0009: ldstr """"
IL_000e: call ""void System.Console.WriteLine(string)""
IL_0013: ret
}
");
}

[Fact]
public void TwoInserts()
{
Expand Down Expand Up @@ -1202,6 +1257,164 @@ static void Main()
);
}

[Fact, WorkItem(54702, "https://github.com/dotnet/roslyn/issues/54702")]
public void InterpolatedStringHandler_ConcatPreferencesForAllStringElements()
{
var code = @"
using System;
Console.WriteLine(TwoComponents());
Console.WriteLine(ThreeComponents());
Console.WriteLine(FourComponents());
Console.WriteLine(FiveComponents());

string TwoComponents()
{
string s1 = ""1"";
string s2 = ""2"";
return $""{s1}{s2}"";
}

string ThreeComponents()
{
string s1 = ""1"";
string s2 = ""2"";
string s3 = ""3"";
return $""{s1}{s2}{s3}"";
}

string FourComponents()
{
string s1 = ""1"";
string s2 = ""2"";
string s3 = ""3"";
string s4 = ""4"";
return $""{s1}{s2}{s3}{s4}"";
}

string FiveComponents()
{
string s1 = ""1"";
string s2 = ""2"";
string s3 = ""3"";
string s4 = ""4"";
string s5 = ""5"";
return $""{s1}{s2}{s3}{s4}{s5}"";
}
";

var handler = GetInterpolatedStringHandlerDefinition(includeSpanOverloads: false, useDefaultParameters: false, useBoolReturns: false);

var verifier = CompileAndVerify(new[] { code, handler }, expectedOutput: @"
12
123
1234
value:1
value:2
value:3
value:4
value:5
");

verifier.VerifyIL("<Program>$.<<Main>$>g__TwoComponents|0_0()", @"
{
// Code size 18 (0x12)
.maxstack 2
.locals init (string V_0) //s2
IL_0000: ldstr ""1""
IL_0005: ldstr ""2""
IL_000a: stloc.0
IL_000b: ldloc.0
IL_000c: call ""string string.Concat(string, string)""
IL_0011: ret
}
");

verifier.VerifyIL("<Program>$.<<Main>$>g__ThreeComponents|0_1()", @"
{
// Code size 25 (0x19)
.maxstack 3
.locals init (string V_0, //s2
string V_1) //s3
IL_0000: ldstr ""1""
IL_0005: ldstr ""2""
IL_000a: stloc.0
IL_000b: ldstr ""3""
IL_0010: stloc.1
IL_0011: ldloc.0
IL_0012: ldloc.1
IL_0013: call ""string string.Concat(string, string, string)""
IL_0018: ret
}
");

verifier.VerifyIL("<Program>$.<<Main>$>g__FourComponents|0_2()", @"
{
// Code size 32 (0x20)
.maxstack 4
.locals init (string V_0, //s2
string V_1, //s3
string V_2) //s4
IL_0000: ldstr ""1""
IL_0005: ldstr ""2""
IL_000a: stloc.0
IL_000b: ldstr ""3""
IL_0010: stloc.1
IL_0011: ldstr ""4""
IL_0016: stloc.2
IL_0017: ldloc.0
IL_0018: ldloc.1
IL_0019: ldloc.2
IL_001a: call ""string string.Concat(string, string, string, string)""
IL_001f: ret
}
");

verifier.VerifyIL("<Program>$.<<Main>$>g__FiveComponents|0_3()", @"
{
// Code size 89 (0x59)
.maxstack 3
.locals init (string V_0, //s1
string V_1, //s2
string V_2, //s3
string V_3, //s4
string V_4, //s5
System.Runtime.CompilerServices.DefaultInterpolatedStringHandler V_5)
IL_0000: ldstr ""1""
IL_0005: stloc.0
IL_0006: ldstr ""2""
IL_000b: stloc.1
IL_000c: ldstr ""3""
IL_0011: stloc.2
IL_0012: ldstr ""4""
IL_0017: stloc.3
IL_0018: ldstr ""5""
IL_001d: stloc.s V_4
IL_001f: ldloca.s V_5
IL_0021: ldc.i4.0
IL_0022: ldc.i4.5
IL_0023: call ""System.Runtime.CompilerServices.DefaultInterpolatedStringHandler..ctor(int, int)""
IL_0028: ldloca.s V_5
IL_002a: ldloc.0
IL_002b: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(string)""
IL_0030: ldloca.s V_5
IL_0032: ldloc.1
IL_0033: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(string)""
IL_0038: ldloca.s V_5
IL_003a: ldloc.2
IL_003b: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(string)""
IL_0040: ldloca.s V_5
IL_0042: ldloc.3
IL_0043: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(string)""
IL_0048: ldloca.s V_5
IL_004a: ldloc.s V_4
IL_004c: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(string)""
IL_0051: ldloca.s V_5
IL_0053: call ""string System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.ToStringAndClear()""
IL_0058: ret
}
");
}

[Theory]
[CombinatorialData]
public void InterpolatedStringHandler_OverloadsAndBoolReturns(bool useDefaultParameters, bool useBoolReturns, bool constructorBoolArg)
Expand Down Expand Up @@ -2948,36 +3161,27 @@ public void NestedInterpolatedStrings()

var interpolatedStringBuilder = GetInterpolatedStringHandlerDefinition(includeSpanOverloads: false, useDefaultParameters: false, useBoolReturns: false);

var verifier = CompileAndVerify(new[] { source, interpolatedStringBuilder }, expectedOutput: @"value:value:1");
var verifier = CompileAndVerify(new[] { source, interpolatedStringBuilder }, expectedOutput: @"value:1");

verifier.VerifyIL("<top-level-statements-entry-point>", @"
{
// Code size 55 (0x37)
.maxstack 4
// Code size 32 (0x20)
.maxstack 3
.locals init (int V_0, //i
System.Runtime.CompilerServices.DefaultInterpolatedStringHandler V_1,
System.Runtime.CompilerServices.DefaultInterpolatedStringHandler V_2)
System.Runtime.CompilerServices.DefaultInterpolatedStringHandler V_1)
IL_0000: ldc.i4.1
IL_0001: stloc.0
IL_0002: ldloca.s V_1
IL_0004: ldc.i4.0
IL_0005: ldc.i4.1
IL_0006: call ""System.Runtime.CompilerServices.DefaultInterpolatedStringHandler..ctor(int, int)""
IL_000b: ldloca.s V_1
IL_000d: ldloca.s V_2
IL_000f: ldc.i4.0
IL_0010: ldc.i4.1
IL_0011: call ""System.Runtime.CompilerServices.DefaultInterpolatedStringHandler..ctor(int, int)""
IL_0016: ldloca.s V_2
IL_0018: ldloc.0
IL_0019: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted<int>(int)""
IL_001e: ldloca.s V_2
IL_0020: call ""string System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.ToStringAndClear()""
IL_0025: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(string)""
IL_002a: ldloca.s V_1
IL_002c: call ""string System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.ToStringAndClear()""
IL_0031: call ""void System.Console.WriteLine(string)""
IL_0036: ret
IL_000d: ldloc.0
IL_000e: call ""void System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted<int>(int)""
IL_0013: ldloca.s V_1
IL_0015: call ""string System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.ToStringAndClear()""
IL_001a: call ""void System.Console.WriteLine(string)""
IL_001f: ret
}
");
}
Expand Down