Support interpolated string handlers in extension blocks #78425

333fred · 2025-05-03T01:13:37Z

Relates to test plan #76130

jjonescz · 2025-05-06T08:57:26Z

src/Compilers/CSharp/Portable/BoundTree/BoundInterpolatedStringArgumentPlaceholder.cs

@@ -7,8 +7,9 @@ namespace Microsoft.CodeAnalysis.CSharp
    internal partial class BoundInterpolatedStringArgumentPlaceholder
    {
        public const int InstanceParameter = -1;
-        public const int TrailingConstructorValidityParameter = -2;
-        public const int UnspecifiedParameter = -3;
+        public const int ExtensionReceiver = -2;


Looks like we adjusted all places that used InstanceParameter to also consider ExtensionReceiver except one which is responsible for reporting ERR_InterpolatedStringsReferencingInstanceCannotBeInObjectInitializers - doesn't that need to be adjusted as well?

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

jjonescz · 2025-05-06T09:04:32Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenParameterSymbol.cs

+                // If this is the extension method receiver (ie, parameter 0), then any non-empty list of indexes must
+                // be an error, and we should have already returned an empty list.
+                Debug.Assert(_underlyingParameter.ContainingSymbol is not NamedTypeSymbol);
+                Debug.Assert(originalIndexes.All(static index => index != BoundInterpolatedStringArgumentPlaceholder.InstanceParameter));


This assert seems unnecessary since we throw for the exact same condition below

jjonescz · 2025-05-06T09:05:32Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenParameterSymbol.cs

+                    BoundInterpolatedStringArgumentPlaceholder.InstanceParameter => throw ExceptionUtilities.Unreachable(),
+                    BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver => 0,
+                    BoundInterpolatedStringArgumentPlaceholder.TrailingConstructorValidityParameter => BoundInterpolatedStringArgumentPlaceholder.TrailingConstructorValidityParameter,
+                    BoundInterpolatedStringArgumentPlaceholder.UnspecifiedParameter => BoundInterpolatedStringArgumentPlaceholder.UnspecifiedParameter,


Should we instead have case var i when i < 0 => i to handle other possible values in the future? Otherwise consider asserting that index >= 0 in the case where we return index + 1.

jjonescz · 2025-05-06T09:07:20Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

@@ -19099,6 +19099,810 @@ public void AppendFormatted<T>(T hole, int alignment = 0, string format = null)
            Diagnostic(ErrorCode.ERR_InterpolatedStringHandlerMethodReturnMalformed, "{f2()}").WithArguments("?.()").WithLocation(1, 21));
    }

+    [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78137")]


Does this PR also by any chance fix #78435?

jjonescz · 2025-05-06T09:12:20Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+            }
+            """;
+
+        var comp = CreateCompilation(src, targetFramework: TargetFramework.Net90);


Consider verifying diagnostics. In the test below as well.

jjonescz · 2025-05-06T09:13:44Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+            """;
+
+        var expectedOutput = ExecutionConditionUtil.IsCoreClr ? "12" : null;
+        CompileAndVerify([exeSource, src], targetFramework: TargetFramework.Net90, expectedOutput: expectedOutput, verify: Verification.FailsPEVerify);


Consider adding more VerifyDiagnostics() calls throughout the new tests.

AlekseyTs · 2025-05-08T17:38:15Z

                            if (handlerPlaceholders.Any(static placeholder => placeholder.ArgumentIndex == BoundInterpolatedStringArgumentPlaceholder.InstanceParameter))

Are we testing extension indexers? They are in a semi-supported state. If the scenario is currently blocked due to an error, let's still add a test so that we do not forget about adjusting this code path if/when the error is no longer reported.

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:5977 in 4d67a96. [](commit_id = 4d67a96, deletion_comment = False)

AlekseyTs · 2025-05-08T17:41:56Z

src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs

