Refactoring feature to suggest to use recursive patterns #32386

alrz · 2019-01-11T05:32:30Z

No description provided.

alrz · 2019-01-11T11:57:04Z

CyrusNajmabadi · 2019-01-11T15:10:54Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+        return b is
+        {
+            P1: 1, P2: 2
+        };


we should probably try to preserve 'single line'-ness the 'after' doesn't seem better here due to it now adding 3 lines.

just FYI, but i do think this is critical.

CyrusNajmabadi · 2019-01-11T15:11:49Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+            {
+                Name: string.Empty, ContainingNamespace: null
+            }
+        };


the formatting here seems wonky. where it doesn't seem to match up with original like i woudl expect.

here's my thinking. if the original was single-line, we should attempt to preserve single-line. If the original is multi-line, we can be multiline too. When multi-line, we should keep properties on a single line, if their patterns are single line. But if they are multi-line, then they should go across multiple lines as well. So this should be:

return b is { MetadataName: string.Empty, ContainingType: null, ContainingNamespace: { Name: string.Empty, ContainingNamespace: null } };

CyrusNajmabadi · 2019-01-11T15:12:22Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+                Name: ""System"", Kind: SymbolKind.Namespace
+            }
+
+            declContainer


what is this random name?

this is the var declContainer pattern in "before" which has become the pattern designation.

this reads super weirdly. especially with the multiple newlines, and no clear indication about which part of hte pattern this corresponds to.

how would you expect this to be formatted (a multi-line property pattern + designation)?

another possibility: type + multi-line property pattern + designation

yup. it points to the exact same object from the source.

yeah having both a newline and a blank line here is bad. it looks likke declContainer is completed separated from teh pattern. We really shoudl not do this. It should at least be:

return type is { Name: ""ValueTuple"", ContainingSymbol: { Name: ""System"", Kind: SymbolKind.Namespace } declContainer }

However, since declContainre isn't used, we should remove it as well.

Could we modify formatting engine to NOT insert new lines in certain places like,

an empty property pattern clause {} does not need to break in multiple lines

we should not insert a new line before a variable designation ever

Or more rules? lookin at tests, only these two would result in a way better source text.

Also, I'm not sure if we should deal with indentation when breaking lists into multiple lines.

I think unless we want to explicitly perverse or move around trivia, we don't need to do anything on formatting in this feature. These all should go to formatting rules, right?

if you are not preserving trivia and you are using ElasticTrivia, then yes, you just need to update formatting rules to do something sensible there.

if you are preserving trivia, then it's likely you'll need to write some custom logic so that hte preserved trivia still looks good.

I'm fine with any approach you take that ends up with a good final result for the user :) '1' would be a good place to start from to see how far that gets us.

CyrusNajmabadi · 2019-01-11T15:13:08Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+    bool M()
+    {
+        return type.Name == ""ValueTuple"" &&
+            type.ContainingSymbol is var declContainer &&


semantics seem like they may have changed slightly (which is probably ok). here, the is var ... will always succeed, and you can null on the checks ref below. but in the converted form, it can't null ref right?

we preserve the variable, in case it's used anywhere else (see previous comment), but semantics haven't changed because the var pattern is &&'ed with others in the source.

"we preserve the variable, in case it's used anywhere else (see previous comment)," we should probably remove the variable if not used.

i'm not quite getting your explanation. previous code could have a null ref exception right? (i.e. on declContainer.Kind). But hte new code won't because it won't do the inner checks if ContainingSymbol is null. Isn't that how it works?

regarding null checks, yes it would be added by the compiler. we might also reorder expressions but property patterns are assumed to be idempotent by the compiler to allow reordering in the generated code. this is kinda a reversed use of that assumption. there will be semantic changes if it doesn't hold.

CyrusNajmabadi · 2019-01-11T15:14:08Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+                    IsGlobalNamespace: true
+                }
+
+                , Name: ""System"", Kind: SymbolKind.Namespace


this formatting seems off.

this is the default formatting from SyntaxFactory, is this in the stable form or it can change?
either way we probably need to do a formatting pass for the feature.

To be pednatic. SyntaxFactory does not formatting. It just adds elastic trivia to things. So this is how the formatting engine is handling elastic formatting here currently.

we probably need to make hte formatter better here.

even if the formatter is better, this feature will likely need specialized trivia handling to try to do a good job for this sort of conversion.

CyrusNajmabadi · 2019-01-11T15:14:18Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+                , Name: ""System"", Kind: SymbolKind.Namespace
+            }
+
+            declContainer


this is the pattern designation which comes from the var pattern. see comment above.

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs

CyrusNajmabadi · 2019-01-11T15:16:32Z

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs

