-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
[C#] Make interpolated strings including string or char constants as optimized as +
operator (for string type)
#72308
base: main
Are you sure you want to change the base?
Conversation
f36dc21
to
6b9d9b7
Compare
6b9d9b7
to
0b63661
Compare
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. |
a4a1070
to
d7d542f
Compare
String.Concat
if possible in interpolated string in VB+
operator
d7d542f
to
2355e52
Compare
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. |
0391843
to
378b699
Compare
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. |
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. |
+
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass. Tests are not looked at, but since they are currently passing, there's clearly a test gap.
IsTreatedAsLiteralInStringConcatenation(fillin.Value) || | ||
fillin.Value.Type is { SpecialType: SpecialType.System_String } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
observedAsString
?
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the parameter name for false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just not deal with these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second one is really bad as you pointed out.
Even the first one? I got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just not deal with these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this true? What does it mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean BoundDynanmicInvocation
.
{ | ||
case BoundInterpolatedString.AppendFormattedMethod: | ||
{ | ||
// never be BoundLiteral (can be BoundLocal), so see ConstantValueOpt instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#74275 - we can't restore this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably condense all of that into a single pattern, but otherwise it looks about right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I have to aggregate some conditional expressions, not only chain them with or
and ||
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know what that question means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the above. I mean
if (candidate is MethodSymbol { IsStatic: false, Parameters: [{ Type.SpecialType: SpecialType.System_String }], DeclaredAssessibility: Assessibility.Public } methodCandidate)
{
return methodCandidate;
}
ea4e845
to
1f3482b
Compare
…ized as + operator (for string type)
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 merged From 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 merged The 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
string
andchar
constants are not embedded as literals in string interpolation #71801VB: #72611This PR targets at only those evaluated as
string
(not string handler or formatter types)