Skip to content

Support interpolated string handlers in extension blocks#78425

Merged
333fred merged 15 commits intodotnet:mainfrom
333fred:extensions-interpolated-string
Jul 17, 2025
Merged

Support interpolated string handlers in extension blocks#78425
333fred merged 15 commits intodotnet:mainfrom
333fred:extensions-interpolated-string

Conversation

@333fred
Copy link
Member

@333fred 333fred commented May 3, 2025

Closes #78137.

Relates to test plan #76130

@333fred 333fred requested a review from a team as a code owner May 3, 2025 01:13
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2025
@333fred 333fred requested review from AlekseyTs, jcouv and jjonescz May 3, 2025 01:14
@jcouv jcouv self-assigned this May 5, 2025
@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 8, 2025

                            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. #Closed


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

@jcouv jcouv removed their assignment May 8, 2025
@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 8, 2025

Done with the review pass (commit 2) #Closed

333fred added 4 commits May 23, 2025 14:13
…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)
  ...
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 15, 2025

[InlineData("01 00 0b 6e 6f 6e 65 78 69 73 74 65 6e 74 00 00")] // "nonexistent"

Isn't there a "verbatim" syntactic form that allows to use strings in attributes without converting them to bytes? At least there is a mode like that in ILDASM. I assume its output can be compiled back. #Closed


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:22047 in 566dcdc. [](commit_id = 566dcdc, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 15, 2025

    Assert.True(symbol.ExtensionParameter.HasInterpolatedStringHandlerArgumentError);

Could you point me to the code that is responsible for setting this flag when we are using "i" as the target parameter name in the attribute? #Closed


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:22161 in 566dcdc. [](commit_id = 566dcdc, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 15, 2025

    private ImmutableArray<int> DecodeInterpolatedStringHandlerArgumentAttribute()

How are we getting away without specially handling parameters of a marker method in this function? #Closed


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

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 12)

@333fred
Copy link
Member Author

333fred commented Jul 15, 2025

    Assert.True(symbol.ExtensionParameter.HasInterpolatedStringHandlerArgumentError);

It's nothing out of the ordinary, PEParameterSymbol.DecodeInterpolatedStringHandlerArgument exits here:

. We don't see the extension parameter as a ExtensionMetadataMethodParameterSymbol, we see it as the parameter of the marker method, which is a static method, so the normal handling applies.


In reply to: 3071739258


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:22161 in 566dcdc. [](commit_id = 566dcdc, deletion_comment = False)

@333fred
Copy link
Member Author

333fred commented Jul 15, 2025

    private ImmutableArray<int> DecodeInterpolatedStringHandlerArgumentAttribute()

The marker method is just an ordinary method, with an ordinary parameter. The marker method is static, so it fails the condition in the default switch case and the normal handling kicks in. If there's some way to explicitly detect that the containing method is the marker method from metadata, I'd be happy to add an early-out.


In reply to: 3071742879


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

@333fred
Copy link
Member Author

333fred commented Jul 15, 2025

[InlineData("01 00 0b 6e 6f 6e 65 78 69 73 74 65 6e 74 00 00")] // "nonexistent"

I'm not aware of any such mode in ILDasm for attributes, only for things like ldstr, and I wasn't able to find such a thing testing locally. If it does exist and I can use that instead of serstring bytes.


In reply to: 3071723611


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:22047 in 566dcdc. [](commit_id = 566dcdc, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    private ImmutableArray<int> DecodeInterpolatedStringHandlerArgumentAttribute()

. If there's some way to explicitly detect that the containing method is the marker method from metadata, I'd be happy to add an early-out.

I see that you were able to figure it out


In reply to: 3071797572


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

}
else if (ContainingType is PENamedTypeSymbol { IsExtension: true } containingPE
&& containingPE.GetMarkerMethodSymbol() is MethodSymbol markerMethod
&& (object)markerMethod == ContainingSymbol)
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainingSymbol

As an optimization consider starting with checking that ContainingSymbol is a method with a WellKnownMemberNames.ExtensionMarkerMethodName name. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 15, 2025

[InlineData("01 00 0b 6e 6f 6e 65 78 69 73 74 65 6e 74 00 00")] // "nonexistent"

If it does exist and I can use that instead of serstring bytes.

There is such a mode, but it puts verbal values into comments, so there is no special alternative IL syntax. My mistake.

/CAVERBAL Output CA blobs in verbal form (default - in binary form).

There is also an option in menu in UI


In reply to: 3071825901


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:22047 in 566dcdc. [](commit_id = 566dcdc, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    Assert.True(symbol.ExtensionParameter.HasInterpolatedStringHandlerArgumentError);

So for the specific value it was failing because we disallow references to itself.


In reply to: 3071792252


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:22161 in 566dcdc. [](commit_id = 566dcdc, deletion_comment = False)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 13). However consider implementing an optimization - #78425 (review)

@AlekseyTs AlekseyTs requested a review from jjonescz July 15, 2025 14:14
@333fred
Copy link
Member Author

333fred commented Jul 16, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@333fred 333fred enabled auto-merge (squash) July 16, 2025 18:57
@333fred 333fred merged commit abd8aba into dotnet:main Jul 17, 2025
24 of 27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 17, 2025
@333fred 333fred deleted the extensions-interpolated-string branch July 17, 2025 20:54
333fred added a commit that referenced this pull request Jul 24, 2025
* Support interpolated string handlers in extension blocks (#78425)

* Support interpolated string handlers in extension blocks

Closes #78137.

* A few tests fail PEVerify

* Update handling of receivers in static scenarios and add tests for indexers/static methods

* Update after merge

* Ensure nullable index adjustment happens always, not just when the receiver parameter is used.

* Additional il verification progress

* PR feedback. The only remaining item is additional testing for ref safety to record the current state.

* Additional PR feedback

* More testing, suggested simplification

* Simplify and more testing.

* Feedback

* Suggested optimization

* Add skipped test for #78433.

* Fix loss of nullability attributes when getting the 'constructed reduced from' method. (#79400)

* Add tracking issue for disabled assert (#79427)

Reference tracking issue #79426 for the disabled assert.

* Fix code gen for some compound assignment scenarios involving extension properties. (#79339)

* Extensions: extension marker type name (#79308)

* Move to xunit.runner.visualstudio 3.1.3

* Avoid eliding pointer-to-ref conversions (#79311)

* Avoid eliding pointer to ref conversions

* Consider only user-defined locals

* Update pre-existing tests

* Test evaluation order

* Explain user-defined local check

* Avoid reusing temps whose refs might be captured (#76009)

* Avoid reusing temps whose refs might be captured

* Revert some changes

* Simplify the heuristic

* Avoid reusing any local whose address has been taken

* Update tests

* Inline a function

* Revert unrelated change

* Remove non-reusable locals after checking for them

* Keep ref count for addressed locals list

* Extend an assert

* Add high-level comment to MightEscapeTemporaryRefs

* Filter non-reusable locals

* Simplify `int`s to `bool`s

* Replace coalesce with an assert

* Mark nested calls as always used

* Fix this parameter of nint methods

* Test chained call

* Simplify by using this parameter symbol

* Fix readonly targets

* Fixup a test

* Remove an unused parameter

* Fix nullability after merge

* [main] Update dependencies from dotnet/arcade (#79385)

[main] Update dependencies from dotnet/arcade


 - Revert .NET SDK bump

 - Set NUGET_PACKAGES for bootstrap builds in CI

 - Set NUGET_PACKAGES for Correctness_Build_Artifacts

 - Add tracking issue for workarounds

 - Call nuget restore with the -publish flag

 - We need to restore where we publish

 - Add comment for strange restore behavior

 - Update comment

* Fixes rename attribute bug (#79418)

* fix bug

* update test

* simplify

* comment

* clarity

* gracefully handle the document or languageservice being null

* Adds EqualityComparer<T>.Create polyfill

* Revert compiler changes

* Update dependencies from https://github.com/dotnet/arcade build 20250717.3

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks
 From Version 10.0.0-beta.25358.3 -> To Version 10.0.0-beta.25367.3

* Move to .NET 10p6 (#79438)

* Move to .NET 10p6

Let's try to get ahead of the next one.

* Update contributing docs as well.

* Allow razor to use SolutionChecksumUpdater (#79373)

* Allow razor to use SolutionChecksumUpdater

SolutionChecksumUpdater allows incremental updates of sourcetexts in the server process, but was previously limited to just documents in the solution. Instead, by loosening this to allow TextDocuments, razor editing can have incremental updates of the primary razor document when cohosting is turned on.

* use is/or instead of ==/||

* Remove dependency on ClassificationOptions and DefinitionItem from Semantic Search service (#79439)

* Update insertions for VS release changes

* Fix static extension method not showing up on enum type

* Add workitem

* cleanup

* Use ConfigureAwait(true) when in a blocking JTF run call

* GetTextSynchronously

* Update src/VisualStudio/Core/Def/LanguageService/AbstractLanguageService`2.VsLanguageDebugInfo.cs

Co-authored-by: Jason Malinowski <jason@jason-m.com>

* Make synchronous

* Docs

* Allow Razor to create a formatting options, and send it to OOP

* Update dependencies from https://github.com/dotnet/arcade build 20250717.4

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks
 From Version 10.0.0-beta.25358.3 -> To Version 10.0.0-beta.25367.4

* Remove assumption about a tree shape that compound assignment produces (#79432)

* Skip GetProjectXml_FileBasedProgram_SdkTooOld_01 test

* Bump Microsoft.VisualStudio.Extensibility.Testing.Xunit to 0.1.796-beta

This brings along some fixes needed for it to work under newer versions
of xunit.runner.visualstudio.

* Reduce path length of Roslyn ServiceHub Services folder (#79460)

I've been hitting lots of gold bars in VS indicating various Roslyn features are unavailable when running when F5'ing the roslyn project. I finally spent some time digging into why this happens on my machine, and it turns out that out service hub service paths are just a bit too long. This PR just reduces that path length by 14 characters, which is enough for all the services to be under the 260 character long path limit.

* downgrade SDK

* Fix Typo

* Simplify stale project handling (#79386)

* Update System.Memory to prevent bootstrap failure

* Include dependencies of System.Memory in VSIX deployment project

* remove unnecessary additional reference

* Update dependencies from https://github.com/dotnet/arcade build 20250717.5

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks
 From Version 10.0.0-beta.25358.3 -> To Version 10.0.0-beta.25367.5

* Use new compiler API

* Correctly change return type of partial definition part

* Feedback

* Move RoslynParallel and ProducerConsumer to threading source package (#79474)

* Downgrade SDK

* Do not remove usings/imports in code-cleanup when file has syntax errors

* Add testws

* Find add-using results in SG docs

* Docs

* Do not place SuppressMessage attributes on an extension block

* remove asynchrony from service registration

* Ignore all targets of MS.CA.Threading.Package in rebuild (#79495)

* Don't mark special compiler synthesized members as obsolete

* Add test

* Use globs in .projitems for source packages (#79480)

* Fix name declaration completion in primary constructor parameters

* nrt

* nrt

* Generate complogs on build failure (#79446)

* Generate complogs on build failure

This should make it easier to reproduce CI compiler failures locally.

* Test bad change for complog creation

* Use --yes

* Include full exception

* Revert "Test bad change for complog creation"

This reverts commit a465981.

* remove from pkgdefs

* Extensions: fix issue with GetParameterOrExtensionParameter (#79402)

* Fix exception when requesting additional file diagnostics

* Do not do full cleanup when we are formatting a new document

The CodeAction.CleanupDocumentAsync was added in
#59091 to ensure the document
had elastic trivia formatted before we applied the result to the
file. It appears the intent was to do just the trivia formatting, but
the CleanupDocumentAsync does a lot of expensive operations around
simplification and adding imports, which aren't needed in this case.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1540532

* Extensions: add test for function type scenario (#79323)

* Reenable analyzer that was disabled when we moved to .Net 10 Preview 5

* Allow captured primary constructor parameters to be still used in base-list

* Simplify ancestor checks

* Fix 'this' keyword not being recomended where allowed

* in progress

* Fix 'switch' keyword recomendations after an expression

* Fix escaping of enum members in completion

* Suggest proper 'Async'-suffixed name for Task-returning symbols

* Fix location of 'partial' modifier when generating new partial members

* Update VMR orchestrator property (#79517)

* Make attachdebugger flag work on non-Windows (#79499)

These compiler flags used Debugger.Launch, which only works on windows. I borrowed the equivalent code from the LSP launcher, which already has this functionality (https://github.com/dotnet/roslyn/blob/cf34acbc5d0f64ff87013cdd02ae61cff562daaf/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs#L75-L89).

* Downgrade System.Memory to make VS happy

* Fix auth issue in PR val builds

* Fix

* Update Basic.Reference.Assemblies (#79525)

* Update Basic.Reference.Assemblies

Update to version 1.8.3 of Basic.Reference.Assemblies and make references to .NET 10p6 available in our tests.

* Feedback

* Feedback

* Allow add using to work on Razor generated documents

* Improve compiler server/client logging (#79335)

* Remove unused constant

* Include timestamp in logs

* Log messages to msbuild as well

* Log server's process ID

* Log process creation on Unix

* Exclude some messages from the binlog

* Fix test loggers

* Revert "Exclude some messages from the binlog"

This reverts commit 12c1050.

* Cleanup

* Simplify

* Simplify

* Simplify

* Add tests

* Add tests

* Simplify

* Simplify

* initial change

* Renames

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Extensions: address or split remaining open issues directly associated with test plan (#79452)

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* Update tests

* In progress

* in progress

* in progress

* in progress

* in progress

* in progress

* in progress

* in progress

* Cleanup split between lexer and sliding-text-window (#79205)

Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

* Add docs

* Fix name qualification when moving a using alias outside of a file scoped namespace

* Fix strucutre out of bounds

* Tests

* Add Version.Details.props (#79539)

* Fix issue with 'use explicit type' and nullable tuples

* Add back

* Convert ValueTaskFactory methods to static extensions (#79541)

* Update logic

* Also downgrade System.Numerics.Vectors for VS

* Add Enum static extensions (#79546)

* Fix main build break

* Fix main build break AGAIN

* Projects to deploy (#79430)

* [main] Source code updates from dotnet/dotnet (#79483)

[main] Source code updates from dotnet/dotnet


 - Workaround cref resolve issue due to new RuntimeHelpers in SCI

* [main] Source code updates from dotnet/dotnet (#79563)

[main] Source code updates from dotnet/dotnet

* Revert "Fix main build break AGAIN (#79559)"

This reverts commit e67e2a4, reversing
changes made to 1a74e66.

* Tweak build break fix

* Tweak build break fix

* inline test strings

* Cleanup test methods

* Fix issue where we weren't properly adding elastic trivia to newly generated members

* Fix behavior

* Add test

* Mark SyntaxTokenParser as non-experimental (#79521)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

---------

Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: AlekseyTs <AlekseyTs@users.noreply.github.com>
Co-authored-by: Julien Couvreur <julien.couvreur@gmail.com>
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
Co-authored-by: Jan Jones <janjones@microsoft.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Ankita Khera <40616383+akhera99@users.noreply.github.com>
Co-authored-by: tmat <tomas.matousek@microsoft.com>
Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Todd Grunke <toddgrun@microsoft.com>
Co-authored-by: David Barbet <dabarbet@microsoft.com>
Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Jason Malinowski <jason@jason-m.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com>
Co-authored-by: DoctorKrolic <mapmyp03@gmail.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
Co-authored-by: Djuradj Kurepa <91743470+dkurepa@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything The extension everything feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error CS8945 when using InterpolatedStringHandlerArgument with new extension block

6 participants