+                public override bool VisitPatternMatch(PatternMatch node) => SyntaxFactory.AreEquivalent(_expression, node.Expression);
+                public override bool VisitSourcePattern(SourcePattern node) => false;
+                public override bool VisitTypePattern(TypePattern node) => false;
+                public override bool VisitVarPattern(VarPattern node) => false;


these all need explanations. for example, why _expression could not be found in a constant pattern

because the expression we're trying to find would not possibly appear in a constant pattern. will add a comment.

CyrusNajmabadi · 2019-01-11T15:16:54Z

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs

+                else
+                {
+                    return new Conjuction(this.Left, rightSub);
+                }


needs explanation (probably with examples).

CyrusNajmabadi · 2019-01-11T15:19:13Z

It's really cool you're doing this! I'm sure it will be taken as a refactoring.
The approach is too different for me to wrap my weak and puny brain around. Can you do a writeup (ideally with comments in code) walking through both the high level ideas of patterns/analyzed-nodes/reducers/etc. Then have some examples in appropriate places showing what's going on?

Thanks!

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs

CyrusNajmabadi · 2019-01-13T05:28:53Z

I must say I have never understood the bar for performance improvements, the fact that it doesn't run that much (but not too rarely either) is enough reason to not care?

Correct. Code should strive for simplicity, maintainability and clarity. When necessary, for performance reasons, we can take extra steps to trim things down. Otherwise, it's putting in unnecessary effort for gains that will not benefit people.

but it would eliminate a lot of allocations in the the local run.

Define 'lot of allocations'. The general idea is this: If your system is producing a veritable torrential downpour of allocations, then avoiding a handful of allocs isn't really buying anything.

Considering that this addition would add zero complexity

That doesn't seem to be the case. You're adding an entirely new type, and having to have this code use it, with the understanding of how this argument is processed. There is conceptual complexity here that seems like overkill given how little actual impact hte existing code would have.

(read this as just a general question, not an argument)

The main reason this is brought up is because this is essentially a form of premature optimization. And expending effort there is just not worthwhile when it won't actually improve things and when many other areas actually would benefit from a focus on performance. There are limited resources to make code cahnges (and that includes the resources to do PR reviews). So tying up those resources should be done in a manner that produces the most bang/buck at the end of the day.

alrz · 2019-01-21T18:57:29Z

Turns out we don't need a Visitor after all, because we already have access to the node declaration itself, we can do all phases (reducer and rewriter) using just methods on the base. I intend to remove the Visitor class and move everything to AnalyzedNode hierarchy. Please let me know if you think it's better to keep it.

CyrusNajmabadi · 2019-01-21T20:47:27Z

I'd have to see. But my gut tells me it would be nicer to not have it

alrz · 2019-01-21T20:50:18Z

The only concern is that every kind of transformations (namely, reduction and rewrite) are mixed in a single class. Perhaps it would help to separate out files for each node, but Visitor is def redundant here.

alrz · 2019-01-21T21:04:02Z

This is the rough sketch of the change: alrz@1b7df05

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs

CyrusNajmabadi · 2019-01-23T17:42:32Z

is that important enough to give a different option based on what we're doing?

Probably not. In general, we're ok with individual options handling a few cases, as long as they all seem to reasonably fit the same 'theme'.

alrz · 2019-11-15T06:41:21Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+                ContainingSymbol: NamespaceSymbol { IsGlobalNamespace: true },
+                Name: ""System"",
+                Kind: 0
+            } declContainer


Could we use a reducer to remove unused variables via Simplifier annotation?

alrz · 2019-11-15T06:42:02Z

src/EditorFeatures/CSharpTest/UseRecursivePatterns/UseRecursivePatternsTests.cs

+            MetadataImportOptions: otherMetadataImportOptions,
+            ReferencesSupersedeLowerVersions: otherReferencesSupersedeLowerVersions
+        } &&
+this.OutputKind.IsNetModule() == other.OutputKind.IsNetModule();


I would expect the formatter indent this..

alrz · 2019-11-16T07:05:08Z

@sharwell This is ready for review.

Note: there's some formatting changes to get a reasonable output from the refactoring.

CyrusNajmabadi · 2020-03-08T20:50:10Z

@alrz do you still want to move forward with this?

alrz · 2020-03-08T21:10:14Z

@CyrusNajmabadi I think this is only blocked on #40361 and a few formatting changes in this PR should be also unnecessary.

alrz · 2020-03-11T17:13:17Z

Closing this for now, maybe I'll come back to it later. this seems like a rather complicated feature for little benefit to end user, at least at its current state.

sharwell · 2020-03-16T17:21:08Z

@alrz If we happen to get some free time, do you mind if @CyrusNajmabadi and/or myself reopen this pull request and finish the work by adding commits to the top of your branch?

alrz · 2020-03-16T17:42:40Z

Of course, if you're happy with the feature feel free to reopen. let me know if there's anything I can do. Thanks.

