[C#] Make interpolated strings including string or char constants as optimized as + operator (for string type)
#72308
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
tats-u
force-pushed
the
string-interpolation-string-char
branch
2 times, most recently
from
f36dc21
to
6b9d9b7
Compare
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
6b9d9b7
to
0b63661
Compare
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
51a6a8c
to
27b6fa5
Compare
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
|
Also, let's separate out the VB changes. These are both fairly large changes, and we would want to be able to revert them separately if issues are discovered. |
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
a4a1070
to
d7d542f
Compare
tats-u
changed the title
String.Concat if possible in interpolated string in VB+ operator
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
d7d542f
to
2355e52
Compare
tats-u
force-pushed
the
string-interpolation-string-char
branch
2 times, most recently
from
ff64036
to
e98eb2d
Compare
|
"Compiler request rejected" |
This often means the change made to the compiler broke it, and prevents it from being able to compile itself. |
I am glad to know that It is due to the bootstrap tests. |
|
I have no idea about where must be fixed as of now. |
|
@tats-u there's a large number of errors in the bootstrap build that look like this: There's also an analyzer error that didn't exist before: This is almost certainly a fallout of your change. What I would do is start with investigating those; fixing them and adding tests for the missing scenarios will likely resolve any other errors, or make them clearer. |
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
0391843
to
378b699
Compare
tats-u
force-pushed
the
string-interpolation-string-char
branch
2 times, most recently
from
1dfb61f
to
113cf59
Compare
|
Sorry for very late reply, but I managed to fix the missing code causing the failure in the bootstrap test without doing local bootstrap test. (I just added some extra mean test cases and some were failed)
Thank you for the information, but it seems that I added a new feature: $"{null}" // ""
$"foo{null}bar{null}" // "foobar"Anyway, although I have to add some extra test cases, but my code is now ready for reviewed. Thank you in advance. |
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
9a14c73
to
4005f1e
Compare
|
I finished my work. Optimizations for handlers or formattable strings have too many things to be determined. (especially what types or methods should be trusted) |
|
If you want me to squash commits, I will do. |
tats-u
changed the title
+ operator+ operator (for string type)
|
I created a draft of a PoC for the further optimization only for the combination of I think I should create another PR whose commits are built on the top of this one because the content of this PR is stable. |
| IsTreatedAsLiteralInStringConcatenation(fillin.Value) || | ||
| fillin.Value.Type is { SpecialType: SpecialType.System_String } |
There was a problem hiding this comment.
The formatting of this makes it pretty hard to read. Consider breaking this out into a separate variable before to make it more understandable.
There was a problem hiding this comment.
I would like to postpone the evaluation of this function as late as possible.
I will split the if statement into two.
!(...) will be an independent if statement with the De Morgan's law applied.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
Outdated
Show resolved
Hide resolved
| if (canHideOptimization && fillin is { Alignment: null, Format: null, Value.ConstantValueOpt: { } constantValueOpt }) | ||
| { | ||
| switch (constantValueOpt) | ||
| { | ||
| case { IsChar: true, CharValue: char ch }: | ||
| stringBuilder.Append(escapeInterpolatedStringLiteral(ch.ToString())); | ||
| continue; | ||
|
|
||
| case { IsString: true, StringValue: var str }: | ||
| stringBuilder.Append(escapeInterpolatedStringLiteral(str ?? "")); | ||
| continue; | ||
|
|
||
| case { IsNull: true }: | ||
| // null literal for string? type | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We can't do this generally. We can do it for interpolated strings that are directly observed as string, but for IFormattable or FormattableString, this will change the observable semantics of the returned formattable string (as you can call GetArguments() and do post-processing on them if you want). If this is what you meant by the canHideOptimizations parameter, then I would suggest calling that parameter something like usingDirectlyAsString instead. Also, please leave a comment with this explanation in the code.
There was a problem hiding this comment.
@333fred
Should we consider FormattableString.Invariant?
usingDirectlyAsString
You seem to have thought "using" is better than "used". If so I will follow you because there do not seem to be a better candidate that is not much longer.
| // TODO: Last argument could be true for some cases | ||
| // e.g. - ((FormattableString)$"Now: {DateTime.Now}").ToString(CultureInfo.GetCulture("some-lang")) | ||
| // - FormattableString.Invariant($"Now: {DateTime.Now}") | ||
| MakeInterpolatedStringFormat((BoundInterpolatedString)conversion.Operand, out format, out expressions, false); |
There was a problem hiding this comment.
Please use the parameter name for false.
There was a problem hiding this comment.
It is better to just remove it (because it is an optional parameter)?
| // TODO: Last argument could be true for some cases | ||
| // e.g. - ((FormattableString)$"Now: {DateTime.Now}").ToString(CultureInfo.GetCulture("some-lang")) | ||
| // - FormattableString.Invariant($"Now: {DateTime.Now}") |
There was a problem hiding this comment.
The second one is really bad as you pointed out.
Even the first one? I got it.
There was a problem hiding this comment.
@333fred How about these ones?
FormattableString.Invariant will not abuse this optimization and outside code cannot observe the optimization.
const string CompanyNameConst = "Your Company";
FormattableString.Invariant($"Copyright (c) {year} {CompanyNameConst}")
// FormattableString.Invariant(FormattableStringFactory.Create("Copyright (c) {0} {1}", [year, "Your Company"]))
// ↓
// FormattableString.Invariant(FormattableStringFactory.Create("Copyright (c) {0} Your Company", [year]))There was a problem hiding this comment.
Let's just not deal with these.
There was a problem hiding this comment.
I've given up trying to optimize that case.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
Outdated
Show resolved
Hide resolved
| reusable = default; | ||
| } | ||
|
|
||
| foreach (var part in parts) |
There was a problem hiding this comment.
I did not get very far into reviewing this foreach. It's extremely complicated, with method-level state implicitly flowing around (I have absolutely no idea what reusable is or what it's tracking). It needs be deeply commented, and not method-level state tracking. I would recommend starting by clarifying what reusable actually means, and making the merge methods be static local functions, to force all state to be explicitly shown.
There was a problem hiding this comment.
no idea what reusable is
It is used to retain nodes of AppendLiteral method calls and string literals that have not been joined to others yet.
They can be reused (output as is) until merged to others.
Maybe I should not have to define it as a tuple form.
making the merge methods be static local functions
The reason why I have not made it static is just because I have to use _factory. The other part is static.
Is it true that you would like me to convert it to an additional parameter of the local function?
I do not care about whether _factory is a captured field or a function parameter.
There was a problem hiding this comment.
Nodes that may be retained by reusable are just previous part of the loop variable part.
|
|
||
| foreach (var part in parts) | ||
| { | ||
| if (part is not BoundCall { Method.Name: not null, } call) |
There was a problem hiding this comment.
When is this true? What does it mean?
There was a problem hiding this comment.
I assume mainly BoundDynamicCall.
It should be treated as is.
{ ... } may be misplaced kindness because neither of Method or Name is nullable.
I have not known what belongs to BoundDynamicCall instead of BoundCall.
There was a problem hiding this comment.
I mean BoundDynanmicInvocation.
| { | ||
| case BoundInterpolatedString.AppendFormattedMethod: | ||
| { | ||
| // never be BoundLiteral (can be BoundLocal), so see ConstantValueOpt instead |
There was a problem hiding this comment.
What will never be a BoundLiteral? The argument certainly can be, and that's the only thing I can think of that you might be saying? Are you saying it will never be a BoundLiteral of a string?
There was a problem hiding this comment.
Are you saying it will never be a BoundLiteral of a string?
I forgot whether or not, but I think I meant so at that time.
I will fix the comment.
| int increasedLiteralLength = 0; | ||
|
|
||
| BoundExpression createAppendLiteralCallFromBoundLiteral(BoundLiteral literal, BoundExpression receiver) => | ||
| _factory.InstanceCall(receiver, BoundInterpolatedString.AppendLiteralMethod, literal); |
There was a problem hiding this comment.
#74274 removed this InstanceCall. What should I write instead of this?
How can I generate a MethodSymbol instance from the method name ("AppendLiteral")?
There was a problem hiding this comment.
I will retore this definition:
public BoundExpression InstanceCall(BoundExpression receiver, string name, bool disallowExpandedNonArrayParams, BoundExpression arg0)
=> MakeInvocationExpression(BinderFlags.None, this.Syntax, receiver, name, disallowExpandedNonArrayParams, [arg0], this.Diagnostics);There was a problem hiding this comment.
#74275 - we can't restore this method.
There was a problem hiding this comment.
I'm afraid I have no idea about how I should get a MethodSymbol instance of an AroundLiteral method instead of restoring the InstanceCall method because it does not belong to any interfaces (cannot use WellKnownMember). Could you tell me a hint or how to do it instead?
There was a problem hiding this comment.
The concern is that we can't do overload resolution at all here. Since we don't need overload resolution to find the specific AppendLiteral(string) method, we can just get the method symbol and use it directly.
There was a problem hiding this comment.
I'd probably condense all of that into a single pattern, but otherwise it looks about right.
There was a problem hiding this comment.
Do I have to aggregate some conditional expressions, not only chain them with or and ||?
There was a problem hiding this comment.
I don't really know what that question means?
There was a problem hiding this comment.
I mean whether you prefer the following
if (
candidate is not MethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_String }], DeclaredAccessibility: Accessibility.Public} methodCandidate
|| methodCandidate.IsStatic
)
{
continue;
}
return methodCandidate;to the following:
if (
candidate is not MethodSymbol methodCandidate
|| methodCandidate.IsStatic
|| methodCandidate is not { Parameters: [{ Type.SpecialType: SpecialType.System_String }] }
|| methodCandidate.DeclaredAccessibility != Accessibility.Public
)
{
continue;
}
return methodCandidate;NOTE: we can't write the following due to dotnet/csharplang#8323:
if (
candidate is not MethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_String }], DeclaredAccessibility: Accessibility.Public} methodCandidate
or { IsStatic: false }
)
{
continue;
}
return methodCandidate;The following is OK. Do you prefer it to the above 2?
if (
candidate is not (
MethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_String }], DeclaredAccessibility: Accessibility.Public} methodCandidate
and { IsStatic: false }
)
)
{
continue;
}
return methodCandidate;There was a problem hiding this comment.
None of the above. I mean
if (candidate is MethodSymbol { IsStatic: false, Parameters: [{ Type.SpecialType: SpecialType.System_String }], DeclaredAssessibility: Assessibility.Public } methodCandidate)
{
return methodCandidate;
}
tats-u
force-pushed
the
string-interpolation-string-char
branch
3 times, most recently
from
ea4e845
to
1f3482b
Compare
…ized as + operator (for string type)
tats-u
force-pushed
the
string-interpolation-string-char
branch
from
1f3482b
to
4c3fac4
Compare
|
@tats-u please do not force-push branches under code review. It prevents us from doing a diff review, and means we have to start from fresh again. I hope to have some time later this week to take another look at the changes, but the force push is going to mean it takes significantly more time and will delay my review. |
|
@333fred Oh, sorry. That's why I can find many roughly-written commit messages in the commit history of this repo. Now I don't have to $ diff -up <(git diff 2bfbc1e1b1194dbfeae2751821f70d3082669e9e~2 2bfbc1e1b1194dbfeae2751821f70d3082669e9e) <(git diff ea4e845~2 ea4e845)
--- /dev/fd/63 2024-08-02 00:09:58.000000000 +0900
+++ /dev/fd/62 2024-08-02 00:09:58.000000000 +0900
@@ -46,15 +46,9 @@ index 46d37fdd2d5..87a3d3f6c64 100644
if (node.OperatorKind is BinaryOperatorKind.Utf8Addition)
diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
-index ce475336445..8f2297e231b 100644
+index ce475336445..6f2840ea6a2 100644
--- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
+++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
-@@ -1,4 +1,4 @@
--// Licensed to the .NET Foundation under one or more agreements.
-+// Licensed to the .NET Foundation under one or more agreements.=> (MethodSymbol)factory.WellKnownTypeMember(WellKnownMember.System_Runtime_CompilerServices_DefaultInterpolatedStringHandler__AppendLiteral));
- // The .NET Foundation licenses this file to you under the MIT license.
- // See the LICENSE file in the project root for more information.
-
@@ -18,6 +18,9 @@ private BoundExpression RewriteInterpolatedStringConversion(BoundConversion conv
Debug.Assert(conversion.ConversionKind == ConversionKind.InterpolatedString);
BoundExpression format;If I had to apply the change in the latest P.S. May these help you: From the last commit after the triple force-push: $ diff -up <(git diff ea4e845~2 ea4e845) <(git diff HEAD~2 HEAD)
--- /dev/fd/63 2024-08-01 23:50:43.000000000 +0900
+++ /dev/fd/62 2024-08-01 23:50:43.000000000 +0900
@@ -31,22 +31,21 @@ index 0a15fecb093..ebdb9101b19 100644
}
case SyntaxKind.InterpolatedStringText:
diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
-index 46d37fdd2d5..87a3d3f6c64 100644
+index 46d37fdd2d5..84047bd73f6 100644
--- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
+++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
-@@ -116,7 +116,9 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
+@@ -116,7 +116,8 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
{
Debug.Assert(node.Type.SpecialType == SpecialType.System_String, "Non-string binary addition should have been handled by VisitConversion or VisitArguments");
ImmutableArray<BoundExpression> parts = CollectBinaryOperatorInterpolatedStringParts(node);
- return LowerPartsToString(data, parts, node.Syntax, node.Type);
+ // We do not have to consider custom handlers or formatters here thanks to the above Debug.Assert, so now we have only to take expression trees into account.
-+ var canHideOptimization = !_inExpressionLambda;
-+ return LowerPartsToString(data, parts, node.Syntax, node.Type, canHideOptimization);
++ return LowerPartsToString(data, parts, node.Syntax, node.Type, usingDirectlyAsString: !_inExpressionLambda);
}
if (node.OperatorKind is BinaryOperatorKind.Utf8Addition)
diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
-index ce475336445..6f2840ea6a2 100644
+index ce475336445..1c660ae0fee 100644
--- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
+++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
@@ -18,6 +18,9 @@ private BoundExpression RewriteInterpolatedStringConversion(BoundConversion conv
@@ -385,7 +384,7 @@ index ce475336445..6f2840ea6a2 100644
+
+ foreach (BoundExpression part in parts)
+ {
-+ // BoundDynamicInvoke etc.
++ // BoundDynamicInvocation etc.
+ if (part is not BoundCall call)
+ {
+ // cannot be mergedFrom the latest force-push: $ diff -up <(git diff 1f3482b~2 1f3482b) <(git diff ea4e845~2 ea4e845)
--- /dev/fd/63 2024-08-02 00:23:03.000000000 +0900
+++ /dev/fd/62 2024-08-02 00:23:03.000000000 +0900
@@ -46,7 +46,7 @@ index 46d37fdd2d5..87a3d3f6c64 100644
if (node.OperatorKind is BinaryOperatorKind.Utf8Addition)
diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
-index ce475336445..1c660ae0fee 100644
+index ce475336445..6f2840ea6a2 100644
--- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
+++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
@@ -18,6 +18,9 @@ private BoundExpression RewriteInterpolatedStringConversion(BoundConversion conv
@@ -385,7 +385,7 @@ index ce475336445..1c660ae0fee 100644
+
+ foreach (BoundExpression part in parts)
+ {
-+ // BoundDynamicInvocation etc.
++ // BoundDynamicInvoke etc.
+ if (part is not BoundCall call)
+ {
+ // cannot be mergedThe current first commit is the change as of the previous review (4005f1e). $ diff -up <(git diff 4005f1ed9b7f1cbdbac0c4ef27e4c482750f7779~22 4005f1ed9b7f1cbdbac0c4ef27e4c482750f7779) <(git diff HEAD~2 HEAD~)
--- /dev/fd/63 2024-08-03 13:57:17.000000000 +0900
+++ /dev/fd/62 2024-08-03 13:57:17.000000000 +0900
@@ -31,10 +31,10 @@ index 0a15fecb093..ebdb9101b19 100644
}
case SyntaxKind.InterpolatedStringText:
diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
-index 8a57e0daae1..072c8a6782d 100644
+index 46d37fdd2d5..87a3d3f6c64 100644
--- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
+++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs
-@@ -115,7 +115,9 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
+@@ -116,7 +116,9 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
{
Debug.Assert(node.Type.SpecialType == SpecialType.System_String, "Non-string binary addition should have been handled by VisitConversion or VisitArguments");
ImmutableArray<BoundExpression> parts = CollectBinaryOperatorInterpolatedStringParts(node);
@@ -46,7 +46,7 @@ index 8a57e0daae1..072c8a6782d 100644
if (node.OperatorKind is BinaryOperatorKind.Utf8Addition)
diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
-index a148a9ee88d..3bc9369f7d7 100644
+index ce475336445..af3b69b2b6e 100644
--- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
+++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringInterpolation.cs
@@ -18,7 +18,10 @@ private BoundExpression RewriteInterpolatedStringConversion(BoundConversion conv
@@ -749,7 +749,7 @@ index 338ad31210c..93b7e280620 100644
}
}
diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
-index 2d3ef41415b..369eecc85af 100644
+index c05cf0acb7a..0b72676b4f0 100644
--- a/src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
+++ b/src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs
@@ -4,6 +4,7 @@
@@ -804,7 +804,7 @@ index 2d3ef41415b..369eecc85af 100644
}
");
}
-@@ -13543,6 +13536,81 @@ .locals init (System.Linq.Expressions.ParameterExpression V_0)
+@@ -13541,6 +13534,81 @@ .locals init (System.Linq.Expressions.ParameterExpression V_0)
");
}
@@ -886,7 +886,7 @@ index 2d3ef41415b..369eecc85af 100644
[Theory]
[CombinatorialData]
public void CustomHandlerUsedAsArgumentToCustomHandler(bool useBoolReturns, bool validityParameter, [CombinatorialValues(@"$""""", @"$"""" + $""""")] string expression)
-@@ -16087,11 +16155,9 @@ public void InterpolatedStringsAddedUnderObjectAddition2(string expression)
+@@ -16050,11 +16118,9 @@ public void InterpolatedStringsAddedUnderObjectAddition2(string expression)
CompileAndVerify(comp, expectedOutput: @"
(
value:1
@@ -900,9 +900,9 @@ index 2d3ef41415b..369eecc85af 100644
value:3
}
");
-@@ -18569,5 +18635,261 @@ public struct A
- // public A(int x, A a = M($"{1}")) { }
- Diagnostic(ErrorCode.ERR_BadArgRef, @"$""{1}""").WithArguments("1", "ref").WithLocation(8, 29));
+@@ -18983,5 +19049,261 @@ public override string ToString()
+ Diagnostic(ErrorCode.ERR_ConversionWithInterface, "C1").WithArguments("C1.implicit operator C1(System.IFormattable)").WithLocation(18, 37)
+ );
}
+
+ [Theory]
@@ -1163,7 +1163,7 @@ index 2d3ef41415b..369eecc85af 100644
}
}
diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/RawInterpolationTests_Handler.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/RawInterpolationTests_Handler.cs
-index 08ff7b26dc4..5786584bd89 100644
+index 4f5656dafe8..894b30bf51b 100644
--- a/src/Compilers/CSharp/Test/Semantic/Semantics/RawInterpolationTests_Handler.cs
+++ b/src/Compilers/CSharp/Test/Semantic/Semantics/RawInterpolationTests_Handler.cs
@@ -2057,13 +2057,11 @@ public void TargetTypedInterpolationHoles(string expression)
@@ -1210,7 +1210,7 @@ index 08ff7b26dc4..5786584bd89 100644
}
");
}
-@@ -11636,11 +11628,9 @@ public void InterpolatedStringsAddedUnderObjectAddition2(string expression)
+@@ -11635,11 +11627,9 @@ public void InterpolatedStringsAddedUnderObjectAddition2(string expression)
CompileAndVerify(comp, expectedOutput: @"
(
value:1 |
| } | ||
| else | ||
| { | ||
| BoundLiteral? literal = null; |
There was a problem hiding this comment.
I wonder if we should temporarily retain not only BoundLiteral but also other BoundExpression that has string ConstantValueOpt.
Those other than BoundLiteral are not ideal but fine as arguments of AppendLiteral.
If we retain such values, extra operations on PooledStringBuilder will be reduced.
We squash PRs in the compiler layer of this repo, so no intermediate commits of compiler PRs are visible in the commit history. It's best to avoid any amending and force pushes.
Yes, please do |
stringandcharconstants are not embedded as literals in string interpolation #71801VB: #72611This PR targets at only those evaluated as
string(not string handler or formatter types)