Add in-process HTML completion provider for Razor#83740
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces an in-process HTML completion provider (LocalHtmlCompletionProvider) that produces HTML element, attribute, attribute-value, entity, and close-tag completions directly from the compiled HTML schema, eliminating the LSP round-trip to the external HTML language server for most completion scenarios. The change also collapses the previous two-pass HTML-dependent completion pipeline into a single pass via a new HtmlLabels field on RazorCompletionContext, and adds context-aware commit characters for snippets.
Changes:
- Add
LocalHtmlCompletionProvider(and supportingEntityCompletion,PositionContext,PositionKind, fallback logic, resolve context, image monikers) producing HTML completions in-proc from the compiled schema; fallback to the external HTML server is preserved for SVG, script/style bodies, file-path attributes, CSS class/style values, and multivalue attributes. - Remove the
IHtmlDependentCompletionItemProvider/RazorHtmlDependentCompletionContext/GetHtmlDependentCompletionsAsynctwo-pass pipeline; tag-helper element providers now consume HTML labels inline viaRazorCompletionContext.HtmlLabels. Updated merger to dematerialize differingEditRangevalues and optimizer to omitTextEditTextwhen equal toLabel. - Snippet provider now accepts
validElementNames/RazorCompletionOptions/isStartTagContext, filters by valid children, excludes Razor-incompatible Web Forms snippets, applies element commit characters in start-tag context, setsSortTextto sort after elements, andSnippetCache.GetSnippetsreturns empty instead of throwing before async population finishes.
Reviewed changes
Copilot reviewed 52 out of 53 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
LocalHtmlCompletionProvider.cs (+ .EntityCompletion, .PositionContext, .PositionKind, _Fallback) |
New in-process HTML completion provider and supporting types. |
LocalHtmlCompletionResolveContext.cs, HtmlCompletionImageMonikers.cs |
Resolve context for description/url and image monikers for completion icons. |
RemoteCompletionService.cs, IRemoteCompletionService.cs, CompletionResult.cs, CompletionPositionInfo.cs |
Routes local HTML completions through the existing OOP service; removed two-phase HTML-dependent service method; new HtmlCompletionList / IsStartTagContext fields. |
CohostDocumentCompletionEndpoint.cs, CohostDocumentCompletionResolveEndpoint.cs |
Awaits OOP completion synchronously, falls back to external HTML server when local is null, applies snippet context filtering, strips leading < from start-tag snippet bodies on resolve. |
RazorCompletionListProvider.cs, RazorCompletionContext.cs, TagHelperCompletionProvider.cs, AbstractRazorCompletionFactsService.cs, IRazorCompletionFactsService.cs, IHtmlDependentCompletionItemProvider.cs, RazorHtmlDependentCompletionContext.cs, CompletionItemsResult.cs |
Collapses the two-pass HTML-dependent provider pattern into single-pass with HtmlLabels. |
DirectiveAttributeCompletionItemProvider.cs |
Replacement range now includes @; ComputeInsertText simplified; FilterText now uses DisplayText. |
DelegatedCompletionHelper.cs |
Rewritten ShouldIncludeSnippets to output isStartTagContext. |
CompletionListMerger.cs, CompletionListOptimizer.cs, DefaultCommitCharacters.cs, VSInternalCompletionItemExtensions.cs, RazorCommitCharacter.cs |
EditRange-aware merging; promote-when-equal-to-label optimization; cached element commit character string[]. |
SnippetCompletionItemProvider.cs, SnippetCache.cs, SnippetInfo.cs, ISnippetCompletionItemProvider.cs, SnippetCompletionResolutionContext.cs |
New signature; tag-name mapping; Razor exclusions; SortText; cache returns empty before population. |
Test files (LocalHtmlCompletionProviderTest, SnippetCacheTest, Cohost*Test, RazorCompletionListProviderTest, DirectiveAttributeCompletionItemProviderTest, CompletionListMergerTest, CompletionListOptimizerTest, ThrowingSnippetResolveProvider) |
Adjusted assertions for new behaviors, new coverage for local provider, edit-range merging, sort text, and snippet stripping. |
Microsoft.VisualStudio.RazorExtension.csproj, RazorCompletionImages.imagemanifest, source.extension.vsixmanifest, Microsoft.CodeAnalysis.Razor.Workspaces.csproj |
VSIX wiring for new completion image asset and HtmlDescriptions.resx logical name. |
- Fix HasEndTag bug: check owner.Parent (the element node) instead of owner (the start-tag node) when determining if an implicitly-closable element has an end tag. Without this, the guard was a no-op since the element being typed never has an end tag yet. - Harden strip-'<' check in snippet resolve: require Length > 1 and char.IsLetter(insertString[1]) to avoid stripping '<' from non-element content (e.g., "< " or "<=" expressions). - Add <remarks> to s_emptyCompletionList documenting the singleton safety contract (Items.Length > 0 guard in RemoteCompletionService). - Fix test: assert on 'autocomplete' (non-boolean attribute with a value) instead of 'autofocus' (boolean attribute that doesn't take a value). - Fix implicit closure tests to use unclosed elements (no </li>) since the feature only offers siblings when the parent lacks an end tag. - Add test: closed element does NOT offer parent as sibling.
- Fix misleading comment on ShouldFallBackForElements branch: document it as a defensive guard since FindEffectiveParentTagName currently only returns schema-known names. - Remove redundant resolveContext = Empty assignment before return in GetElementCompletionList. - Fix potential null in CreateDocumentation: use value ?? "" instead of value! for MarkupContent.Value. - Add comment explaining why char.IsLetter check in strip-'<' logic intentionally excludes "<!", "</", "<?" prefixes. - Remove redundant StringComparer.OrdinalIgnoreCase argument from ToFrozenDictionary (already inherited from source dictionary). - Reword IsStartTagContext XML doc to describe cursor state rather than consumer behavior.
|
/pr-val |
|
View PR Validation Run triggered by @ToddGrun Parameters
|
davidwengier
left a comment
There was a problem hiding this comment.
I did not review this as much as the previous one, but it looks fine. I'm going to give up on the snippet changes needing an option, I think we can just wait and see if people complain. I suspect the new start tag stuff, where it trims off <, might just appease 90% of people. though it's technically possible that someone had a snippet that is a plain piece of text, hopefully there aren't many, and not appearing during typing is definitely nicer, and should help with Emmet support.
I am still obliged to mention though that I thought we said in that meeting with Sayed this would only be for VS users, since those are the completions we are baking in. But I'm not going to block on it if you have other desires, I'll just assign the VS Code feedback to you if it comes in :)
Introduces LocalHtmlCompletionProvider, which provides HTML tag, attribute, attribute value, entity, and close-tag completions directly in-process rather than delegating to an external HTML language server. This enables faster completions with lower latency and removes the dependency on the HTML LSP for basic completion scenarios. Falls back to the external HTML LSP for SVG content and script/style blocks where our schema has no data. Simplifies the completion pipeline by eliminating the two-pass approach (GetCompletionItems + GetHtmlDependentCompletionItems). Tag helper completions that need HTML labels now receive them via RazorCompletionContext.HtmlLabels in a single pass, removing IHtmlDependentCompletionItemProvider, RazorHtmlDependentCompletionContext, CompletionItemsResult, and the GetHtmlDependentCompletionsAsync remote service method. Also includes: - Context-aware commit characters for snippets — snippets in start-tag context commit like elements, while snippets in text content have no commit characters - DefaultCommitCharacters.GetElementCommitCharacterStrings() for shared cached string[] instances across providers - Image monikers for HTML completion items - SnippetCache.GetSnippets returns empty before async population completes (instead of throwing) Builds on dotnet#83739
- Fix HasEndTag bug: check owner.Parent (the element node) instead of owner (the start-tag node) when determining if an implicitly-closable element has an end tag. Without this, the guard was a no-op since the element being typed never has an end tag yet. - Harden strip-'<' check in snippet resolve: require Length > 1 and char.IsLetter(insertString[1]) to avoid stripping '<' from non-element content (e.g., "< " or "<=" expressions). - Add <remarks> to s_emptyCompletionList documenting the singleton safety contract (Items.Length > 0 guard in RemoteCompletionService). - Fix test: assert on 'autocomplete' (non-boolean attribute with a value) instead of 'autofocus' (boolean attribute that doesn't take a value). - Fix implicit closure tests to use unclosed elements (no </li>) since the feature only offers siblings when the parent lacks an end tag. - Add test: closed element does NOT offer parent as sibling.
- Fix misleading comment on ShouldFallBackForElements branch: document it as a defensive guard since FindEffectiveParentTagName currently only returns schema-known names. - Remove redundant resolveContext = Empty assignment before return in GetElementCompletionList. - Fix potential null in CreateDocumentation: use value ?? "" instead of value! for MarkupContent.Value. - Add comment explaining why char.IsLetter check in strip-'<' logic intentionally excludes "<!", "</", "<?" prefixes. - Remove redundant StringComparer.OrdinalIgnoreCase argument from ToFrozenDictionary (already inherited from source dictionary). - Reword IsStartTagContext XML doc to describe cursor state rather than consumer behavior.
The static s_cache used a SumType<string[], VSInternalCommitCharacter[]> as its value, which can only hold one representation at a time. When ToVsCommitCharacters cached VSInternalCommitCharacter[] (for sources with Insert=false) and ToStringCommitCharacters was later called for the same source, it called .First on the SumType and threw InvalidCastException. Change the cache value to a tuple (string[]?, VSInternalCommitCharacter[]?) so both representations are stored independently and neither path clobbers the other.
e2c4b97 to
e2e2bc3
Compare
VSCode testing is on my list for tomorrow. I'm guessing I'm going to have to make some tweaks, probably starting with stopping the local html completion depending on how much it's changing the experience. |

Introduces LocalHtmlCompletionProvider, which provides HTML tag, attribute, attribute value, entity, and close-tag completions directly in-process rather than delegating to an external HTML language server. This enables faster completions with lower latency and removes the dependency on the HTML LSP for basic completion scenarios. Falls back to the external HTML LSP for SVG content and script/style blocks where our schema has no data.
Simplifies the completion pipeline by eliminating the two-pass approach (GetCompletionItems + GetHtmlDependentCompletionItems). Tag helper completions that need HTML labels now receive them via RazorCompletionContext.HtmlLabels in a single pass, removing IHtmlDependentCompletionItemProvider, RazorHtmlDependentCompletionContext, CompletionItemsResult, and the GetHtmlDependentCompletionsAsync remote service method.
Also includes:
- Context-aware commit characters for snippets — snippets in start-tag context commit like elements, while snippets in text content have no commit characters
- DefaultCommitCharacters.GetElementCommitCharacterStrings() for shared cached string[] instances across providers
- Image monikers for HTML completion items
- SnippetCache.GetSnippets returns empty before async population completes (instead of throwing)
Not as wide spread of an improvement as I was hoping, but at least there were a couple scenarios that showed good improvement:
Microsoft Reviewers: Open in CodeFlow