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.

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

Pattern Matching: Multiple property reads #34933

Closed
HaloFour opened this issue Apr 11, 2019 · 4 comments
Closed

Pattern Matching: Multiple property reads #34933

HaloFour opened this issue Apr 11, 2019 · 4 comments

Comments

@HaloFour
Copy link

I understand that the number of property reads to facilitate pattern matching is undefined so that the compiler can optimize for as few reads as it can. I was curious about the following case as the compiler emits two reads for the property Name although the compiler should be able to deduce that both are for the same property on the Person class and could theoretically optimize it down to one read:

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;
        }
    }
}

This results in IL that accesses Name twice, once for the first case and once for the second case. I assume that this is due to the type-switch perhaps interfering with internal bookkeeping and I can't think of a reason why it would be "Correct"™ to emit the two reads.

Decompilation of the resulting IL:

public class C
{
    public void M(Person p)
    {
        if (p != null)
        {
            string name = p.Name; // read #1
            if (name == null || !(name == "Bill"))
            {
                Student student = p as Student;
                if (student != null)
                {
                    string name2 = student.Name; // read #2
                    Console.WriteLine("Hello student " + name2 + "!");
                }
                else
                {
                    Console.WriteLine("Hello non-student " + name + "!");
                }
            }
            else
            {
                Console.WriteLine("Hey Bill!");
            }
        }
    }
}

This would make sense if Student shadowed Name, but it doesn't and the compiler is aware of that as it emits callvirt calls to Person::get_Name both times. If Name was virtual it would still only necessitate a single call.

Any particular reason for this, or is it just a case that the compiler hasn't optimized for?

@gafter
Copy link
Member

gafter commented Apr 11, 2019

We just haven't optimized the case of the same property being accessed from different types.

@gafter gafter self-assigned this Apr 11, 2019
@gafter gafter added this to the Backlog milestone Apr 11, 2019
@YairHalberstadt
Copy link
Contributor

Is the compiler team prepared to continue making changes to the number of times properties/deconstruct methods are called by patterns once C# 8.0 is shipped, even though this may break existing code?
I presume that if so this could be done in future releases, but if not this is more urgent?

@gafter
Copy link
Member

gafter commented Apr 12, 2019

@YairHalberstadt Yes, we expect to have such changes going forward. However, we want to be intentional about when changes occur.

@gafter
Copy link
Member

gafter commented Jun 3, 2019

Fixed in #35669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants