-
Notifications
You must be signed in to change notification settings - Fork 4k
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 extended property patterns #52139
Merged
jcouv
merged 42 commits into
dotnet:features/extended-property-patterns
from
alrz:extended-property-patterns
May 5, 2021
Merged
Changes from 34 commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
75bf6e7
WIP
alrz 9f51bbb
WIP
alrz 1e67ee2
WIP
alrz b4019e1
WIP
alrz d61c3c8
WIP
alrz 5593114
WIP
alrz b384f78
WIP
alrz 808ce1f
WIP
alrz 675a57d
WIP
alrz a4cebf9
WIP
alrz bc7efe6
WIP
alrz c620d01
Update resources
alrz daea3f1
WIP
alrz 411a6b5
Merge remote-tracking branch 'origin/features/extended-property-patte…
alrz 4bc2dac
Update resources
alrz be4e20a
Address feedback on syntax tree
alrz 49e151e
Revert
alrz 6e5b186
Merge remote-tracking branch 'origin/features/extended-property-patte…
alrz 4d0a51d
Address feedback
alrz 371c4f4
Typo
alrz 8e82fe7
Revert code
alrz 13eb678
Remove ERR_ConditionalAccessInSubpattern
alrz 359681f
Add PROTOTYPE comments
alrz 8996089
Add suggested test
alrz 7c487de
Add backcompat APIs
alrz 81037c3
Typo
alrz eaa5c5d
Attempt to disable RS0016
alrz ffcb24e
Renamings
alrz 6d5f488
Fix typo
alrz ae153fd
Add missing PROTOTYPE comment
alrz 732486c
Create ExpressionColon when a NameColon.WithName is called with an ex…
alrz 7d7bd53
Pass IOp verification
alrz 11f1ea8
Add missing member
alrz a6ba2fe
Attempt to fix build
alrz 35ea5b3
Typo
alrz b6b7bd3
Address feedback on syntax
alrz 6f518d7
Add more parsing tests
alrz d0e80c4
Add suggested tests
alrz 3a83ba0
Fix build
alrz 4897191
Add test
alrz d3f1e7f
Move langversion check to the parser
alrz 05f9a88
Add suggested tests
alrz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
32 changes: 32 additions & 0 deletions
32
src/Compilers/CSharp/Portable/BoundTree/BoundSubpattern.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
#nullable enable | ||
|
||
using System.Collections.Immutable; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp | ||
{ | ||
// PROTOTYPE(extended-property-patterns) Split BoundPropertySubpattern and remove this (requires IOperation changes) | ||
partial class BoundSubpattern | ||
{ | ||
internal BoundSubpattern(SyntaxNode syntax, Symbol? symbol, BoundPattern pattern, bool hasErrors = false) | ||
: this(syntax, symbol is null ? ImmutableArray<Symbol>.Empty : ImmutableArray.Create(symbol), pattern, hasErrors) | ||
{ | ||
} | ||
|
||
internal Symbol? Symbol | ||
{ | ||
get | ||
{ | ||
if (this.Symbols.IsEmpty) | ||
return null; | ||
if (this.Symbols.Length == 1) | ||
return this.Symbols[0]; | ||
return null; | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 changes: 11 additions & 10 deletions
21
src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
11 changes: 10 additions & 1 deletion
11
src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 check for
!hasErrors
seems unnecessary to me.hasErrors
may be already set by the caller or from an earlier subpattern. And if we don't report a/langversion
error now because of another error, won't the user just get the/langversion
error after fixing the current error?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.
If we don't care if there's already error on the name we might as well move it to the parser. But in that case it will err in completely invalid scenarios
{ (c?a:b): p } // invalid name + feature is in preview (?!)
Is that ok?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.
If we want to avoid reporting this error if the are syntax errors, could we check
!expr.HasErrors
here instead?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.
Reading this again, that's actually a pretty bad experience. Another scenario is doing
a?.b
which produces "invalid name" and only after fixing that we give langversion error (even with expr.HasError) which is unnecessary back and forth.I'm leaning towards moving this check to the parser and produce all diagnostics upfront -
(c?a:b)
example is a hypothetical scenario that I think we shouldn't be worry about.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.
Moving this check to the parser seems reasonable, but it might be a fair amount of code churn (with "/langversion" errors reported in parsing tests, etc.). Also, I suspect there are other codepaths in binding we don't report all binding errors if the expression contains syntax errors. So, to me, it seems sufficient to either remove the
!hasErrors
check above or change to!expr.HasErrors
.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.
Not for subpatterns apparently - probably because we only had an identifier before and it couldn't go wrong. I did a little experiment to see how it looks like (with all green pattern tests; semantic and syntax): d3f1e7f?w=1
We already use
preview
version so there wasn't a need to adjust the baseline.Anywho, if you feel that's gonna be an issue I'll be fine to keep it in the binder.
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.
My hesitation was just avoiding code churn if there was minimal benefit to moving the check.
It looks like we have a number of pattern feature checks in the parser already so perhaps it makes sense to check this feature in the parser as well since there's no test impact currently.
If you do move the check, please include at least one parsing test with
TestOptions.Regular9
andTestOptions.RegularPreview
, and consider including a binding test that demonstrates that errors in an earlier subpattern have no effect on the/langversion
error: perhaps_ = new C() is { Unrecognized: null, Prop1.Prop2: null };