Add IDE tests for nameof(parameter) and mitigation for speculation issue#60802
Add IDE tests for nameof(parameter) and mitigation for speculation issue#60802jcouv merged 4 commits intodotnet:features/nameof-parameterfrom
Conversation
| // An invocation may not be considered a 'nameof' operator unless it has one argument | ||
| await VerifyItemIsAbsentAsync(MakeMarkup(source), "parameter"); |
There was a problem hiding this comment.
I think it would be good to support this. It can be very helpful for CallerArgumentExpression too:
void M(int arg, [CallerArgumentExpression(nameof($$))] int parameter) { }It would be good to suggest arg without having to type a
There was a problem hiding this comment.
Agreed, but that will likely have to be left for IDE layer to refine, as compiler API is behaving by-design. I think there's already IDE logic to handle nameof specially, and it would have to be expanded in some later PR.
|
@sharwell for IDE review. This is the outcome of my IDE test pass on |
|
@AlekseyTs @RikkiGibson @dotnet/roslyn-compiler for review (small change to mitigate a binding issue when speculating). Thanks |
src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
Outdated
Show resolved
Hide resolved
|
@AlekseyTs @RikkiGibson @dotnet/roslyn-compiler for review. Thanks |
RikkiGibson
left a comment
There was a problem hiding this comment.
changes under Compilers LGTM
|
@AlekseyTs https://github.com/orgs/dotnet/teams/roslyn-compiler for second review. Thanks |
AlekseyTs
left a comment
There was a problem hiding this comment.
Changes under Compilers LGTM (commit 4)
| int? glyph, int? matchPriority, bool? hasSuggestionItem, string displayTextSuffix, | ||
| string displayTextPrefix, string inlineDescription = null, bool? isComplexTextEdit = null, | ||
| List<CompletionFilter> matchingFilters = null, CompletionItemFlags? flags = null) | ||
| List<CompletionFilter> matchingFilters = null, CompletionItemFlags? flags = null, bool skipSpeculation = false) |
There was a problem hiding this comment.
There's an existing compiler issue with speculation inside attributes on local functions and lambdas (see filed issue from OP). One of the impacts is that nameof doesn't introducing extra identifiers in scope during speculation.
So speculation portion of these IDE tests was skipped for now (tracked with follow-up issue). It's sad that this flag had to be threaded through so many places :-/
| "; | ||
| // Speculation within attributes on local functions is broken | ||
| // Tracked by https://github.com/dotnet/roslyn/issues/60801 | ||
| await VerifyItemExistsAsync(MakeMarkup(source), "parameter", skipSpeculation: true); |
There was a problem hiding this comment.
so that really worries me. is this high pri to fix?
There was a problem hiding this comment.
Brainstormed with Aleksey and it's a bit involved to fix (I have notes from what Aleksey suggested). I likely won't be able to look at it before June. Might ask Rikki if he has bandwidth, since likely affects other scenarios with attributes on local functions and lambdas.
|
@CyrusNajmabadi Did you have any other feedback? Thanks |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
LGTM, as long as we take care of the speculation flag :)
Filed #60801 (issue with speculation inside attributes)
Filed #60812 (issue with completion in empty
nameofin new locations)Test plan #40524