Don't read the same property twice in pattern matching by YairHalberstadt · Pull Request #35669 · dotnet/roslyn

YairHalberstadt · 2019-05-12T12:26:23Z

when accessed through the same underlying object, but references of different types.

Namely:

using System;

public class Person {
    public string Name { get; set; }
}

public class Student : Person { }


public class C {
    public void M(Person p) {
        switch (p) {
            case { Name: "Bill" }:
                Console.WriteLine("Hey Bill!");
                break;
            case Student { Name: var name }:
                Console.WriteLine($"Hello student {name}!");
                break;
            case { Name: var name }:
                Console.WriteLine($"Hello non-student {name}!");
                break;
        }
    }
}

will now only evaluate Person.Name once.

There are a number of limitations to this - the tests describe the details in comments.

…through the same underlying object, but references of different types.

YairHalberstadt · 2019-05-12T12:39:55Z

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

@@ -9,7 +9,7 @@ partial class BoundDagEvaluation
        public override bool Equals(object obj) => obj is BoundDagEvaluation other && this.Equals(other);
        public virtual bool Equals(BoundDagEvaluation other)
        {


Because we're overriding object.Equals, it's impossible to know statically everywhere this is called.
Instead I've set a breakpoint here, and on GetHashCode, and I've seen where it's been called from in the tests I added.

Namely:

DecisionDagBuilder.SimplifyTestsAndBindings(ArrayBuilder<BoundDagTest> testsBuilder, ArrayBuilder<BoundPatternBinding> bindingsBuilder) usedValues DecisionDagBuilder.RemoveEvaluation(RemainingTestsForCase c, BoundDagEvaluation e) RemainingTests: c.RemainingTests.WhereAsArray(d => !(d is BoundDagEvaluation e2) || e2 != e), NullableWalker.LearnFromDecisionDag() tempMap LocalRewriter.PatternLocalRewriter _map

I was not able to understand what was happening in NullableWalker.LearnFromDecisionDag, so I've simply added tests to verify that my changes do not affect nullability analysis. I imagine @gafter would be able to help here.

In all other cases my changes to Equals seemed correct.
#Resolved

Thanks to @jcouv for giving me an overview of how the nullable walker works. It seems to me that this behaviour is correct - since we assume property evaluations are non-mutating, the nullability state of any evaluations of the same property through different types should be the same, so it's ok to use the same slot for them. #Resolved

gafter · 2019-05-13T23:47:25Z

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

+        /// So two evaluations act on the same input so long as they have the same original input.
+        /// </summary>
+        /// <returns></returns>
+        private BoundDagTemp GetOriginalInput()


Please remove the empty <returns> section. #Resolved

gafter · 2019-05-13T23:52:39Z

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

+
+public class Student : Person {
+    public override string Name { get => Name; }
+}


This looks like infinite recursion. Did you mean base.Name? #Resolved

gafter

gafter · 2019-05-13T23:55:38Z

@agocke Could you look at this please?

@YairHalberstadt You may have to pull the latest from the master branch to fix the build error in the CI tests. #Resolved

…reduce-number-of-reads

YairHalberstadt · 2019-05-14T15:49:24Z

This might cause another issue in NullableWalker.LearnFromDecisionDag().
There we call NullableWalker.AsMemberOfType() on the property of a BoundDagPropertyEvaluation. We then pass that property into GetOrCreateSlot(). It is thus theoretically possible that if two BoundDagPropertyEvaluation have different input types, that they will be given different slots. However tempmap will consider them equal now.

It doesn't look like GetOrCreateSlot() depends on the changes NullableWalker.AsMemberOfType() makes to the property, however I do not know the purpose of NullableWalker.AsMemberOfType() (i.e. why it's necessary), so I couldn't say for sure. I think @cston worked on it. Would you be able to explain it to me? Thanks.

As an aside, I think that there is limited documentation of the Nullable Walker. @jcouv gave me a high level overview a couple of days ago. I would be happy to add that information to the Nullable Walker, and will also try and add any information @cston gives.

@jcouv's overview was:

NullableWalker visits bound nodes from initial binding (ie. with actual nullabilities only for explicit ones in source such as string? local = "";, but all inferred nullabilities left as oblivious such as var local = "";) to:

compute the nullability of expressions (and save those for semantic model),
keep track of knowledge for different variables (more on that below), and
produce warnings.
Slots and state are how it keeps track of knowledge for variables, or more generally storage locations. A slot is just a position/index into the state array.
For instance, parameters and locals in a method get slot, which holds either non-null or maybe-null state. Consider a parameter string? p1, we give it a slot/index and we'll initialize its state to maybe-null (ie. State[slot] = MaybeNull, because its declared type or TypeWithAnnotations is NotAnnotated), then when we visit p1 = ""; we can just override that state, and when we visit p1.ToString() we consult that state to decide whether to warn.

We also track fields within structs, so we also make slots for those. That way we can warn on localStruct.field1.ToString(), but not localStruct.field2.ToString() independently.
I don't remember how that's represented, but slots are related to each other, so that when assigning local2 = local1; we can not only copy the slot for local1to set the state of local2, but we can copy the nested slots.

