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

Select better auto-property backing field type to support devirtualization #30992

Closed
wants to merge 2 commits into from
Closed

Select better auto-property backing field type to support devirtualization #30992

wants to merge 2 commits into from

Conversation

AustinWise
Copy link
Contributor

Fixes #30797

@AustinWise AustinWise requested a review from a team as a code owner November 6, 2018 18:29
@jaredpar
Copy link
Member

jaredpar commented Nov 6, 2018

Thanks for the contribution here. Couple of things:

  1. We still need to clarify whether or not we want to take this change with the language design team. I'm optimistic this will be supported but I need to ensure that's the case.
  2. Tests: with this change I would expect to see a number of tests here, both positive and negative. There don't seem to be any tests so far.

@jaredpar jaredpar added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Nov 6, 2018
@AustinWise
Copy link
Contributor Author

I got this working in the editor experience and the compiler. There is still some work to do before merging, but I'd like some feedback to make sure this approach is not totally wrong.

Things I think I need to do before merging:

  • Add unit tests
  • Perhaps refactor the binder code so that BindVariableOrAutoPropInitializerValue and this new code share the same logic for determining field conversions.

@jaredpar jaredpar self-assigned this Nov 6, 2018
@AustinWise
Copy link
Contributor Author

Rebased and updated with some unit tests. These mostly focus on the positive cases (implicit reference conversions from the initializer expression to the property type).

@jaredpar
Copy link
Member

jaredpar commented Nov 9, 2018

In the process of checking with LDM about this particular change we ended up discovering some problems with the proposal. I added a lot of detail to the original issue + CC'd you.

#30797

@AustinWise
Copy link
Contributor Author

As mentioned in #30797, the fact that constructors can reassign readonly auto properties make this approach intractable.

Interesting, there is no test currently that excises this feature in a way that was broken by my changes. Perhaps it would be worth adding a little unit test to exercise this case:

class A { }
class B1 : A { }
class B2 : A { }
class C
{
    public C()
    {
        P = new B2();
    }
    A P { get; } = new B1();
}

@AustinWise AustinWise closed this Nov 9, 2018
@jaredpar
Copy link
Member

jaredpar commented Nov 9, 2018

I agree. I was surprised as well. I got lulled into a false sense of completeness when your PR passed the first round of testing.

Again thanks for trying to get this optimization in! Dissapointing that it's harder than we thought but also good to have a firm resolution on it.

@AustinWise AustinWise deleted the austin/PropertyBackingField branch November 19, 2018 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants