Skip to content

Introduce 'use pattern combinators' feature#43596

Merged
CyrusNajmabadi merged 48 commits intodotnet:release/dev16.8-preview1from
alrz:use-patterns
Jun 17, 2020
Merged

Introduce 'use pattern combinators' feature#43596
CyrusNajmabadi merged 48 commits intodotnet:release/dev16.8-preview1from
alrz:use-patterns

Conversation

@alrz
Copy link
Copy Markdown
Member

@alrz alrz commented Apr 23, 2020 •
edited
Loading

Contributes to #42368

@alrz alrz requested a review from a team as a code owner April 23, 2020 13:42
@alrz
Copy link
Copy Markdown
Member Author

alrz commented Apr 23, 2020 •
edited
Loading

Sample results: 9699f28

/cc @CyrusNajmabadi

@alrz alrz mentioned this pull request Apr 23, 2020
Merged
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 23, 2020
View reviewed changes
...Features/CSharpTest/UsePatternCombinators/CSharpUsePatternMatchingDiagnosticAnalyzerTests.cs Outdated
{
return variable is 0 or
1 or
2;
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Apr 23, 2020

Choose a reason for hiding this comment

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

open question about the right thing to do here. it's arguable that if we get down to a primary expression/token that we might want to remove the newlines.

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 23, 2020
View reviewed changes
src/Features/CSharp/Portable/UsePatternCombinators/AnalyzedPattern.cs Outdated

private Not(AnalyzedPattern pattern) => Pattern = pattern;

private static BinaryOperatorKind Negate(BinaryOperatorKind kind) => kind switch
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Apr 23, 2020

Choose a reason for hiding this comment

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

so we already have a negate helper for patterns. i dont' love that we now have two.

Copy link
Copy Markdown
Member Author

@alrz alrz Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There's lots of code that could be shared with if-to-switch. I just didn't go thought that yet. I'd prefer this to be done separately.

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 23, 2020
View reviewed changes
...Features/CSharp/Portable/UsePatternCombinators/CSharpUsePatternCombinatorsCodeFixProvider.cs Outdated
BinaryOperatorKind.LessThan => SyntaxKind.LessThanToken,
BinaryOperatorKind.GreaterThan => SyntaxKind.GreaterThanToken,
BinaryOperatorKind.LessThanOrEqual => SyntaxKind.LessThanEqualsToken,
BinaryOperatorKind.GreaterThanOrEqual => SyntaxKind.GreaterThanEqualsToken,
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Apr 23, 2020

Choose a reason for hiding this comment

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

i feel like we have this in a few places. could we move this helper to another directory and share any of this common stuff in a static class?

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 24, 2020
@alrz alrz mentioned this pull request Apr 24, 2020
Merged
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 4, 2020

Very close to signing off. Just a few pieces of final feedback.

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 4, 2020
View reviewed changes
src/Analyzers/CSharp/Analyzers/UsePatternCombinators/AnalyzedPattern.cs
return (leftPattern, rightPattern) is (Not left, Not right)
? Not.Create(new Binary(left.Pattern, right.Pattern, !isDisjunctive, token, target))
: new Binary(leftPattern, rightPattern, isDisjunctive, token, target);
return new Binary(leftPattern, rightPattern, isDisjunctive, token, target);
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jun 4, 2020

Choose a reason for hiding this comment

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

i think it's a worth a comment that we are explicitly not trying to simplify here as user may not find the change substantively better for their domain.

Copy link
Copy Markdown
Member Author

@alrz alrz Jun 6, 2020

Choose a reason for hiding this comment

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

when we do want to do that, it'll be in another pass, not the factory method.

alrz added 3 commits June 5, 2020 13:58
@alrz
Merge remote-tracking branch 'origin/master' into use-patterns
93e3544 
# Conflicts:
#	src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs
#	src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs
@alrz
Fix build
019c735
@alrz
PR feedback
6adbbfd
@alrz alrz requested a review from CyrusNajmabadi June 5, 2020 10:07
@alrz
Copy link
Copy Markdown
Member Author

alrz commented Jun 12, 2020

@CyrusNajmabadi #44513 and #45094 are going in. If we're gonna target either branch we can relax is not to permit variables. Which branch do we want to target here?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 12, 2020

Let's just target master, and just allow for tihs (knowing that Neal's work will eventually flow in).

alrz added 2 commits June 12, 2020 12:40
@alrz
Merge remote-tracking branch 'origin/master' into use-patterns
9e67ecb 
# Conflicts:
#	src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs
#	src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs
@alrz
Permit variables under top-level not patterns
e902967
@alrz
Copy link
Copy Markdown
Member Author

alrz commented Jun 12, 2020

@CyrusNajmabadi Done.

quick question: when shared analyzer layer is going to reference latest bits? is it after final release or sometime sooner?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 12, 2020

when shared analyzer layer is going to reference latest bits? is it after final release or sometime sooner?

Not certain. @mavasani do you know when we do this?

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 15, 2020

@jinujoseph Do you know when master is open for new feature work?

@jinujoseph
Copy link
Copy Markdown
Contributor

jinujoseph commented Jun 15, 2020

@CyrusNajmabadi , master will be open for new feature post 16.7.Preview6
you can retarget this to 16.8.Preview1 branch.

@CyrusNajmabadi CyrusNajmabadi changed the base branch from master to release/dev16.8-preview1 June 15, 2020 01:03
CyrusNajmabadi
CyrusNajmabadi approved these changes Jun 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

code looks great! one question about what default state this is in. thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit 53a0876 into dotnet:release/dev16.8-preview1 Jun 17, 2020
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jun 17, 2020

Merged. Thanks!

@alrz
Copy link
Copy Markdown
Member Author

alrz commented Jun 17, 2020

Thanks!

I discovered two issues while testing this out in our codebase

  • default is not valid as a pattern anywhere, should rewrite to default(T)
  • should not suggest in expression trees

If I find some time for it I'll open a follow up.

@alrz alrz mentioned this pull request Jun 18, 2020
Merged
@mavasani mavasani mentioned this pull request Jun 30, 2020
Closed
@Youssef1313 Youssef1313 mentioned this pull request Mar 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@gafter gafter gafter left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@CyrusNajmabadi CyrusNajmabadi

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request

Projects

IDE: Design review
Status: Complete

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alrz @CyrusNajmabadi @sharwell @jinujoseph @gafter