-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Implement SymbolInfo for the property name of a property pattern #9233
Conversation
@@ -496,6 +496,11 @@ internal virtual IOperation GetOperationWorker(CSharpSyntaxNode node, GetOperati | |||
public abstract SymbolInfo GetSymbolInfo(SelectOrGroupClauseSyntax node, CancellationToken cancellationToken = default(CancellationToken)); | |||
|
|||
/// <summary> | |||
/// Gets the symbol information for the property of a sub-property pattern. | |||
/// </summary> | |||
public abstract SymbolInfo GetSymbolInfo(SubPropertyPatternSyntax node, CancellationToken cancellationToken); |
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.
Should add a default value for cancellationToken
here.
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.
Fine, though there are no clients that would use it. The enclosing class is internal.
OK, I've made a bunch of changes per review comments. Please let me know if there are any new issues. |
Implement SymbolInfo for the property name of a property pattern
p = propPats[1]; // NotFound is 4 | ||
si = model.GetSymbolInfo(p); | ||
Assert.Null(si.Symbol); | ||
Assert.Equal(CandidateReason.None, si.CandidateReason); |
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.
Missing test for LookupResultKind.Ambiguous, it looks like this change explicitly introduces a case when it can happen.
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.
Agreed. Filed a separate PR for that, and a new bug because the SymbolInfo
does not contain candidates.
LGTM |
Fixes #9072 #9073 #9079 #9080
/cc @jaredpar @AlekseyTs @CyrusNajmabadi FYI