alrz · 2020-04-20T03:38:33Z

I plan to split this into a few more focused refactorings/analyzers. Keeping this open for reference.

alrz · 2020-05-02T02:48:39Z

I think we should change this to operate on IOperation nodes instead of sytnax, for the following reasons:

We need a simple and fast equality function. We use AreEqivalent too many times in the current impl, with IOperation it could be trivially done via a reference comparison.

Plus, some edge cases demand for a more reliable equality check, In the current impl we have to separately handle different symbols with identical names.

We are snipping out lots of code and keeping the syntax around wouldn't help much. And it could be done via a separate syntax pass if needed, like we did with a few other refactorings.

And finally, there's a chance we can share the logic with those features as well, all of which use IOperation nodes.

alrz · 2020-05-02T02:51:23Z

Since we aggressively make patterns out of any expression, I think we need a way for user to choose subparts of an expression to be replaced. I want to consider the selection span for that purpose, if any. Otherwise we apply it to the whole expression.

alrz requested a review from a team as a code owner January 11, 2019 05:32

CyrusNajmabadi reviewed Jan 11, 2019

View reviewed changes

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 11, 2019

View reviewed changes

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 11, 2019

View reviewed changes

alrz commented Jan 11, 2019

View reviewed changes

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs Outdated Show resolved Hide resolved

jinujoseph added Area-IDE PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 15, 2019

jinujoseph added this to InQueue in IDE: CommunityPR via automation Jan 15, 2019

alrz changed the title ~~Refactoring feature for using recursive patterns~~ Refactoring feature to suggest to use recursive patterns Jan 18, 2019

alrz commented Jan 23, 2019

View reviewed changes

...table/UseRecursivePatterns/CSharpUseRecursivePatternsCodeRefactoringProvider.AnalyzedNode.cs Outdated Show resolved Hide resolved

alrz requested review from a team as code owners January 25, 2019 15:29

alrz changed the base branch from features/recursive-patterns to master January 25, 2019 15:29

alrz added 7 commits November 14, 2019 15:23

PR feedback

acf03e0

Add comment.

c930841

Simplify code.

0bde97a

Add assertion.

59b15df

Fix comments.

dc28d60

Add recursive pattern reducer

d6a537d

Address feedback

f3af169

alrz force-pushed the use-recursive-patterns branch from 1fa5819 to f3af169 Compare November 15, 2019 06:39

alrz commented Nov 15, 2019

View reviewed changes

alrz mentioned this pull request Dec 13, 2019

Formatter doesn't handle indentation of wrapped expressions #40361

Open

alrz closed this Mar 11, 2020

IDE: CommunityPR automation moved this from BuddyAssigned to Completed Mar 11, 2020

sharwell reopened this Mar 16, 2020

alrz marked this pull request as draft April 21, 2020 08:13

sharwell mentioned this pull request Apr 27, 2020

Introduce 'use pattern combinators' feature #43596

Merged

alrz mentioned this pull request Jul 2, 2020

Introduce 'use pattern matching' refactoring feature #45629

Closed

2 tasks

Base automatically changed from master to main March 3, 2021 23:51

jinujoseph moved this from Completed-166 to Completed 2020 in IDE: CommunityPR Apr 2, 2021

alrz closed this May 20, 2021

IDE: CommunityPR automation moved this from Completed 2020 to May-2021 May 20, 2021

jinujoseph moved this from May-2021 to Completed 2021 in IDE: CommunityPR Jan 9, 2022

Refactoring feature to suggest to use recursive patterns #32386

Refactoring feature to suggest to use recursive patterns #32386

Conversation

alrz commented Jan 11, 2019 • edited

alrz commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jan 29, 2019 • edited

Choose a reason for hiding this comment

alrz Jan 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Jan 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 11, 2019

CyrusNajmabadi commented Jan 13, 2019

alrz commented Jan 21, 2019

CyrusNajmabadi commented Jan 21, 2019

alrz commented Jan 21, 2019

alrz commented Jan 21, 2019

CyrusNajmabadi commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz commented Nov 16, 2019

CyrusNajmabadi commented Mar 8, 2020

alrz commented Mar 8, 2020 • edited

alrz commented Mar 11, 2020

sharwell commented Mar 16, 2020

alrz commented Mar 16, 2020

alrz commented Apr 20, 2020 • edited

alrz commented May 2, 2020 • edited

alrz commented May 2, 2020

alrz commented Jan 11, 2019 •

edited

alrz Jan 29, 2019 •

edited

alrz Jan 29, 2019 •

edited

alrz Jan 11, 2019 •

edited

alrz commented Mar 8, 2020 •

edited

alrz commented Apr 20, 2020 •

edited

alrz commented May 2, 2020 •

edited