Remove ItemCollection from TagHelperDescriptorProviderContext #10720
Conversation
This file contains 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
Every single `TagHelperDescriptorProviderContext` created in Razor compiler or tooling code adds a valid compilation. So, adding the compilation to the `ItemsCollection` is pure overhead and no `ITagHelperDescriptorProvider` needs to check it for null. I removed a single test that verified the behavior if a compilation was never set. Now that a compilation is required, this test is no longer needed.
This can be safely removed now that there are no more references.
- Enable nullability - Mark all as sealed - Use `Lazy<T>` for singleton `ITagHelperDescriptorProviders` - Introduce `TagHelperDescriptorProviderBase` - Inherit from `TagHelperDescriptorProviderBase` - Remove unnecessary properties - Remove unnecessary property setters In addition, I spotted a bug in `EventHandlerTagDescriptorProvider` while addressing nullability warnings. The buggy code is right here: https://github.com/dotnet/razor/blob/fb84ae5d9bb8132972c2c23cf209721161f81deb/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/EventHandlerTagHelperDescriptorProvider.cs#L70-L76 When reading the constructor arguments of an `EventHandlerAttribute` it appears that we swap the last two arguments of the 4-argument variant. The constructor is defined like so: ```C# EventHandlerAttribute(string attributeName, Type eventArgsType, bool enableStopPropagation, bool enablePreventDefault); ``` Unfortunately, the compiler reads the third parameter as `enablePreventDefaeult` and the fourth parameter as `enableStopPropagation`. This has been broken since support for the 4-argument constructor variant was added in dotnet@7635bba. Fixing this bug is tracked by dotnet#10497.
333fred
approved these changes
Aug 8, 2024
jaredpar
pushed a commit
that referenced
this pull request
Aug 12, 2024
Every `TagHelperDescriptorProviderContext` creates an `ItemCollection` to hold onto, at most, two objects: a `Compilation` and a target `ISymbol`. This is enormously wasteful because an `ItemCollection` internally creates a `ConcurrentDictionary<object, object>`. So, I've changed `TagHelperDescriptorProviderContext` to just hold onto a compilation and a target symbol and avoid the `ItemCollection` altogether. I recommend reviewing commit-by-commit. Also, I bumped into a long standing compiler bug in `EventHandlerTagHelperDescriptorProvider` that was recently filed by a customer: #10497. I opted to stay on target and not fix this issue, but I made it painfully obvious where the fix would go. 😄
This was referenced Aug 15, 2024
DustinCampbell
added a commit
that referenced
this pull request
Aug 22, 2024
Because of a dare from @333fred, I'm currently on a crusade to remove `ItemCollection` from the Razor Compiler completely. Previously, I removed `ItemCollection` from `TagHelperDescriptorProviderContext` (#10720). This time, I'm removing it from `CodeRenderingContext`. It turns out that every `CodeRenderingContext` greedily creates an `ItemCollection`, though there are only ever two things stored there: 1. A string to use for new lines in `CodeWriter`. 2. A string representing "suppress IDs", which is already specified in `RazorCodeGenerationOptions`. These two items were really present as a hook that compiler tests could set. However, it's much easier and less wasteful to elevate both items to `RazorCodeGenerationOptions` and make tests set the options directly. I made a few other refactorings as part of this change: - I merged several abstract base classes with their single default concrete type: - `CodeRenderingContext` and `DefaultCodeRenderingContext` - `RazorCSharpDocument` and `DefaultRazorCSharpDocument`, - `RazorCodeGenerationOptions` and `DefaultRazorCodeGenerationOptions` - `RazorCodeGenerationOptionsBuilder` and `DefaultRazorCodeGenerationOptionsBuilder`. - Reworked `RazorCodeGenerationOptions` and introduced `RazorCodeGenerationOptionsFlags` to track its boolean fields. - Cleaned up `RazorCSharpDocument` and unified its collections on `ImmutableArray<>` rather than `IReadOnlyList<>`. - Enabled nullability annotations for several types. - Used more pooled collections in `CodeRenderingContext`. Note: I was careful with my commit history, and it should be clean enough to review commit-by-commit if that's your preference.
DustinCampbell
added a commit
that referenced
this pull request
Sep 3, 2024
…#10828) This fixes an issue that @tmat pointed out to me over email. In a recent change (#10720), I added a call to `Assumed.Unreachable()` when `[EventHandler]` tag helper discovery encounters an attribute with invalid constructor attributes. However, throwing an exception during tag helper discovery is usually the wrong approach. Normally, if Roslyn symbols aren't in the proper shape during tag helper discovery, Razor will simply not produce a tag helper. (We _do_ support diagnostics for tag helpers, but those are usually reserved for warnings and errors that are related to a tag helper's data that would make it unusable, such as a name containing whitespace.) It turns out that the "unreachable" condition wasn't actually all that unreachable and @tmat was hitting it while working on hot reload tests. So, I've changed the code to make the success conditions clearer, i.e., the attribute data must match one of the two constructor calls. And, I changed the logic to simply skip `[EventHandler]` attributes that don't meet the success conditions.
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.
Every
TagHelperDescriptorProviderContextcreates anItemCollectionto hold onto, at most, two objects: aCompilationand a targetISymbol. This is enormously wasteful because anItemCollectioninternally creates aConcurrentDictionary<object, object>. So, I've changedTagHelperDescriptorProviderContextto just hold onto a compilation and a target symbol and avoid theItemCollectionaltogether.I recommend reviewing commit-by-commit.
Also, I bumped into a long standing compiler bug in
EventHandlerTagHelperDescriptorProviderthat was recently filed by a customer: #10497. I opted to stay on target and not fix this issue, but I made it painfully obvious where the fix would go. 😄