Revise the shape of the syntax trees for property patterns #9505
Conversation
gafter
added
Area-IDE
Concept-API
|
IDE and syntax changes LGTM. |
| @@ -123,7 +123,8 @@ private BoundPattern BindRecursivePattern(RecursivePatternSyntax node, BoundExpr | |||
| 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)); | |||
| var member = new BoundPropertyPatternMember(node.PatternList.SubPatterns[i], properties[i], ImmutableArray<BoundExpression>.Empty, ImmutableArray<string>.Empty, ImmutableArray<RefKind>.Empty, false, default(ImmutableArray<int>), LookupResultKind.Empty, properties[i].GetTypeOrReturnType(), hasErrors); | |||
There was a problem hiding this comment.
This line is way too long, consider adding line breaks.
|
I'd actually go the opposite route. I'd allow parsing out any sort expressions within these pattersn. Then i'd give a higher level error saying which legal expressions you can use (currently only "x is Blah" expressions). |
|
But i don't feel strongly about it. |
|
@CyrusNajmabadi I agree, that is reasonable, but I wasn't ready to write the recovery code today. I was ready to put the syntax trees in place to allow that later. |
|
@gafter Makes sense to me. I like that the syntax is relaxed but we can then adjust the implementation as we want in the future. |
| { | ||
| // TODO: consider refactoring out common code with BindObjectInitializerMember | ||
|
|
||
| BoundImplicitReceiver implicitReceiver = new BoundImplicitReceiver(memberName.Parent, patternType); |
There was a problem hiding this comment.
Consider marking this node as compiler generated?
There was a problem hiding this comment.
There is no need, as it is not put into the bound tree that results from the binder. It is only used to facilitate lookup.
|
Test prtest/win/dbg/unit32 please. |
|
Test prtest/win/dbg/eta please. |
|
|
||
| case BoundKind.EventAccess: | ||
| // TODO: Should a property pattern be capable of referencing an event? | ||
| // https://github.com/dotnet/roslyn/issues/9515 |
There was a problem hiding this comment.
It might make sense to allow referencing a field-like event from within a type that declares it (this is not a suggestion to handle this case in this change).
There was a problem hiding this comment.
That is already on the open issue list for the LDM.
There was a problem hiding this comment.
That is great, consider this comment as an opinion.
|
@gafter I am done with a review pass. |
|
@AlekseyTs This has been updated per review. |
| var subProperty = pat.Subpatterns[i].Property; | ||
| // TODO: review and test this code path. | ||
| // https://github.com/dotnet/roslyn/issues/9542 | ||
| var subProperty = (pat.Subpatterns[i].Member as BoundPropertyPatternMember)?.MemberSymbol; |
There was a problem hiding this comment.
The #9542 doesn't capture the issue brought up against this line of code during code review. It should specifically talk about replacement of an as cast with a "normal" type cast because this conversion is not expected to fail. Please make sure the issue captures this information.
There was a problem hiding this comment.
Another part was removal of conditional access off of subProperty, it is not expected to have null value.
There was a problem hiding this comment.
I will expand the comment just above.
There was a problem hiding this comment.
That would be a viable alternative.
|
All comments responded to. Please re-review. IDE work for Build is still blocked on this API change. |
|
LGTM. However, we should make sure that outlined error scenarios (events, methods, nested types) are covered by unit-tests before this change makes it into the 'future'. |
|
@dotnet/roslyn-compiler Can I please have another set of eyes on this from the compiler team? |
| diagnostics: diagnostics); | ||
|
|
||
| LookupResultKind resultKind = boundMember.ResultKind; | ||
| bool hasErrors = boundMember.HasAnyErrors || implicitReceiver.HasAnyErrors; |
There was a problem hiding this comment.
Consider moving bool hasErrors = ... below if block so that hasErrors can be calculated once.
|
LGTM |
Revise the shape of the syntax trees for property patterns
Fixes #9371
Related to #9284
@CyrusNajmabadi This should unblock the IDE work. Please review.
@AlekseyTs @jaredpar Please review
@dotnet/roslyn-compiler FYI