Make string concat assert more precise #80619

333fred · 2025-10-09T22:58:14Z

These asserts are intended to ensure that, if we ever come across a scenario that should be optimized, we consider that. The current assert is broader than this though, and already isn't in line with the code it reflects. This updates the assert to be clearer. Fixes #80254.

These asserts are intended to ensure that, if we ever come across a scenario that should be optimized, we consider that. The current assert is broader than this though, and already isn't in line with the code it reflects. This updates the assert to be clearer. Fixes dotnet#80254.

333fred · 2025-10-09T22:59:20Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

            }

-            if (argument is BoundConversion { ConversionKind: ConversionKind.Boxing, Type.SpecialType: SpecialType.System_Object, Operand: { Type.SpecialType: SpecialType.System_Char } operand })
+            if (argument is BoundConversion conversion && StringConcatOptimizationPatterns.IsBoxedChar(conversion, out BoundExpression? operand))


Rather than duplicating these checks, making it non-obvious why the assert is what it is, I've opted to pull them into the single StringConcatOptimizationPatterns struct as a single place for all the different possible checks.

333fred · 2025-10-09T23:00:39Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenStringConcat.cs

+        {
+            var source = """
+                var c = new C();
+                c.P += "a" + c.P;


The issue was that c.P is a property, so visitedCompoundAssignmentLeftRead was just a BoundCall to the getter. This ran afoul of the existing assert, which made the bad assumption that the only BoundCalls left in the tree at this point would be optimizable.

333fred · 2025-10-09T23:01:37Z

@dotnet/roslyn-compiler for review

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

AlekseyTs · 2025-10-10T04:27:05Z

Done with review pass (commit 1) #Closed

333fred · 2025-10-14T22:13:52Z

@AlekseyTs addressed your feedback, thanks. To hopefully simplify your review, I did the reset in a separate commit from the extraction: 41462f0 was just git checkout upstream/main -- src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs. 045e5ea then applied your suggested refactor.

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

AlekseyTs · 2025-10-15T17:12:26Z

Done with review pass (commit 4) #Closed

AlekseyTs

LGTM (commit 5)

333fred · 2025-10-15T20:51:30Z

@dotnet/roslyn-compiler for a second review

333fred · 2025-10-22T17:39:58Z

@dotnet/roslyn-compiler for a second revie

RikkiGibson · 2025-10-22T19:30:57Z

It looks like the original version of this assertion was failing when building the complog in dotnet/runtime#120975 using a debug compiler, when compiling this line.

Including a little more info here in case it helps verify that the change to this assert is sufficient.

Dump of the failing visitedCompoundAssignmentLeftRead

call
├─receiverOpt
│ └─local
│   ├─localSymbol: System.Collections.Generic.List<System.String> LoweringTemp.1699
│   ├─declarationKind: None
│   ├─constantValueOpt
│   ├─isNullableUnknown: False
│   └─type: System.Collections.Generic.List<string>
├─initialBindingReceiverIsSubjectToCloning: Unknown
├─method: System.Collections.Generic.List<string>.this[int].get
├─arguments
│ └─local
│   ├─localSymbol: System.Int32 LoweringTemp.1700
│   ├─declarationKind: None
│   ├─constantValueOpt
│   ├─isNullableUnknown: False
│   └─type: int
├─argumentNamesOpt: (null)
├─argumentRefKindsOpt: (null)
├─isDelegateCall: False
├─expanded: False
├─invokedAsExtensionMethod: False
├─argsToParamsOpt: (null)
├─defaultArguments: Microsoft.CodeAnalysis.BitVector
├─resultKind: Viable
├─originalMethodsOpt: (null)
└─type: string

Call stack

>	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.CollectAndVisitConcatArguments(Microsoft.CodeAnalysis.CSharp.BoundBinaryOperator originalOperator, Microsoft.CodeAnalysis.CSharp.BoundExpression visitedCompoundAssignmentLeftRead, out Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.CSharp.BoundExpression> destinationArguments) Line 221	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitCompoundAssignmentStringConcatenation(Microsoft.CodeAnalysis.CSharp.BoundExpression left, Microsoft.CodeAnalysis.CSharp.BoundExpression unvisitedRight, Microsoft.CodeAnalysis.CSharp.BinaryOperatorKind operatorKind, Microsoft.CodeAnalysis.SyntaxNode syntax) Line 31	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitBuiltInOrStaticCompoundAssignmentOperator.__rewriteAssignment|210_0(Microsoft.CodeAnalysis.CSharp.BoundExpression leftRead, Microsoft.CodeAnalysis.CSharp.BoundExpression right, bool rightIsVisited, ref Microsoft.CodeAnalysis.CSharp.LocalRewriter.<>c__DisplayClass210_0 value) Line 208	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitBuiltInOrStaticCompoundAssignmentOperator(Microsoft.CodeAnalysis.CSharp.BoundCompoundAssignmentOperator node, bool used) Line 162	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitCompoundAssignmentOperator(Microsoft.CodeAnalysis.CSharp.BoundCompoundAssignmentOperator node, bool used) Line 32	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitUnusedExpression(Microsoft.CodeAnalysis.CSharp.BoundExpression expression) Line 58	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.RewriteExpressionStatement(Microsoft.CodeAnalysis.CSharp.BoundExpressionStatement node, bool suppressInstrumentation) Line 23	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitExpressionStatement(Microsoft.CodeAnalysis.CSharp.BoundExpressionStatement node) Line 18	C#
 	[External Code]	
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitStatement(Microsoft.CodeAnalysis.CSharp.BoundStatement node) Line 241	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitPossibleUsingDeclaration(Microsoft.CodeAnalysis.CSharp.BoundStatement node, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CSharp.BoundStatement> statements, int statementIndex, out bool replacedLocalDeclarations) Line 123	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitStatementSubList(Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.CSharp.BoundStatement> builder, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CSharp.BoundStatement> statements, int startIndex) Line 81	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitBlock(Microsoft.CodeAnalysis.CSharp.BoundBlock node) Line 33	C#
 	[External Code]	
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitStatement(Microsoft.CodeAnalysis.CSharp.BoundStatement node) Line 241	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitIfStatement(Microsoft.CodeAnalysis.CSharp.BoundIfStatement node) Line 24	C#
 	[External Code]	
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitStatement(Microsoft.CodeAnalysis.CSharp.BoundStatement node) Line 241	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitPossibleUsingDeclaration(Microsoft.CodeAnalysis.CSharp.BoundStatement node, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CSharp.BoundStatement> statements, int statementIndex, out bool replacedLocalDeclarations) Line 123	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitStatementSubList(Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.CSharp.BoundStatement> builder, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CSharp.BoundStatement> statements, int startIndex) Line 81	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitBlock(Microsoft.CodeAnalysis.CSharp.BoundBlock node) Line 33	C#
 	[External Code]	
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.VisitStatement(Microsoft.CodeAnalysis.CSharp.BoundStatement node) Line 241	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.LocalRewriter.Rewrite(Microsoft.CodeAnalysis.CSharp.CSharpCompilation compilation, Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol method, int methodOrdinal, Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol containingType, Microsoft.CodeAnalysis.CSharp.BoundStatement statement, Microsoft.CodeAnalysis.CSharp.TypeCompilationState compilationState, Microsoft.CodeAnalysis.CSharp.SynthesizedSubmissionFields previousSubmissionFields, bool allowOmissionOfConditionalCalls, Microsoft.CodeAnalysis.Emit.MethodInstrumentation instrumentation, Microsoft.CodeAnalysis.CodeGen.DebugDocumentProvider debugDocumentProvider, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag diagnostics, out System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CodeGen.SourceSpan> codeCoverageSpans, out bool sawLambdas, out bool sawLocalFunctions, out bool sawAwaitInExceptionHandler) Line 146	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.LowerBodyOrInitializer(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol method, Microsoft.CodeAnalysis.CSharp.Symbols.SourceExtensionImplementationMethodSymbol extensionImplementationMethod, int methodOrdinal, Microsoft.CodeAnalysis.CSharp.BoundStatement body, Microsoft.CodeAnalysis.CSharp.SynthesizedSubmissionFields previousSubmissionFields, Microsoft.CodeAnalysis.CSharp.TypeCompilationState compilationState, Microsoft.CodeAnalysis.Emit.MethodInstrumentation instrumentation, Microsoft.CodeAnalysis.CodeGen.DebugDocumentProvider debugDocumentProvider, out System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.CodeGen.SourceSpan> codeCoverageSpans, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag diagnostics, ref Microsoft.CodeAnalysis.CodeGen.VariableSlotAllocator lazyVariableSlotAllocator, Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.Emit.EncLambdaInfo> lambdaDebugInfoBuilder, Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.CodeGen.LambdaRuntimeRudeEditInfo> lambdaRuntimeRudeEditsBuilder, Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.Emit.EncClosureInfo> closureDebugInfoBuilder, Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder<Microsoft.CodeAnalysis.CodeGen.StateMachineStateDebugInfo> stateMachineStateDebugInfoBuilder, out Microsoft.CodeAnalysis.CSharp.StateMachineTypeSymbol stateMachineTypeOpt) Line 1495	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethod(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol methodSymbol, int methodOrdinal, ref Microsoft.CodeAnalysis.CSharp.Binder.ProcessedFieldInitializers processedInitializers, Microsoft.CodeAnalysis.CSharp.SynthesizedSubmissionFields previousSubmissionFields, Microsoft.CodeAnalysis.CSharp.TypeCompilationState compilationState) Line 1222	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol containingType) Line 561	C#
 	Microsoft.CodeAnalysis.CSharp.dll!Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedTypeAsync.AnonymousMethod__0() Line 446	C#
 	Microsoft.CodeAnalysis.dll!Roslyn.Utilities.UICultureUtilities.WithCurrentUICulture.AnonymousMethod__0() Line 139	C#
 	[External Code]

333fred · 2025-10-22T20:41:37Z

@RikkiGibson do you have reason to suspect that this is not sufficient?

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

* upstream/main: (332 commits) Cache lambdas in analyzer driver (dotnet#80759) Add information for NuGet package version 4.14 (dotnet#80870) Add missing search keywords to VB Advanced options page Fix IDE0031 false positive when preprocessor directives are used in if statements (dotnet#80878) Use core compiler on netfx hosts with toolset package (dotnet#80631) Make string concat assert more precise (dotnet#80619) Extensions: address some diagnostic quality issues (dotnet#80827) Add note on traversal order for bound nodes (dotnet#80872) Ensure that locals at the top level of a constructor have the same safe-context as parameters (dotnet#80807) Fix handling of SymbolDisplayCompilerInternalOptions.UseArityForGenericTypes option for non-native symbol implementations (dotnet#80826) Update src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests.cs Add IsValidContainingStatement check to prevent collection initializers in using declarations Add back old DocumentSpan constructor (dotnet#80864) Add tests verifying pointer types in type parameters require unsafe context (dotnet#80776) Add regression test for Interlocked.Exchange with nullable types (dotnet#80796) Add regression test for ParseAttributeArgumentList with invalid input (fixes dotnet#8699) (dotnet#80705) Add regression test for compiler crash with syntax error in indexer declaration (dotnet#80772) Add runtime NullReferenceException validation to foreach null iteration tests (dotnet#80839) Update MicrosoftBuildTasksCoreVersionForMetrics to 17.11.48 (dotnet#80812) Mark CS4009 error as a "build only" error. (dotnet#80698) ...

333fred requested a review from a team as a code owner October 9, 2025 22:58

github-actions bot added the Area-Compilers label Oct 9, 2025

333fred commented Oct 9, 2025

View reviewed changes

AlekseyTs reviewed Oct 9, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 10, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Show resolved Hide resolved

jcouv self-assigned this Oct 10, 2025

333fred added 2 commits October 14, 2025 14:09

Revert to original state

41462f0

Extract simplification and use for Debug.Asserts.

045e5ea

Fix assert

ef26995