Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement extended property patterns #52139

Merged
merged 42 commits into from
May 5, 2021
Merged

Implement extended property patterns #52139

merged 42 commits into from
May 5, 2021

Conversation

alrz
Copy link
Contributor

@alrz alrz commented Mar 25, 2021 •
edited
Loading

@alrz alrz mentioned this pull request Mar 25, 2021
Merged
@alrz
Copy link
Contributor Author

alrz commented Mar 25, 2021 •
edited by jcouv
Loading

@333fred Can you please rebase to a feature branch? I'll prep this for review soon.

I think the bulk of feature will be simple enough for review in this PR. I'll cover the test plan, IOp, etc as follow up PRs afterwards.

In reply to: 807729473

In reply to: 807729473

@alrz alrz mentioned this pull request Mar 26, 2021
Open
4 tasks
@333fred 333fred changed the base branch from main to features/extended-property-patterns March 26, 2021 21:53
@333fred
Copy link
Member

333fred commented Mar 26, 2021 •
edited by jcouv
Loading

@333fred Can you please rebase to a feature branch? I'll prep this for review soon.

I think the bulk of feature will be simple enough for review in this PR. I'll cover the test plan, IOp, etc as follow up PRs afterwards.

Done. Feel free to create the test plan issue whenever you're ready.

In reply to: 808550478

@alrz alrz mentioned this pull request Apr 7, 2021
Closed
34 tasks
alrz added 2 commits April 8, 2021 00:24
@alrz
Merge remote-tracking branch 'origin/features/extended-property-patte…
411a6b5 
…rns' into extended-property-patterns

# Conflicts:
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf
#	src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf
@alrz
Update resources
4bc2dac
@alrz alrz changed the title Draft implementation for extended property patterns Implement extended property patterns Apr 7, 2021
@alrz
Copy link
Contributor Author

alrz commented Apr 7, 2021

@333fred I should probably sort out public APIs just like the other PR, also check lang version. Up until the next round of changes I'd like a review pass if possible. Thanks.

@alrz alrz marked this pull request as ready for review April 7, 2021 20:10
@alrz alrz requested review from a team as code owners April 7, 2021 20:10
@jcouv
Copy link
Member

jcouv commented Apr 7, 2021 •
edited
Loading

@alrz Just to let you know, we have a lot of features in flight as the moment that we'd prefer to land first. We love the contribution/enthusiasm and this feature, but this means we won't be able to dedicate much time reviewing this for a while.
Tagging @jaredpar in case anything to add.

In reply to: 815249337

@alrz
Copy link
Contributor Author

alrz commented Apr 7, 2021

That's why I wrote "if possible" 😄 I don't expect this feature to be a priority obviously.

@jaredpar
Copy link
Member

jaredpar commented Apr 8, 2021

We super appreciate your enthusiasm here and want to get as much of your features into C# 10 as possible. But we are starting to run up against resource constraints (just can't review it all at the same time 😄). I think there are essentially three features in flight now:

  1. List patterns
  2. Enumerable patterns (extension of List)
  3. Extended property patterns

Had a brief chat earlier this week with @jcouv and we were pretty interested and trying to get (1) in first. That is a self contained feature, adds a ton of value, and it seems to be on the right path impl + design. After that I'm somewhat ambivilent as to what we push on next but we should discuss.

@alrz alrz requested a review from jcouv April 29, 2021 10:37
@alrz alrz mentioned this pull request May 1, 2021
Merged
jcouv
jcouv approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 34) with a minor comment (missing PROTOTYPE marker?)

@jcouv
Copy link
Member

jcouv commented May 3, 2021

@333fred @dotnet/roslyn-compiler for a second review. Thanks

cston
cston reviewed May 3, 2021
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs Outdated
var memberBuilder = ArrayBuilder<Symbol>.GetInstance();
LookupMembersForPropertyPattern(inputType, expr, memberBuilder, diagnostics, ref hasErrors, out memberType);
members = memberBuilder.ToImmutableAndFree();
if (!hasErrors && members.Length > 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!hasErrors

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?

Copy link
Contributor Author

@alrz alrz May 4, 2021 •
edited
Loading

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?

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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.

Copy link
Member

@cston cston May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor Author

@alrz alrz May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I suspect there are other codepaths in binding we don't report all binding errors if the expression contains syntax errors.

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.

Copy link
Member

@cston cston May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a little experiment to see how it looks like (with all green pattern tests; semantic and syntax)

Anywho, if you feel that's gonna be an issue I'll be fine to keep it in the binder.

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 and TestOptions.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 };

cston
cston reviewed May 4, 2021
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests5.cs
static bool Test1(C o) => o is { Prop1.Prop2.Prop3: null };
static bool Test2(S o) => o is { Prop1.Prop2.Prop3: null };
static bool Test3(S? o) => o is { Prop1.Prop2.Prop3: null };
static bool Test4(S0 o) => o is { Prop1.Prop2.Prop3: 420 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test4

Consider adding VerifyIL("C.Test4"). I think it would be interesting to see the difference between that generated code and Test2.

cston
cston approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A couple of remaining comments that are not blocking.

@jcouv jcouv merged commit f24acfe into dotnet:features/extended-property-patterns May 5, 2021
@jcouv
Copy link
Member

jcouv commented May 5, 2021

Merged/squashed. Thanks @alrz!
Could you rebase and select commits from your follow-up PR (#53082) then ping me again for review?
I tried reaching you by email. I hope I had the right email address. Would like to discuss whether you could use assistance on either patterns features. Let me know how we could sync up (my address is jcouv@microsoft).

@alrz
Copy link
Contributor Author

alrz commented May 5, 2021 •
edited
Loading

@jcouv Thanks. I did not receive your email (it's @outlook) though I'd prefer discord if possible.
I think for now the early steps are clear, I will reach you if anything comes up towards the end.
Just in case you you didn't see my comment, list patterns is blocked on LDM: #51299 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@cston cston cston approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
Compiler: Julien's umbrellas
Archived in project
Milestone
C# 10
Development

Successfully merging this pull request may close these issues.
6 participants
@alrz @333fred @jcouv @jaredpar @CyrusNajmabadi @cston