Share pattern-matching temps when they are the same type. #19378

gafter · 2017-05-09T23:31:01Z

Customer scenario

Use edit-and-continue in a method that contains a pattern-switch statement. It doesn't work today, but we'd like to enable it. This work is a first step in making that happen.

Bugs this fixes:

Fixes #19280

Workarounds, if any

None.

Risk

Quite low. We simply reuse temporary variables when they are used in non-overlapping ranges.

Performance impact

No compiler perf change expected. Slight improvement to code quality.

Is this a regression from a previous update?

N/A

Root cause analysis:

N/A

How was the bug found?

N/A

@dotnet/roslyn-compiler @tmat Please review this compiler fix. It is a prerequisite to E&C work we hope to get into 15.3.

Fixes dotnet#19280

tmat · 2017-05-09T23:52:09Z

src/Compilers/CSharp/Portable/BoundTree/DecisionTreeBuilder.cs

+            LocalSymbol temp;
+            if (!localByType.TryGetValue(type, out temp))
+            {
+                temp = new SynthesizedLocal(_enclosingSymbol as MethodSymbol, type, SynthesizedLocalKind.PatternMatching, Syntax);


_enclosingSymbol as MethodSymbol [](start = 44, length = 32)

Can this be null? If not use explicit cast.

Yes, the parameter is "optional" in the constructor.

While it would be possible to change this to a cast now, that would be an error when we introduce a match expression, because in that case the enclosing symbol could be a field (for a field initializer). I'd rather leave this as is so it is future-proof.

tmat · 2017-05-09T23:53:11Z

…-19280

AlekseyTs · 2017-05-10T20:07:58Z

src/Compilers/CSharp/Portable/Binder/PatternSwitchBinder.cs

@@ -101,6 +101,7 @@ internal override void BindPatternSwitchLabelForInference(CasePatternSwitchLabel
        /// the decision tree.
        /// </summary>
        private ImmutableArray<BoundPatternSwitchSection> BindPatternSwitchSections(
+            SyntaxNode syntax,


SyntaxNode syntax [](start = 12, length = 17)

It looks like there is only one call-site. Consider making this a SwitchStatementSyntax and removing the two next parameters (boundSwitchExpression is coming from a property, sections are available from SwitchStatementSyntax). #Closed

Yes; even this syntax parameter is unneeded (it is available in the property SwitchSyntax).

AlekseyTs · 2017-05-10T20:10:32Z

src/Compilers/CSharp/Portable/BoundTree/DecisionTreeBuilder.cs

@@ -26,17 +26,63 @@ internal abstract class DecisionTreeBuilder
        protected readonly Symbol _enclosingSymbol;
        protected readonly Conversions _conversions;
        protected HashSet<DiagnosticInfo> _useSiteDiagnostics = new HashSet<DiagnosticInfo>();
+        private Dictionary<TypeSymbol, LocalSymbol> localByType = new Dictionary<TypeSymbol, LocalSymbol>();


new Dictionary<TypeSymbol, LocalSymbol>(); [](start = 66, length = 42)

Should this dictionary be sensitive to dynamic/object, tuple names and custom modifier differences?

No, it definitely should not.

No, it definitely should not.

Please add a test targeting this scenario.

Added TestIgnoreDynamicVsObjectAndTupleElementNames_03.

My "No" above should be "Yes".

AlekseyTs · 2017-05-10T20:18:30Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_PatternSwitchStatement.cs

@@ -59,15 +59,9 @@ private BoundStatement MakeLoweredForm(BoundPatternSwitchStatement node)
                var expression = _localRewriter.VisitExpression(node.Expression);
                var result = ArrayBuilder<BoundStatement>.GetInstance();

-                // if the expression is "too complex", we copy it to a temp.


// if the expression is "too complex", we copy it to a temp. [](start = 16, length = 60)

Could you please explain why this is no longer needed?

Lowering starts by lowering to a decision tree, which starts by calling CreateEmptyDecisionTree, which contains code equivalent to this deleted code. The existing generated code always contained two temps instead of one.

/// <summary> /// Create a fresh decision tree for the given input expression of the given type. /// </summary> protected DecisionTree CreateEmptyDecisionTree(BoundExpression expression) { var type = expression.Type; LocalSymbol temp = null; if (expression.ConstantValue == null) { // Unless it is a constant, the decision tree acts on a copy of the input expression. // We create a temp to represent that copy. Lowering will assign into this temp. temp = PatternMatchingTemp(type); expression = new BoundLocal(expression.Syntax, temp, null, type); }

Thanks for the explanation.

AlekseyTs · 2017-05-10T20:20:57Z

Done with review pass.

Fix some additional affected tests.

…ic are not merged.

gafter · 2017-05-10T20:48:40Z

@AlekseyTs The PR has been modified to respond to your comments.

AlekseyTs · 2017-05-10T21:06:49Z

src/Compilers/CSharp/Portable/Binder/PatternSwitchBinder.cs

@@ -115,11 +113,11 @@ internal override void BindPatternSwitchLabelForInference(CasePatternSwitchLabel

            // Bind match sections
            var boundPatternSwitchSectionsBuilder = ArrayBuilder<BoundPatternSwitchSection>.GetInstance();
-            SubsumptionDiagnosticBuilder subsumption = new SubsumptionDiagnosticBuilder(ContainingMemberOrLambda, this.Conversions, boundSwitchExpression);
-            foreach (var sectionSyntax in sections)
+            SubsumptionDiagnosticBuilder subsumption = new SubsumptionDiagnosticBuilder(base.ContainingMemberOrLambda, SwitchSyntax, this.Conversions, SwitchGoverningExpression);


base. [](start = 88, length = 5)

Does the base. make a difference?

AlekseyTs · 2017-05-10T21:11:00Z

src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs

+        }
+
+        [Fact, WorkItem(19280, "https://github.com/dotnet/roslyn/issues/19280")]
+        public void TestIgnoreDynamicVsObjectAndTupleElementNames_03()


TestIgnoreDynamicVsObjectAndTupleElementNames_03 [](start = 20, length = 48)

I am confused, the name of the test says "Ignore", but it looks like the differences aren't ignored.

I'll rename it TestSignificanceOfDynamicVersusObjectAndTupleNamesInUniquenessOfPatternMatchingTemps

AlekseyTs

LGTM

gafter · 2017-05-10T22:17:56Z

@dotnet/roslyn-compiler May I please have a second review from the compiler team?

VSadov

LGTM

gafter · 2017-05-11T04:47:28Z

/CC @MeiChin-Tsai for ask mode approval, please

MeiChin-Tsai · 2017-05-11T04:58:35Z

Approved. thx.

Share pattern-matching temps when they are the same type.

4eb82b1

Fixes dotnet#19280

gafter added 4 - In Review A fix for the issue is submitted for review. Area-Compilers Interactive-EnC Feature - Pattern Matching Pattern Matching labels May 9, 2017

gafter added this to the 15.3 milestone May 9, 2017

gafter self-assigned this May 9, 2017

gafter requested review from tmat, agocke and AlekseyTs May 9, 2017 23:31

dnfclas added the cla-already-signed label May 9, 2017

tmat reviewed May 9, 2017

View reviewed changes

gafter added 2 commits May 10, 2017 12:47

Update test to account for changed code in pattern-matching

11f360d

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

e088442

…-19280

AlekseyTs reviewed May 10, 2017

View reviewed changes

gafter added 2 commits May 10, 2017 13:29

Eliminate some unneeded parameters.

300a47e

Fix some additional affected tests.

Add a test demonstrating that temps differing by tuple names or dynam…

c63b016

…ic are not merged.

AlekseyTs reviewed May 10, 2017

View reviewed changes

AlekseyTs approved these changes May 10, 2017

View reviewed changes

Rename a test.

716d6df

VSadov approved these changes May 10, 2017

View reviewed changes

gafter merged commit 841044b into dotnet:master May 11, 2017

gafter added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share pattern-matching temps when they are the same type. #19378

Share pattern-matching temps when they are the same type. #19378

gafter commented May 9, 2017

tmat May 9, 2017

gafter May 10, 2017

tmat commented May 9, 2017

AlekseyTs May 10, 2017 •

edited

Loading

gafter May 10, 2017 •

edited

Loading

AlekseyTs May 10, 2017

gafter May 10, 2017

AlekseyTs May 10, 2017

gafter May 10, 2017

gafter May 10, 2017

AlekseyTs May 10, 2017

gafter May 10, 2017

AlekseyTs May 10, 2017

AlekseyTs commented May 10, 2017

gafter commented May 10, 2017

AlekseyTs May 10, 2017

gafter May 10, 2017

AlekseyTs May 10, 2017

gafter May 10, 2017

AlekseyTs left a comment

gafter commented May 10, 2017

VSadov left a comment

gafter commented May 11, 2017

MeiChin-Tsai commented May 11, 2017

Share pattern-matching temps when they are the same type. #19378

Share pattern-matching temps when they are the same type. #19378

Conversation

gafter commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented May 9, 2017

AlekseyTs May 10, 2017 • edited Loading

Choose a reason for hiding this comment

gafter May 10, 2017 • edited Loading

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

AlekseyTs commented May 10, 2017

gafter commented May 10, 2017

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

AlekseyTs left a comment

Choose a reason for hiding this comment

gafter commented May 10, 2017

VSadov left a comment

Choose a reason for hiding this comment

gafter commented May 11, 2017

MeiChin-Tsai commented May 11, 2017

AlekseyTs May 10, 2017 •

edited

Loading

gafter May 10, 2017 •

edited

Loading