It's worth noting that the state is generally just a simple array, but it can also be two arrays in some cases. That's called "conditional state". It's is used for analyzing expressions like x == null. We keep track of the state "if the expression were true" and "if the expression were false" separately. Slots are still used to index into those arrays as normal, but if I recall correctly it is good to ensure that slots get assigned before entering such a state.
Another operation that is common is that of cloning states. If you analyze if (b) ... else ..., then we clone the state so that we can analyze each branch separately. Then there are rules for merging states when the branches rejoin (Meet takes the worst case values).

There are rules for branches that are not reachable if (false) { ... unreachable ...}. In such unreachable code, every value you read is NotNull regardless of current state. #Resolved

cston · 2019-05-14T16:48:41Z

@YairHalberstadt, AsMemberOfType() is used to get the corresponding member for a symbol from initial binding to match an updated receiver type in NullableWalker.

For instance, AsMemberOfType() maps from List<string~>.Add(string~) to List<string?>.Add(string?):

string s = null;
var list = new[] { s }.ToList();
list.Add(null);

GetOrCreateSlot(Symbol symbol, int containingSlot) ignores the type arguments for constructed members though. It only depends on symbol.OriginalDefinition (and containingSlot). #Resolved

gafter · 2019-05-21T21:31:30Z

@cston Could you please review this? I'd be happy to walk through it with you if it helps. #Resolved

cston · 2019-05-31T23:00:07Z

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

@@ -9,7 +9,7 @@ partial class BoundDagEvaluation
        public override bool Equals(object obj) => obj is BoundDagEvaluation other && this.Equals(other);
        public virtual bool Equals(BoundDagEvaluation other)


Equals [](start = 28, length = 6)

Logged #36102 to consider renaming the Equals method to something more specific. (That shouldn't block this PR.) #Resolved

cston · 2019-05-31T23:03:51Z

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

+        [Fact, WorkItem(34933, "https://github.com/dotnet/roslyn/issues/34933")]
+        public void NoRedundantPropertyRead04()
+        {
+            // Cannot combine the evalations of name here, since we first check if p is Teacher,


evalations [](start = 34, length = 10)

Typo, here and other tests. #Resolved

cston · 2019-05-31T23:09:54Z

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

+            var source = @"using System;
+
+public class Person {
+    public string Name { get; set; }


public string Name { get; set; } [](start = 4, length = 32)

Consider testing with abstract or virtual property with no override in derived type. #Resolved

YairHalberstadt · 2019-06-03T07:55:35Z

@cston
I've made your requested changes. #Resolved

gafter · 2019-06-03T19:16:08Z

@YairHalberstadt Thank you for your contribution!

Don't read the same property twice in pattern matching when accessed …

d7ad250

…through the same underlying object, but references of different types.

YairHalberstadt requested a review from a team as a code owner May 12, 2019 12:26

YairHalberstadt commented May 12, 2019

View reviewed changes

gafter self-assigned this May 13, 2019

gafter added Area-Compilers Feature - Pattern Matching Pattern Matching labels May 13, 2019

gafter added this to the 16.2 milestone May 13, 2019

gafter reviewed May 13, 2019

View reviewed changes

gafter approved these changes May 13, 2019

View reviewed changes

YairHalberstadt added 2 commits May 14, 2019 08:31

Merge remote-tracking branch 'upstream/master' into pattern-matching-…

3cde814

…reduce-number-of-reads

changes based on code review

2e2b5ad

YairHalberstadt added 2 commits May 15, 2019 08:30

Add doc comment to NullableWalker.AsMemberOfType

1682199

Added example to doc comment for NullableWalker.AsMemberOfType()

adb31b8

RikkiGibson requested a review from agocke May 17, 2019 21:29

gafter requested a review from cston May 21, 2019 21:31

cston mentioned this pull request May 31, 2019

Replace BoundDagEvaluation.Equals and GetHashCode with specific method for testing equivalence #36102

Closed

cston reviewed May 31, 2019

View reviewed changes

Add tests for abstract and virtual property reads

1b1cf93

cston approved these changes Jun 3, 2019

View reviewed changes

gafter merged commit 59b717d into dotnet:master Jun 3, 2019

gafter mentioned this pull request Jun 3, 2019

Pattern Matching: Multiple property reads #34933

Closed

		@@ -9,7 +9,7 @@ partial class BoundDagEvaluation
		public override bool Equals(object obj) => obj is BoundDagEvaluation other && this.Equals(other);
		public virtual bool Equals(BoundDagEvaluation other)

Conversation

YairHalberstadt commented May 12, 2019

Uh oh!

YairHalberstadt May 12, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YairHalberstadt May 13, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

gafter commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented May 14, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented May 14, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 31, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 31, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 31, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YairHalberstadt commented Jun 3, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YairHalberstadt May 12, 2019 •

edited by gafter

Loading

YairHalberstadt May 13, 2019 •

edited by gafter

Loading

gafter May 13, 2019 •

edited

Loading

gafter May 13, 2019 •

edited

Loading

gafter commented May 13, 2019 •

edited

Loading

YairHalberstadt commented May 14, 2019 •

edited by gafter

Loading

cston commented May 14, 2019 •

edited by gafter

Loading

gafter commented May 21, 2019 •

edited

Loading

cston May 31, 2019 •

edited by gafter

Loading

cston May 31, 2019 •

edited by gafter

Loading

cston May 31, 2019 •

edited by gafter

Loading

YairHalberstadt commented Jun 3, 2019 •

edited by gafter

Loading