-
Notifications
You must be signed in to change notification settings - Fork 229
TagHelperCollection Part 2: Mechanical Updates #12505
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
TagHelperCollection Part 2: Mechanical Updates #12505
Conversation
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Rename/RenameService.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Rename/RenameService.cs
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.VisualStudio.LanguageServices.Razor/Discovery/OutOfProcTagHelperResolver.cs
Outdated
Show resolved
Hide resolved
...or.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.DefaultCommitCharacters.cs
Show resolved
Hide resolved
| var descriptionInfo = BoundAttributeDescriptionInfo.From(attribute, isIndexer, attribute.Parent.TypeName); | ||
|
|
||
| if (!TryAddCompletion(attributeDescriptor.Name, descriptionInfo, descriptor, context, RazorCompletionItemKind.DirectiveAttribute, attributeCompletions) && attributeDescriptor.Parameters.Length > 0) | ||
| var tagHelper = attribute.Parent; |
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.
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.
Yeah, I added the Parent property awhile back: #11953
I didn't do a survey of the code base to update everything that could take advantage of Parent. I've been updating code as I see opportunities though. For example...
Lines 20 to 22 in 80cf794
| public BoundAttributeParameterDescriptor BoundAttributeParameter => _match.Parameter.AssumeNotNull(); | |
| public BoundAttributeDescriptor BoundAttribute => BoundAttributeParameter.Parent; | |
| public TagHelperDescriptor TagHelper => BoundAttribute.Parent; |
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs
Show resolved
Hide resolved
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs
Outdated
Show resolved
Hide resolved
| Prelude | [Part 1](#12504) | [Part 2](#12505) | [Part 3](#12506) | [Part 4](#12507) | [Part 5](#12509) | This is a set of support utilities and helpers added as part of the `TagHelperCollection` changes: - Added `InterlockedOperations.Initialize(...)` overloads for lock free initialization with a factory method. (f3686aa) - Added two `LazyValue` structs that use the new `InterlockedOperations.Initialize(...)` overloads. These are useful for lazily initializing a value without extra allocations or locks. (81795f5) - Added `OverloadResolutionPriorityAttribute` polyfill type. (27ab174) - Refactored `TagHelperCache` to extract `CleanableWeakCache<TKey, TValue>`. This caches items as weak references. When the number of items added reaches a configurable threshold, the cache is cleaned up, and any dead weak references are removed. (673a9b5) ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842136&view=results Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842197&view=results
ToddGrun
left a comment
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.
![]()
| [Prelude](#12503) | Part 1 | [Part 2](#12505) | [Part 3](#12506) | [Part 4](#12507) | [Part 5](#12509) | This pull request represents all new code! It introduces a new immutable collection type, `TagHelperCollection`, that is designed to contain `TagHelperDescriptors`. It is built with the following principles: 1. Guarantee that collections never contain duplicate tag helpers. 2. Collections can be compared efficiently via checksums. 3. Multiple collections can be merged into a single collection efficiently, with minimal array copying. 4. Determining whether a collection contains a tag helper is efficient. > [!TIP] > Since this is all new code, I recommend reviewing commit-by-commit. Each commit represents a cohesive portion of new code. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842145&view=results Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2842216&view=results
Note: Because ProjectWorkspaceState.TagHelpers now returns a TagHelperCollection, the serialization format needs to be updated.
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.
ae9dd61 to
fe38743
Compare
…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
| [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
…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
| Prelude | Part 1 | Part 2 | Part 3 | Part 4 | Part 5 |
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
TagHelperCollectionacross the Razor code base. Most references toImmutableArrary<TagHelperDescriptor>,IReadOnlyList<TagHelperDescriptor>,IEnumerable<TagHelperDescriptor>, andTagHelperDescriptor[]have been replaced withTagHelperCollection. This is by far the largest of theTagHelperCollectionpull 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
RenameServiceto removeImmutableArray<TagHelperDescriptor>(fa3ad2b)This includes a fair amount of refactoring in
RenameServiceto fix bugs found when moving toTagHelperCollection.Update
TagHelperFactsto useTagHelperCollectionExtra work was done in
DirectiveAttributeComplationItemProviderto clean up a bit following Better handle parameter attribute directive completion #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