-
Notifications
You must be signed in to change notification settings - Fork 229
Better handle parameter attribute directive completion #12473
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
Better handle parameter attribute directive completion #12473
Conversation
… directive attribute completion. 2) Rename some test files to make diffing easier
…eAttributeCompletionItemProvider
Small cleanup to GetAttributeCompletions to reduce allocations
…round 2) Move local methods to private member methods
…are it for removal in next commit
…e due to the completion classes merging
bed4546 to
5b0dc82
Compare
davidwengier
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.
Maybe I'm misunderstanding something, but I'm beginning to wonder if we still need separate attribute name and parameter logic at all? Can't we just have the logic be "we're going to add a completion item for the attribute, and each parameter, unless they already exist"? I don't understand why there is an attribute name context and a parameter name context, if we're always just adding parameters to completion.
...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/IServiceCollectionExtensions.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| [Fact] | ||
| public void GetCompletionItems_OnDirectiveAttributeName_bind_ReturnsParameterSnippetCompletions() |
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.
This looks identical to the test above?
...or.Workspaces.Test/Completion/DirectiveAttributeCompletionItemProviderTest.ParameterNames.cs
Show resolved
Hide resolved
...or.Workspaces.Test/Completion/DirectiveAttributeCompletionItemProviderTest.ParameterNames.cs
Show resolved
Hide resolved
| var inSnippetContext = InSnippetContext(containingAttribute, razorCompletionOptions); | ||
| using var _ = SpecializedPools.GetPooledStringDictionary<(ImmutableArray<BoundAttributeDescriptionInfo>, ImmutableArray<RazorCommitCharacter>, RazorCompletionItemKind kind)>(out var attributeCompletions); | ||
|
|
||
| // Collect indexer descriptors and their parent tag helper type names |
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.
I have no idea what an indexer descriptor is, and I'm guessing you didn't before you started working in this file. Assuming you've had to find out to do this PR, could you put a comment somewhere explaining (unless its already somewhere not shown in the diff)
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.
I certainly knew nothing about them before this change. I've added a comment outlining my limited understanding and how they are used here.
|
|
||
| foreach (var parameterDescriptor in attributeDescriptor.Parameters) | ||
| { | ||
| if (!context.ExistingAttributes.IsDefault |
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.
Nit (and I'm guessing you didn't write this anyway): The IsDefault check could be outside the loop, though I'm not sure its needed at all with .Any
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.
It is needed. The ImmutableArray extension for Any doesn't check for the underlying array being null. Taking a closer look, there was a logic issue as this should be adding the completion if there are no attributes. Will fix that, but keep inside the loop as it fits there.
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| // Add indexer parameter completions first |
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.
Similar to above, whats an indexer parameter?
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.
Described in the other location, updated comment here to indicating why the ordering here is intentional
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs
Show resolved
Hide resolved
Parameter completion items display differently based on the context: |
|
Ahh okay, so we kinda have the new behaviour and the old behaviour. I definitely missed that. Though I do wonder, is it worth the complexity for the latter screenshot? Presumably if we just returning all of the completion items in full, won't the IDE filter the list to only things that start with Or does it not do it because |
|
That is funky. Okay, I rescind those comments :) |
2) Fix logic error around checking for default IA
| internal record DirectiveAttributeCompletionContext( | ||
| string SelectedAttributeName = "", | ||
| string? SelectedParameterName = null, | ||
| ImmutableArray<string> ExistingAttributes = default, |
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.
Consider changing this type so ExistingAttributes is [] instead of default when not set. Adding a little more code here will help avoid extra IsDefault checks below.
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.
I'm feeling kinda dense here as default params need to be constants. Was there an easier way that you were thinking of rather than the change I made in commit 13?
davidwengier
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.
I think my comments ended up being mostly irrelevant due to my own misunderstanding.
The rest neatly demonstrate why I prefer the more snapshot type of tests in the cohosting completion tests, but migrating all of our current completion tests over is definitely not in scope for this PR!
| { | ||
| public string SelectedAttributeName { get; init; } | ||
| public string? SelectedParameterName { get; init; } | ||
| public ImmutableArray<string> ExistingAttributes { get; init; } |
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.
The current default is only applied if the constructor is used. In other places we have used this pattern to ensure the property is never observable default.
| public ImmutableArray<string> ExistingAttributes { get; init; } | |
| public ImmutableArray<string> ExistingAttributes { get; set => field = value.NullToEmpty(); } = []; |
| public bool InParameterName { get; init; } | ||
| public RazorCompletionOptions Options { get; init; } | ||
|
|
||
| public DirectiveAttributeCompletionContext( |
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.
The fact that all of these are optional makes me wonder if its worth having, versus just having callers use the object initializer syntax. Looks like there is only one caller of this anyway?
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, that's nicer. Thanks!
|
Waiting for @sayedihashimi to give an ok after trying out the validation build. |
|
I'm going through it right now. I should have some info to share soon. Thus far I've found 1 issue when using @Bind=. I'll share the info here soon. |
|
I've tried this out just now and noticed an issue with When I type Here is a video 2025.11.11.razor.editor.bind.issue.01.mp4In testing this, I used copilot to generate some a sample file with a bunch of examples. Below is the file that I used to test. You'll notice a bunch of duplicated elements. I just copied what Copilot created to see what it would be like typing it out. If there are other expressions that I should test, please let me know. |
|
Can you validate whether this is a regression from this change or if it does the same before this change? I'm not reproing on my machine with or without the change. From your video, you don't have the "@Bind" or the "@bind-value:*" entries showing up. Is that consistently the case, or is it a timing sort of issue? |
It wasn't consistent, sometimes it would appear and others not.
VS2026 18.0 (public build) has similar, but different behavior. This doesn't have cohosting option available. Should I test it with a different version that has cohosting? Here is a video 2025.11.11.razor.editor.bind.issue.02.mp4 |
|
@ToddGrun when working with a Blazor web app it's working good. I didn't run into any issues from your changes. I tried the following. |
| [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



Modifies completion in several ways:
Old completion behavior:

New completion behavior:

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2607948
Review by commit might make things a bit easier