Escape keywords in object initializer completion#26740
Escape keywords in object initializer completion#26740ivanbasov merged 3 commits intodotnet:masterfrom
Conversation
|
cc @jcouv |
| { | ||
| context.AddItem(SymbolCompletionItem.CreateWithSymbolId( | ||
| displayText: uninitializedMember.Name, | ||
| displayText: uninitializedMember.ToNameDisplayString(), |
There was a problem hiding this comment.
Since this code is shared for C# and VB, please also add a corresponding VB test. Thanks
jcouv
left a comment
There was a problem hiding this comment.
Needs a VB test, but otherwise looks good (iteration 1).
|
Note: test failures look real. |
If the member was a VB property with parameter(s) then the completion would not be correct as `ToNameDisplayString()` also includes the parameter(s). Hence, we only use `ToNameDisplayString()`, if the member does not have parameters, otherwise we keep the old behaviour.
|
I've pushed a test for VB and also corrected the logic. The problem was that for VB properties with parameters, the parameters would also be visible in the completion which did not make sense. So now we only use Illustration of the problem (highlighted by the two VB tests) |
|
|
||
| foreach (var uninitializedMember in uninitializedMembers) | ||
| { | ||
| var displayText = uninitializedMember.GetParameters().Length == 0 ? uninitializedMember.ToNameDisplayString() : uninitializedMember.Name; |
There was a problem hiding this comment.
i don't think we want ToNameDisplayString at all.
Something is causing the insertion codepath to not go through CompletionUtilities.GetInsertionText (which would properly escape the string as necessary).
There was a problem hiding this comment.
The problem here seems to be that AbstractObjectINitializerCompletionProvider does not do the same things as AbstractSymbolCompletionProvider where it asks what the appropriate 'insertion' text should be.
There was a problem hiding this comment.
@CyrusNajmabadi I didn't fully understand your comment, but wanted to mention that in the completion for patterns (which had the same problem with escaping keywords), I used untestedMember.Name.EscapeIdentifier(). Would that be appropriate here as well?
See https://github.com/dotnet/roslyn/pull/26553/files#diff-71f1153caf63996a37515baedd143adbR53
There was a problem hiding this comment.
looking at this code overall... yup, i think that would be sufficient!
|
|
||
| foreach (var uninitializedMember in uninitializedMembers) | ||
| { | ||
| var displayText = uninitializedMember.GetParameters().Length == 0 ? uninitializedMember.ToNameDisplayString() : uninitializedMember.Name; |
There was a problem hiding this comment.
I'm not sure that checking the number of parameters is the correct way to filter out such members. It feels like we should look at the kind of member (field or properties are good for this completion, but methods are not).
There was a problem hiding this comment.
Agreed. This is patching things up at the wrong level. We have a mechanism to do this already in normal symbol completion. But unfortunately, we're not doing the same thing here. The right fix is to adopt that strategy. @mikkelbu I can provide more information if my explanation so far wasn't clear.
There was a problem hiding this comment.
@CyrusNajmabadi I think I need some more information. Originally when I looked into this issue I was also by AbstractSymbolCompletionProvider and I looked at CompletionUtilities.GetDisplayAndInsertionText, but I failed to discover that CompletionUtilities exists both as a C# and a VB class, so I took another approach.
Are you suggesting that I make a similar abstract method in AbstractObjectINitializerCompletionProvider and then use CompletionUtilities.GetDisplayAndInsertionText in the derived classes in C# and VB? And if so, should I also have something similar to the special handling in ObjectCreationCompletionProvider.GetDisplayAndInsertionText(...) and AbstractObjectCreationCompletionProvider.GetDisplayAndInsertionText(...).
Ps. GetDisplayAndInsertionText is using EscapeIdentifier to create the insertion text.
There was a problem hiding this comment.
That was my original suggestion for the overall 'best' approach here.
However, i think julien's suggestions is just as reasonable and far easier to accomplish. So i would just go with that.
Cheers!
|
@dotnet/roslyn-ide please review this community contribution. Thanks |
|
|
||
| <WorkItem(26560, "https://github.com/dotnet/roslyn/issues/26560")> | ||
| <Fact, Trait(Traits.Feature, Traits.Features.Completion)> | ||
| Public Async Function TestKeywordsEscaped() As Task |
There was a problem hiding this comment.
nit: it would be nice if this had the same name as the c# test (so it's easier to find both)
|
Thank you very much for your contribution, @mikkelbu, and for your help with review, @CyrusNajmabadi! |
|
sorry! closed it before merged. will have to wait for tests to complete again. will merge just after that. |
Fixes #26560
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.