supporting types from params in intellisense#36040
Conversation
|
Thank you for reporting this, @matoushan! #Resolved |
|
This seems to be a regression introduced by #34312 #Resolved |
|
@jasonmalinowski , @genlu , @dpoeschl , @CyrusNajmabadi , please review #Resolved |
| if (expressionSymbol != null && | ||
| type is INamedTypeSymbol expressionSymbolNamedTypeCandidate && | ||
| expressionSymbolNamedTypeCandidate.OriginalDefinition.Equals(expressionSymbol)) | ||
| var arrayType = method.Parameters.LastOrDefault().Type; |
There was a problem hiding this comment.
LastOrDefault().? #Resolved
| type is INamedTypeSymbol expressionSymbolNamedTypeCandidate && | ||
| expressionSymbolNamedTypeCandidate.OriginalDefinition.Equals(expressionSymbol)) | ||
| var arrayType = method.Parameters.LastOrDefault().Type; | ||
| type = ((IArrayTypeSymbol)arrayType).ElementType; |
There was a problem hiding this comment.
i don't know if it's guaranteed that it will be an array. can you test something like params int i? #Resolved
There was a problem hiding this comment.
also, the compiler/language may not always provide an array here in the future (for example: params IEnumerable). So best to actually explicitly check for the array case and bail otherwise. #Resolved
There was a problem hiding this comment.
param int i is not an allowed syntax: The params parameter must be a single dimensional array.
I agree that we should check if it is an array and bail out otherwise. However, I do not think we should have a test case for the incorrect syntax.
In reply to: 289223266 [](ancestors = 289223266)
There was a problem hiding this comment.
param int i is not an allowed syntax: The params parameter must be a single dimensional array.
I don't think that's strictly true. Namely, the parser totally allows this (though later passes may error if they see somethin that was a non-array).
I agree that we should check if it is an array and bail out otherwise. However, I do not think we should have a test case for the incorrect syntax.
The test case just validates we dont crash :) I don't expect the user experience to be one we cater to. I just want us to not crash, and to have a test that prevents us from regressing in the future :)
| var arrayType = method.Parameters.LastOrDefault().Type; | ||
| type = ((IArrayTypeSymbol)arrayType).ElementType; | ||
| } | ||
| else if (method.Parameters.Length > ordinalInInvocation) |
There was a problem hiding this comment.
can you flip this? this just reads weird to me :) #Resolved
|
Note to reviewers. Turn off whitespace changes. it wil lmake it much easier to review :) #Resolved |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
LGTM with a couple of suggestions/tests to look into.
Approve
Submit feedback approving these changes.
|
@jasonmalinowski , @dpoeschl , @genlu , please review. #Resolved |
1 similar comment
|
@jasonmalinowski , @dpoeschl , @genlu , please review. #Resolved |
|
|
||
| if (type.IsDelegateType()) | ||
| { | ||
| var methods = type.GetMembers(WellKnownMemberNames.DelegateInvokeName); |
There was a problem hiding this comment.
Should we also support params in the case of delegates?
There was a problem hiding this comment.
Thank you, @JoeRobich !
Let us consider the following delegate:
public delegate void Delegate1(Guid g, params Uri[] u);
public void M(Delegate1 d) { }
M((g, u) => { u.$$ })- here the compiler would consideruas an array not as the first element.M((g, u, v) => { $$ })- the delegate does not take 3 arguments.M((Guid g, params Uri[] u) => { })- params is not valid in the context.M((Guid g, Uri[] u) => { u.$$ })- here the compiler would consideruas an array not as the first element.
If you see an example, please let me know. Then, we will make a separate fix for it.
Fixes #36029