Remove RazorCompletionItem.Items property#11360
Conversation
Every RazorCompletionItem creates an instance of ItemCollection and exposes it via an Items property. The Items property constructs the ItemCollection lazily, but that doesn't matter since it accessed immediately after each RazorCompletionItem constructor call to add a "description info" object (used to build completion description tooltips). No other data is stored in the ItemCollection. This is an unnecessary amount of overhead to store a single object per RazorCompletionItem. Internally, ItemCollection is implemented with a ConcurrentDictionary, so its definitely not cheap! The change removes the Items property altogether and provides a DescriptionInfo property that is set via the constructor.
| return _items; | ||
| } | ||
| } | ||
| public static RazorCompletionItem CreateMarkupTransition( |
There was a problem hiding this comment.
I was doing review commit-by-commit - looking at the last commit, I see we added all factory methods to the completion item itself. I personally would've added them in the corresponding provider types as those are already specific to those more specific item instances, but don't have very strong feelings either way.
There was a problem hiding this comment.
The purpose of these factory methods was to ensure that a RazorCompletionItem is always created correctly. To support that goal, I want to enforce that RazorCompletionItem can only be constructed via factory methods. If the factory methods aren't on RazorCompletionItem, the constructor would have to be public and there wouldn't be any enforcement. Plus, it is far more typical from a discoverability perspective for "Create" factory methods for a type to appear on a either the type that is being created or a dedicated factory type.
There was a problem hiding this comment.
I personally would've preferred this added to MarkupTransitionCompletionItemProvider since that class is already specific to MarkupTransition.
FYI that I appreciate you explaining your thinking and that's a good place to start logically. For me, when I look at RazorCompletionItem, I see the logic for MarkupTransition already split across other related areas, such as RazorCompletionItemResolver. So, making RazorCompletionItem a bit more knowledgeable about the RazorCompletionItemKinds that it's constructed with seemed reasonable, and the encapsulation win helps avoid future bugs.
| insertText: SyntaxConstants.TextTagName, | ||
| descriptionInfo: new(CodeAnalysisResources.MarkupTransition_Description), | ||
| commitCharacters: RazorCommitCharacter.CreateArray([">"])); | ||
| } |
There was a problem hiding this comment.
Nit: I that we replaced these checks with ArgHelper.ThrowIfNull in some cases but not in others. Curious about reasoning behind the choices, e.g. why not use ArgHelper.ThrowIfNull here?
There was a problem hiding this comment.
IMO, there isn't a need to use ArgHelper.ThrowIfNull in internal code called exclusively by our code if the calls won't compile when null is passed.
| using Microsoft.AspNetCore.Razor.PooledObjects; | ||
| using Microsoft.CodeAnalysis.Razor.Tooltip; | ||
| using Microsoft.VisualStudio.Editor.Razor; | ||
| using RazorSyntaxNode = Microsoft.AspNetCore.Razor.Language.Syntax.SyntaxNode; |
There was a problem hiding this comment.
There was a problem hiding this comment.
I moved this to the top of the file because C#'s Add Using code action adds new using directives in the wrong place if there are usings at the top of the file and inside the namespace.
Yes, we had a discussion, but I don't remember agreeing to alias SyntaxNode in this way, so there may have been confusion about the outcome. (FWIW, most cases like this in Razor tooling alias SyntaxNode as RazorSyntaxNode.)
RazorSyntaaxNode is actually a superfluous type in the compiler. It sits in the inheritance hierarchy between SyntaxNode and all Razor syntax node types. I suspect that it exists to as a built in "alias" for SyntaxNode to avoid exactly the problem that this using type directive is trying to avoid. In fact, I'm pretty sure we could delete all using RazorSyntaxNode = and using SyntaxNode = directives from Razor tooling code, replace SyntaxNode with RazorSyntaxNode everywhere, and everything would work fine. 😄
There are actually only six types that inherit from SyntaxNode directly rather than RazorSyntaxNode: SyntaxTrivia, SyntaxList (+ 3 variations), and RazorSyntaxNode itself. Given that tooling doesn't ever care about processing these types as syntax nodes, it seems like all of the effort to add using aliases is probably unnecessary. If we just use the real RazorSyntaxNode throughout, there'd be more consistency and that would alleviate any human reader confusion.
RazorCompletionItem implements value equality, but that equality doesn't appear to be used in production code. Looking back through the history of `RazorCompletionItem.cs` it seems that `IEquatable` was added for testing in a8a91e2 when it was a much simpler type. Removing the value equality does not break tests, so I'm removing the Equals override to avoid confusion and subtle equality bugs that might crop up in the future.
`ItemCollection` is essentially a property bag that results in dictionaries being used instead of plain ol' fields, and it's been overused in Razor. I've been trying to get rid of it for nearly a year. #10720, #10764, #11360, and #11931 each represent changes that remove a use of `ItemCollection`. This pull request finally gets rid of `ItemCollection` completely. The last use of `ItemCollection` is the `RazorCodeDocument.Items`, which is used by the compiler to store various objects during compilation, and tooling uses it to cache some expensive-to-create data. This change stores all of the compiler objects on `RazorCodeDocument` directly. The data cached by tooling is stored on the side in a `ConditionalWeakTable`. As part of this, I've refactoring the `RazorCodeDocument.TryComputeNamespace` extension method to be more understandable and maintainable. ---- CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2728842&view=results Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/643246 Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2728843&view=results
4 participants
Every
RazorCompletionItemcreates an instance ofItemCollectionand exposes it via anItemsproperty. The property lazily constructs theItemCollection, but that doesn't really matter much because the property is always accessed. Every time theRazorCompletionItemconstructor is called a "description info" object (used to build completion description tooltips) is immediately added to theItemsproperty. No other data is stored in theItemCollection.It is unnecessary overhead to store a single object per
RazorCompletionItemin anItemCollection. Internally,ItemCollectionis implemented with aConcurrentDictionary, so it's definitely not cheap!This change removes the
Itemsproperty altogether and provides aDescriptionInfoproperty that is set via the constructor. I've also done a fair amount of clean up in separate commits. The first commit contains the meat of removing theItemsproperty.