@@ -712,6 +712,7 @@ private void GetInterpolatedStringPlaceholders(
                switch (argIndex)
                {
                    case BoundInterpolatedStringArgumentPlaceholder.InstanceParameter:
+                    case BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver:


case BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver:

Do we need to do something special here to account for the fact that this receiver can be a 'by value' parameter in case of a struct?

It looks like ref safety analysis also does the same trick as nullable analysis by adjusting the arguments to include receiver at position 0. See usages of ReplaceWithExtensionImplementationIfNeeded helper. It also looks like GetInterpolatedStringPlaceholders is getting called before that transformation is applied. I think that it would be less confusing and more robust to always operate on data in the transformed form. Therefore, the transformation should be applied earlier (before GetInterpolatedStringPlaceholders calls) and changes here should probably be similar to the changes made in NullableWalker,

In terms of implementation, we might want to group all transformed information into a single struct type, always pass it around as a single instance and make the transformation a part of construction of the instance. This way, the transformation will be done in one place and no one will be able to go around it.

We should make sure we have functional test coverage for this code as well.

AlekseyTs · 2025-05-08T18:13:53Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -9571,6 +9571,9 @@ void visitInterpolatedStringHandlerConstructor()
                    return;
                }

+                // In new extension form, the nullable rewriter processes the arguments as if receiver is the first item in the argument list, like the old extension form. This means that all of
+                // our placeholders will be off-by-one, with the extension receiver in the first position.
+                var newExtensionFormOffset = handlerData.ArgumentPlaceholders.Any(p => p.ArgumentIndex == BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver) ? 1 : 0;


BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver

This logic doesn't look right. I think the adjustment should happen even if receiver is not referenced. If an instance extension member is invoked, we should adjust indexes. #Closed

AlekseyTs · 2025-05-08T18:31:48Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenParameterSymbol.cs

@@ -22,8 +24,31 @@ public RewrittenParameterSymbol(ParameterSymbol originalParameter) :

        internal sealed override int CallerArgumentExpressionParameterIndex => _underlyingParameter.CallerArgumentExpressionParameterIndex;

-        internal sealed override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes => throw ExceptionUtilities.Unreachable(); // Tracked by https://github.com/dotnet/roslyn/issues/76130 : Follow up
+        internal sealed override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes


internal sealed override ImmutableArray InterpolatedStringHandlerArgumentIndexes

I think this override belongs in ExtensionMetadataMethodParameterSymbol. This override should probably keep throwing and the comment should be removed.

AlekseyTs · 2025-05-08T18:47:58Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenParameterSymbol.cs

+                var originalIndexes = this._underlyingParameter.InterpolatedStringHandlerArgumentIndexes;
+                if (originalIndexes.IsDefaultOrEmpty)
+                {
+                    return originalIndexes;


return originalIndexes;

We probably should also get here for a static underlying member #Closed

AlekseyTs · 2025-05-08T18:51:12Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

+                        else if (ContainingSymbol.ContainingSymbol is TypeSymbol { IsExtension: true, ExtensionParameter.Name: var extensionParameterName }
+                                 && string.Equals(extensionParameterName, name, StringComparison.Ordinal))
+                        {
+                            builder.Add(BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver);


builder.Add(BoundInterpolatedStringArgumentPlaceholder.ExtensionReceiver);

This doesn't feel appropriate for a static member.

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs

AlekseyTs · 2025-05-08T19:29:38Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

@@ -19099,6 +19099,810 @@ public void AppendFormatted<T>(T hole, int alignment = 0, string format = null)
            Diagnostic(ErrorCode.ERR_InterpolatedStringHandlerMethodReturnMalformed, "{f2()}").WithArguments("?.()").WithLocation(1, 21));
    }

+    [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78137")]
+    [CombinatorialData]
+    public void InterpolationHandler_ReceiverParameter_Identity(bool useMetadataRef)


InterpolationHandler_ReceiverParameter_Identity

I think we can benefit from asserting IL for unique enough scenarios. In particular I am interested in understanding and have a baseline for receiver capturing. For example, when we have a by value struct receiver, it is going to be captured by reference or by value? Are ByRef locals going to be involved? If so, could this block some async scenarios, etc.

AlekseyTs · 2025-05-08T19:36:49Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+
+    [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78137")]
+    [CombinatorialData]
+    public void InterpolationHandler_ReceiverParameter_WithConversion(bool useMetadataRef)


WithConversion

What is converted in this scenario? #Closed

I'll add a comment; the receiver is a string, so there's an implicit BoundConversion as the receiver to the extension method itself.

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

AlekseyTs · 2025-05-08T19:50:49Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+
+    [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78137")]
+    [CombinatorialData]
+    public void InterpolationHandler_ReceiverParameter_ByRef(bool useMetadataRef)


InterpolationHandler_ReceiverParameter_ByRef

It looks like we are lacking tests for "by value" struct scenarios. In addition to a trivial case, it would be interesting to test a scenario when a handler constructor modifies the receiver by direct access (not through parameter), for example by changing value of the field used as the receiver. Are we going to observe the change in the receiver value inside the extension method?

AlekseyTs · 2025-05-08T19:51:53Z

Done with the review pass (commit 2)

…dexers/static methods

…ated-string * upstream/main: (330 commits) Add support for 'fixed' Add test Add assert Modify ABWQ.AddWork to take in a ROS instead of an IEnumerable (dotnet#78691) [main] Source code updates from dotnet/dotnet (dotnet#78692) Add a test to verify that behavior is expected and bug no longer occurs [main] Source code updates from dotnet/dotnet (dotnet#78663) Add comment Revert "fix symbol publishing? (dotnet#78671)" (dotnet#78686) Revert "Update Microsoft.Build.Tasks.CodeAnalysis.Sdk.csproj (dotnet#78681)" (dotnet#78687) Use VerifyDiagnostics format to report CompileAndVerify diagnostics (dotnet#78666) Fix nullable crash for field keyword in partial property (dotnet#78672) Update Microsoft.Build.Tasks.CodeAnalysis.Sdk.csproj (dotnet#78681) Applied code review suggestions. Add test assertions Use ThrowIfFalse when resolving paths Cleanup fix symbol publishing? (dotnet#78671) Use SBRP version of M.CA for analyzers in the context of source build (dotnet#78668) Hot Reload: Handle document deletes (dotnet#78516) ...

…ceiver parameter is used.

AlekseyTs · 2025-05-27T18:09:59Z

src/Compilers/CSharp/Portable/Symbols/Extensions/ReceiverParameterSymbol.cs

+            {
+                var originalIndexes = _underlyingParameter.InterpolatedStringHandlerArgumentIndexes;
+                Debug.Assert(originalIndexes.IsEmpty);
+                return originalIndexes;


originalIndexes

Since this represents a parameter on a type, I think we should simply return an empty array. #Closed

AlekseyTs · 2025-05-27T18:10:45Z

src/Compilers/CSharp/Portable/Symbols/Extensions/ReceiverParameterSymbol.cs

+            }
+        }
+
+        internal override bool HasInterpolatedStringHandlerArgumentError => _underlyingParameter.HasInterpolatedStringHandlerArgumentError;


_underlyingParameter.HasInterpolatedStringHandlerArgumentError

Similarly here, I think we should simply return false. #Closed

I don't think we should return false always. If the parameter has handler arguments, then it should have an error on it, regardless of whether you're looking at it as an receiver parameter or not. InterpolationHandler_ParameterErrors_MappedCorrectly_02 asserts that this returns true, and I think it should stay that way.

AlekseyTs · 2025-05-27T18:13:53Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenParameterSymbol.cs

@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information.

 using System.Collections.Immutable;
+using System.Diagnostics;


Are the added usings still necessary?

AlekseyTs · 2025-05-27T18:36:17Z

                    builder.Add(BoundInterpolatedStringArgumentPlaceholder.InstanceParameter);

Is it possible to get here for an extension member?

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs:891 in b20fcbd. [](commit_id = b20fcbd, deletion_comment = False)

AlekseyTs · 2025-05-27T18:46:46Z

    private void DecodeInterpolatedStringHandlerArgumentAttribute(ref DecodeWellKnownAttributeArguments<AttributeSyntax, CSharpAttributeData, AttributeLocation> arguments, BindingDiagnosticBag diagnostics, int attributeIndex)

Is this function going to do the right thing for a receiver parameter declared in source?

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs:1237 in b20fcbd. [](commit_id = b20fcbd, deletion_comment = False)

AlekseyTs · 2025-05-27T18:49:49Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertyClonedParameterSymbolForAccessors.cs

@@ -28,5 +28,9 @@ internal override ParameterSymbol WithCustomModifiersAndParams(TypeSymbol newTyp
                _originalParam.WithCustomModifiersAndParamsCore(newType, newCustomModifiers, newRefCustomModifiers, newIsParams),
                this.ContainingSymbol);
        }
+
+        internal override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes => _originalParam.InterpolatedStringHandlerArgumentIndexes;


_originalParam.InterpolatedStringHandlerArgumentIndexes;

Is this change going to affect non-extension scenarios? #Closed

I don't think so currently: if it was going to, then we'd have already known about it because SourceClonedParameterSymbol throws an unreachable exception for this codepath. This is only hittable in extension scenarios. I can add an assert to that effect if you'd like?

AlekseyTs · 2025-05-27T18:56:18Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

@@ -2339,7 +2441,7 @@ class Nested { }

        var tree = comp.SyntaxTrees[0];
        var model = comp.GetSemanticModel(tree);
-        var extension = tree.GetRoot().DescendantNodes().OfType<ExtensionBlockDeclarationSyntax>().Single();


"i"

Should this be an error?

AlekseyTs · 2025-05-27T18:58:05Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+        public int this[int j] { get => i + j; set { } }
+    }
+}
+""";


"nonexistent"

Consider testing with an empty string as well.

AlekseyTs · 2025-05-27T19:11:00Z

Done with review pass (commit 6)

AlekseyTs · 2025-06-04T14:17:59Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

+            }
+            """;
+
+        var comp = CreateCompilation(src, targetFramework: TargetFramework.Net90);


var comp = CreateCompilation(src, targetFramework: TargetFramework.Net90);

As already suggested, diagnostics should be verified one way or another in all tests.

Support interpolated string handlers in extension blocks

8b3d60c

Closes dotnet#78137.

333fred requested a review from a team as a code owner May 3, 2025 01:13

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2025

333fred requested review from jjonescz, AlekseyTs and jcouv May 3, 2025 01:14

jcouv self-assigned this May 5, 2025

A few tests fail PEVerify

4d67a96

jjonescz reviewed May 6, 2025

View reviewed changes

AlekseyTs reviewed May 8, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs Show resolved Hide resolved

AlekseyTs reviewed May 8, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs Show resolved Hide resolved

AlekseyTs reviewed May 8, 2025

View reviewed changes

jcouv removed their assignment May 8, 2025

AlekseyTs reviewed May 8, 2025

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs Show resolved Hide resolved

AlekseyTs reviewed May 8, 2025

View reviewed changes

333fred added 4 commits May 23, 2025 14:13

Update handling of receivers in static scenarios and add tests for in…

9dde95f

…dexers/static methods

Update after merge

1608eaa

Ensure nullable index adjustment happens always, not just when the re…

b20fcbd

…ceiver parameter is used.

AlekseyTs reviewed May 27, 2025

View reviewed changes

jaredpar added Feature - Extension Everything The extension everything feature and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 4, 2025

AlekseyTs reviewed Jun 4, 2025

View reviewed changes

Support interpolated string handlers in extension blocks #78425

Are you sure you want to change the base?

Support interpolated string handlers in extension blocks #78425

Uh oh!

Conversation

333fred commented May 3, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlekseyTs May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 8, 2025

Uh oh!

AlekseyTs May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 27, 2025

Uh oh!

AlekseyTs commented May 27, 2025

Uh oh!

AlekseyTs May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented May 3, 2025 •

edited by jcouv

Loading

AlekseyTs May 8, 2025 •

edited

Loading

AlekseyTs May 8, 2025 •

edited

Loading

AlekseyTs May 8, 2025 •

edited

Loading

AlekseyTs May 8, 2025 •

edited

Loading

AlekseyTs May 8, 2025 •

edited

Loading

AlekseyTs May 27, 2025 •

edited

Loading

AlekseyTs May 27, 2025 •

edited

Loading

AlekseyTs May 27, 2025 •

edited

Loading