-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,7 +120,13 @@ private BoundPattern BindRecursivePattern(RecursivePatternSyntax node, BoundExpr | |
// use a dedicated bound node for that form. | ||
var properties = correspondingMembers; | ||
var boundPatterns = BindRecursiveSubPropertyPatterns(node, properties, type, diagnostics); | ||
return new BoundPropertyPattern(node, type, boundPatterns, properties, hasErrors: hasErrors); | ||
var builder = ArrayBuilder<BoundSubPropertyPattern>.GetInstance(properties.Length); | ||
for (int i = 0; i < properties.Length; i++) | ||
{ | ||
builder.Add(new BoundSubPropertyPattern(node.PatternList.SubPatterns[i], properties[i].GetTypeOrReturnType(), properties[i], LookupResultKind.Empty, boundPatterns[i], hasErrors)); | ||
} | ||
|
||
return new BoundPropertyPattern(node, type, builder.ToImmutableAndFree(), hasErrors: hasErrors); | ||
} | ||
|
||
private ImmutableArray<BoundPattern> BindRecursiveSubPropertyPatterns(RecursivePatternSyntax node, ImmutableArray<Symbol> properties, NamedTypeSymbol type, DiagnosticBag diagnostics) | ||
|
@@ -162,21 +168,20 @@ private BoundPattern BindPropertyPattern(PropertyPatternSyntax node, BoundExpres | |
{ | ||
var type = (NamedTypeSymbol)this.BindType(node.Type, diagnostics); | ||
hasErrors = hasErrors || CheckValidPatternType(node.Type, operand, operandType, type, false, diagnostics); | ||
var properties = ArrayBuilder<Symbol>.GetInstance(); | ||
var boundPatterns = BindSubPropertyPatterns(node, properties, type, diagnostics); | ||
hasErrors |= properties.Count != boundPatterns.Length; | ||
return new BoundPropertyPattern(node, type, boundPatterns, properties.ToImmutableAndFree(), hasErrors: hasErrors); | ||
var boundPatterns = BindSubPropertyPatterns(node, type, diagnostics); | ||
return new BoundPropertyPattern(node, type, boundPatterns, hasErrors: hasErrors); | ||
} | ||
|
||
private ImmutableArray<BoundPattern> BindSubPropertyPatterns(PropertyPatternSyntax node, ArrayBuilder<Symbol> properties, TypeSymbol type, DiagnosticBag diagnostics) | ||
private ImmutableArray<BoundSubPropertyPattern> BindSubPropertyPatterns(PropertyPatternSyntax node, TypeSymbol type, DiagnosticBag diagnostics) | ||
{ | ||
var boundPatternsBuilder = ArrayBuilder<BoundPattern>.GetInstance(); | ||
var result = ArrayBuilder<BoundSubPropertyPattern>.GetInstance(); | ||
foreach (var syntax in node.PatternList.SubPatterns) | ||
{ | ||
var propName = syntax.Left; | ||
BoundPattern pattern; | ||
HashSet<DiagnosticInfo> useSiteDiagnostics = null; | ||
Symbol property = FindPropertyByName(type, propName, ref useSiteDiagnostics); | ||
LookupResultKind resultKind; | ||
Symbol property = FindPropertyOrFieldByName(type, propName, out resultKind, ref useSiteDiagnostics); | ||
if ((object)property != null) | ||
{ | ||
bool hasErrors = false; | ||
|
@@ -190,39 +195,51 @@ private ImmutableArray<BoundPattern> BindSubPropertyPatterns(PropertyPatternSynt | |
diagnostics.Add(node, useSiteDiagnostics); | ||
} | ||
|
||
properties.Add(property); | ||
pattern = this.BindPattern(syntax.Pattern, null, property.GetTypeOrReturnType(), hasErrors, diagnostics); | ||
var propertyType = property.GetTypeOrReturnType(); | ||
pattern = this.BindPattern(syntax.Pattern, null, propertyType, hasErrors, diagnostics); | ||
result.Add(new BoundSubPropertyPattern(syntax, propertyType, property, resultKind, pattern, hasErrors)); | ||
} | ||
else | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_NoSuchMember, propName, type, propName.Identifier.ValueText); | ||
Error(diagnostics, ErrorCode.ERR_NoSuchMember, propName, type, propName.ValueText); | ||
pattern = new BoundWildcardPattern(node, hasErrors: true); | ||
result.Add(new BoundSubPropertyPattern(syntax, CreateErrorType(), null, resultKind, pattern, true)); | ||
} | ||
|
||
boundPatternsBuilder.Add(pattern); | ||
} | ||
|
||
return boundPatternsBuilder.ToImmutableAndFree(); | ||
return result.ToImmutableAndFree(); | ||
} | ||
|
||
private Symbol FindPropertyByName(TypeSymbol type, IdentifierNameSyntax name, ref HashSet<DiagnosticInfo> useSiteDiagnostics) | ||
private Symbol FindPropertyOrFieldByName(TypeSymbol type, SyntaxToken name, out LookupResultKind resultKind, ref HashSet<DiagnosticInfo> useSiteDiagnostics) | ||
{ | ||
var symbols = ArrayBuilder<Symbol>.GetInstance(); | ||
var result = LookupResult.GetInstance(); | ||
this.LookupMembersWithFallback(result, type, name.Identifier.ValueText, arity: 0, useSiteDiagnostics: ref useSiteDiagnostics); | ||
var lookupResult = LookupResult.GetInstance(); | ||
this.LookupMembersWithFallback(lookupResult, type, name.ValueText, arity: 0, useSiteDiagnostics: ref useSiteDiagnostics); | ||
resultKind = lookupResult.Kind; | ||
Symbol result = null; | ||
|
||
if (result.IsMultiViable) | ||
if (lookupResult.IsMultiViable) | ||
{ | ||
foreach (var symbol in result.Symbols) | ||
foreach (var symbol in lookupResult.Symbols) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A property can be overloaded, it is not clear why it is a right thing to grab the first one. Perhaps we should grab the first one that doesn't have parameters? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can even have more than one with no parameters due to multiple interface inheritance. This code should probably defer to lookup code elsewhere to do the disambiguation, and then give an error if the result isn't as expected. Filed a language issue as #9255. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, reworked this code to report ambiguous if there is more than one property. |
||
{ | ||
if (symbol.Kind == SymbolKind.Property || symbol.Kind == SymbolKind.Field) | ||
{ | ||
return symbol; | ||
if (result != null && symbol != result) | ||
{ | ||
resultKind = LookupResultKind.Ambiguous; | ||
result = null; | ||
break; | ||
} | ||
else | ||
{ | ||
result = symbol; | ||
} | ||
} | ||
} | ||
} | ||
|
||
return null; | ||
lookupResult.Free(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we are leaking lookupResult when we are returning from inside of the loop above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. Should probably either be freed there too or done in a try-finally. |
||
return result; | ||
} | ||
|
||
private BoundPattern BindConstantPattern(ConstantPatternSyntax node, BoundExpression operand, TypeSymbol operandType, bool hasErrors, DiagnosticBag diagnostics) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -430,6 +430,22 @@ public static Conversion ClassifyConversion(this Compilation compilation, ITypeS | |
} | ||
} | ||
|
||
/// <summary> | ||
/// Gets the symbol information for the property of a sub-property pattern. | ||
/// </summary> | ||
public static SymbolInfo GetSymbolInfo(this SemanticModel semanticModel, SubPropertyPatternSyntax node, CancellationToken cancellationToken = default(CancellationToken)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure it is appropriate to support GetSymbolInfo for SubPropertyPatternSyntax. The syntax node doesn't represent a reference to a symbol, its child node does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That appears to be the current pattern we use in SemanticModel (e.g. it is done this way for constructor initializer and attribute). You can also get the symbol from the Left child node. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a similarity, ConstructorInitializerSyntax actually represents a constructor reference. Also, I believe SymbolInfo cannot be requested for a token and base/this are tokens. So, there is only one node that can be used to get information about the constructor used. Similar with an attribute, it represents a constructor reference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify my position, I think that, given SubPropertyPatternSyntax node, one should pass SubPropertyPatternSyntax.Left to get SymbolInfo for the target property or a field. If SubPropertyPatternSyntax node itself is passed, SymbolInfo.None should be returned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'm changing |
||
{ | ||
var csmodel = semanticModel as CSharpSemanticModel; | ||
if (csmodel != null) | ||
{ | ||
return csmodel.GetSymbolInfo(node, cancellationToken); | ||
} | ||
else | ||
{ | ||
return SymbolInfo.None; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Returns what symbol(s), if any, the given expression syntax bound to in the program. | ||
/// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -937,6 +937,23 @@ internal override Optional<object> GetConstantValueWorker(CSharpSyntaxNode node, | |
return GetTypeInfoForQuery(bound); | ||
} | ||
|
||
/// <summary> | ||
/// Gets the symbol information for the property of a sub-property pattern. | ||
/// </summary> | ||
public override SymbolInfo GetSymbolInfo(SubPropertyPatternSyntax node, CancellationToken cancellationToken) | ||
{ | ||
var boundNode = GetLowerBoundNode(node) as BoundSubPropertyPattern; | ||
if (boundNode != null) | ||
{ | ||
var property = boundNode.Property; | ||
return new SymbolInfo(property, boundNode.ResultKind == LookupResultKind.Viable ? CandidateReason.None : boundNode.ResultKind.ToCandidateReason()); | ||
} | ||
else | ||
{ | ||
return default(SymbolInfo); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SymbolInfo.None ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are not the same. I'd prefer to use the same pattern as is currently used elsewhere in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See line 919 in this file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm aware of the existing inconsistency - there is that one place where we do it differently. I'm not intending to address that inconsistency now, nor do I want to add to it. |
||
} | ||
} | ||
|
||
private void GetBoundNodes(CSharpSyntaxNode node, out CSharpSyntaxNode bindableNode, out BoundNode lowestBoundNode, out BoundNode highestBoundNode, out BoundNode boundParent) | ||
{ | ||
bindableNode = this.GetBindableSyntaxNode(node); | ||
|
@@ -1609,7 +1626,8 @@ internal protected virtual CSharpSyntaxNode GetBindableSyntaxNode(CSharpSyntaxNo | |
!(node is OrderingSyntax) && | ||
!(node is JoinIntoClauseSyntax) && | ||
!(node is QueryContinuationSyntax) && | ||
!(node is ArrowExpressionClauseSyntax)) | ||
!(node is ArrowExpressionClauseSyntax) && | ||
!(node is SubPropertyPatternSyntax)) | ||
{ | ||
return GetBindableSyntaxNode(parent); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -423,6 +423,13 @@ internal override Optional<object> GetConstantValueWorker(CSharpSyntaxNode node, | |
return (model == null) ? default(TypeInfo) : model.GetTypeInfo(node, cancellationToken); | ||
} | ||
|
||
public override SymbolInfo GetSymbolInfo(SubPropertyPatternSyntax node, CancellationToken cancellationToken) | ||
{ | ||
CheckSyntaxNode(node); | ||
var model = this.GetMemberModel(node); | ||
return (model == null) ? default(SymbolInfo) : model.GetSymbolInfo(node, cancellationToken); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SymbolInfo.None instead of default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be different from all the other places this pattern appears in the code. I'd rather be consistent and review a possible change all at once. |
||
} | ||
|
||
public override IPropertySymbol GetDeclaredSymbol(AnonymousObjectMemberDeclaratorSyntax declaratorSyntax, CancellationToken cancellationToken = default(CancellationToken)) | ||
{ | ||
CheckSyntaxNode(declaratorSyntax); | ||
|
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.
Why not use UnknownResultType singleton for the error type 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.
The binder never does so. This is the pattern currently used throughout the binder. We can consider changing that design, but I'd rather be consistent until we make a decision to do things differently.
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 see no point in allocating a new object every time when it doesn't carry any additional information (I am referring to the empty argument list).
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 agree that it would be valuable to review this design issue in the context of the binder as a whole. I do not want to start that review by making a change in this PR that is inconsistent with the existing design.
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.
Fair enough