Merged
Conversation
Note: This fixes a couple of significant rename bugs:
1. TagHelperDescriptor comparison was done be reference ('==') rather than by checksum ('Equals'). So, it was possible for a tag helper not to be found when locating the "associated" tag helper from the "primary".
2. Edits could be produced with duplicate ranges.
In addition, refactoring has been performed to avoid unnecessary async state machines and simplify code.
The previous commit added an overload for TagHelperDocumentContext.Create(...) that removes the prefix parameter. Callers that we're previously null or string.Empty should call the new overload.
| [Prelude](#12503) | [Part 1](#12504) | Part 2 | [Part 3](#12506) | [Part 4](#12507) | [Part 5](#12509) | > [!WARNING] > This pull request contains breaking changes for the RazorSdk. Once this is merged and flows to the VMR, dotnet/dotnet@9a7e708 will need to be cherry-picked to resolve build breaks in `src/sdk/src/RazorSdk`. These commits represent the (mostly) mechanical changes needed to integrate `TagHelperCollection` across the Razor code base. Most references to `ImmutableArrary<TagHelperDescriptor>`, `IReadOnlyList<TagHelperDescriptor>`, `IEnumerable<TagHelperDescriptor>`, and `TagHelperDescriptor[]` have been replaced with `TagHelperCollection`. This is **by far** the largest of the `TagHelperCollection` pull requests. While most of the commits contain mechanical changes across the code base, there are few that include more substantial work that require a bit more scrutiny: - **Update `RenameService` to remove `ImmutableArray<TagHelperDescriptor>`** (fa3ad2b) This includes a fair amount of refactoring in `RenameService` to fix bugs found when moving to `TagHelperCollection`. - **Update `TagHelperFacts` to use `TagHelperCollection`** Extra work was done in `DirectiveAttributeComplationItemProvider` to clean up a bit following #12473. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842165&view=results Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842237&view=results
Refactors the tag helper change detection logic to be more efficient and avoid allocations by directly comparing collections rather than using LINQ's Except method.
…c in source generator (#12506) | [Prelude](#12503) | [Part 1](#12504) | [Part 2](#12505) | Part 3 | [Part 4](#12507) | [Part 5](#12509) | This is a relatively small change to remove LINQ from the Razor source generator and use `TagHelperCollection` when checking for added/removed tag helpers. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842169&view=results Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842240&view=results
Add TagHelperDiscoveryService to the compiler that manages calling into ITagHelperDescriptorProviders. All existing code that previously used ITagHelperDecriptorProviders directly now uses TagHelperDiscoveryService. TagHelperDiscoveryService manages a per-assembly cache using the existing AssemblySymbolData infrastructure. Currently, this is essentially a copy-paste of the per-assembly cache in TagHelperCollector.
BindTagHelperDescriptorProvider and ComponentTagHelperDescriptorProvider are a bit unique in that there's a dependency between them. When BindTagHelperDescriptorProvider runs, it walks all of the previously added tag helpers and tries to add bind tag helpers for any components. This change breaks this dependency by introducing BindTagHelperProducer, which is used by both BindTagHelperDescriptorProvider and ComponentTagHelperDescriptorProvider. Now, ComponentTagHelperDescriptorProvider is responsible for adding bind tag helpers for the components any it adds.
- Move assembly-walking algorithm from TagHelperCollector to TagHelperDiscoveryService. - Introduce TagHelperDiscoverer to cache details derived from a compilation while walking assemblies. - Delete ITagHelperDescriptorProvider and all implementations - Use a ConcurrentDIctionary<int, TagHelperCollection> as the per-assembly cache and compute the key based on both options and the selected producers. - Add new RegisterDefaultTagHelperProducer API for RazorSdk to use rather than directly referencing DefaultTagHelperDescriptorProvider. - Update tests
| [Prelude](#12503) | [Part 1](#12504) | [Part 2](#12505) | [Part 3](#12506) | Part 4 | [Part 5](#12509) | > [!WARNING] > This pull request contains breaking changes for the RazorSdk. Once this is merged and flows to the VMR, dotnet/dotnet@dc17a09 will need to be cherry-picked to resolve build breaks in `src/sdk/src/RazorSdk`. Previously, tag helpers were discovered by `ITagHelperDescriptorProvider`. Each provider was responsible for walking a compilation's assemblies and producing `TagHelperDescriptors` from the types within. This change inverts the tag helper discovery process by introducing `ITagHelperDiscoveryService` and moving the tag helper construction logic into a set of `TagHelperProducers`. The `ITagHelperDiscoveryService` performs a single walk of the compilation or assembly and calls the producers as needed. Importantly, the new process allows a more expansive cache to be maintained. There is now a per-assembly cache that holds onto `TagHelperCollection` instances. The old cache that was owned by providers via `TagHelperCollector` has been removed. To complete this change, `ITagHelperDescriptorProvider` and related types have been _deleted_. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842196&view=results Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842249&view=results
TagHelperBinders are expensive to create, and Razor often creates them for the same set of tag helpers. This change introduces a cache to avoid creating a new TagHelperDocumentContext (and therefore, a new TagHelperBinder) for the same TagHelperCollection. Note: This requires adding some deduping logic to Rename, which started creating duplicate ranges with this change.
Had to add an annoyingly long delay, while we wait for the editor for fix their bug, but a green run is a green run. I wouldn't say these are going to be consistently green from now on, and obviously I've skipped a bunch, but it's the first green in ~6 months so seems like a step forward to me at least.
Part of #12491 Removes rzls and the devkit DLL, and all of the code in the VS Code extension that supported the non-cohosting model, which has now been deleted.
…xt (#12509) | [Prelude](#12503) | [Part 1](#12504) | [Part 2](#12505) | [Part 3](#12506) | [Part 4](#12507) | Part 5 | This change introduces a weak cache for `TagHelperDocumentContext` keyed by the tag helper prefix string and `TagHelperCollection` checksum. This helps avoid creating new `TagHelperBinders` for the same set of tag helpers, since `TagHelperBinder` is expensive to create. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842228&view=results Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842250&view=results
Add the csharp razor keywords to show up in completion after typing an '@' Should fix #6927 and part of #12483 From David's analysis, which I didn't stray too far away from: When Roslyn introduced semantic snippets, it broke Razor because it wouldn't offer the if snippet outside of a code block. This is because until you actually write code after an @ in Razor, the compiler doesn't know if it should go in the class, or the render method. Roslyn is "smart" enough not to show if as a completion option if you're at the class level, and of course as soon as you do type @if the Razor compiler is smart enough to put that in the render method, and everyone is happy. Except the user who was just trying to type :) So as a consequence, we turned off semantic snippets for Razor files, as a temporary workaround. 3 years later is still temporary, right? Anyway, that "turn off for Razor files" is probably broken for cohosting, so we're back in this position. So the plan, in #6927, is for us to add back our own snippets for @ if (and maybe @ for, @ foreach etc.) to this list and then I think we might be good.
@ToddGrun and @DustinCampbell had competing PR merges
Co-authored-by: davidwengier <754264+davidwengier@users.noreply.github.com>
Co-authored-by: davidwengier <754264+davidwengier@users.noreply.github.com>
This pull request updates the following dependencies [marker]: <> (Begin:2907dbca-fa2e-42bc-f7dd-08dc0c5b4e6d) ## From https://github.com/dotnet/arcade - **Subscription**: [2907dbca-fa2e-42bc-f7dd-08dc0c5b4e6d](https://maestro.dot.net/subscriptions?search=2907dbca-fa2e-42bc-f7dd-08dc0c5b4e6d) - **Build**: [20251226.5](https://dev.azure.com/dnceng/internal/_build/results?buildId=2867607) ([295620](https://maestro.dot.net/channel/8394/github:dotnet:arcade/build/295620)) - **Date Produced**: December 26, 2025 6:04:22 PM UTC - **Commit**: [d8dca0b41b903e7182e64543773390b969dab96b](dotnet/arcade@d8dca0b) - **Branch**: [release/10.0](https://github.com/dotnet/arcade/tree/release/10.0) [DependencyUpdate]: <> (Begin) - **Dependency Updates**: - From [10.0.0-beta.25609.2 to 10.0.0-beta.25626.5][3] - Microsoft.DotNet.Arcade.Sdk [3]: dotnet/arcade@8d3de8e...d8dca0b [DependencyUpdate]: <> (End) - **Updates to .NET SDKs:** - Updates **sdk.version** to 10.0.101 - Updates **tools.dotnet** to 10.0.101 [marker]: <> (End:2907dbca-fa2e-42bc-f7dd-08dc0c5b4e6d)
# Conflicts: # eng/Version.Details.props
…nProcess/EditorInProcess_Text.cs
…cs (#12629) Fixes #12628 Needs dotnet/roslyn#81822 to compile
…#12630) I was starting to play with removing the cohosting feature flag, and very quickly got to a point where the old LSP server endpoint tests stopped running, and in some cases even compiling, which would mean needing to delete them. Before we do that, I wanted to make sure we had identified any gaps in the coverage of the equivalent cohosting endpoint tests. I tried to get copilot planning mode to create a todo list for this, hoping to then be able to assign it tasks and have it create tests etc. but it didn't work, and it's been pretty quiet while you've all been on vacation anyway, so I did this the old fashioned way :) Should be all test only changes, and each commit is a different feature area.
Fixes #12608 Previous code was making bad assumptions about everything being in one project, and just using hint name to identify a generated document. New code uses SourceGeneratedDocumentIdentity, which can reason about projects, and hides direct hint name use as much as possible.
One of these tests was trying to rename in a .cs file using a position from a Razor file, so that was never going to work. The other one seemed to sometimes just not run the rename command, so I added a small delay. Hopefully that will help.
…ain-to-release/dev18.3
RikkiGibson
reviewed
Jan 6, 2026
| }, | ||
| "branches": { | ||
| "main": { | ||
| "release/dev18.3": { |
Member
There was a problem hiding this comment.
Usually i would expect separate objects for both "main" and "release/dev18.3" to stick around here. You can just set "insertionCreateDraftPR": true for the main branch entry until VS is ready to accept 18.4 PRs to main.
Member
There was a problem hiding this comment.
Maybe things have changed here? Because we use the PublishData file from the current branch being built, we actually only want a property for the "current" branch in this file? @dibarbet
Member
Author
There was a problem hiding this comment.
I like the format in Roslyn better, where there is only one set of info (and I'm presuming its for the "current" branch). Can that be done here?
Member
There was a problem hiding this comment.
I ported the pull from current branch change to Razor, but not the followup changes to remove the branch entirely from the publishdata.json file. So this needs a single entry for dev18.3 (IIRC)
Member
There was a problem hiding this comment.
It sounds like the PublishData is supposed to just have info about the current branch now, so, the way you have it looks good.
dibarbet
approved these changes
Jan 6, 2026
| }, | ||
| "branches": { | ||
| "main": { | ||
| "release/dev18.3": { |
Member
There was a problem hiding this comment.
Believe this is correct. I need to finish porting over the rest of the publishdata changes from Roslyn so we don't have to modify this file as much.
eng/config/PublishData.json
Outdated
Member
There was a problem hiding this comment.
consider changing this to [18.3] (cant put a suggestion for some reason)
Member
Author
There was a problem hiding this comment.
Should I do that in Roslyn too?
Also just realised I need to change Versions.props here too, and in Roslyn
Member
There was a problem hiding this comment.
Should I do that in Roslyn too?
You can, or can do it when we update the vsBranch tomorrow after VS snaps.
Member
Author
There was a problem hiding this comment.
Actually, the PublishData file in the merge branch in Roslyn needs to be updated anyway, its currently rel/d18.3, but should be main for now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the changes
Fixes: