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

Making a member virtual should be considered a breaking change #21750

Closed
MichalStrehovsky opened this issue May 17, 2017 · 2 comments
Closed
Labels
area-Meta documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@MichalStrehovsky
Copy link
Member

The breaking change document currently considers making a member virtual a non-breaking change with a small sidenote. When that rule was decided, the C# compiler was only emitting call IL instruction in a couple places that didn't really matter and used the IL callvirt for everything else (even if the destination instance method was non-virtual, because per the C# spec, calling instance methods with a null this needs to throw and call IL instruction won't throw). This made it unlikely that people would hit problems with it.

The addition of the null-propagation operator gave the C# compiler more opportunities to optimize callvirt into a call if the this is known to be not null.

dotnet/corert#3565 has an example where making a member virtual resulted in wrong method being called when compiling against PCL contracts because a breaking change was made making ConstructorInfo.Invoke virtual:

var abc = (ABC) constr.Invoke(new object[0]); // This works
var abc = (ABC) constr?.Invoke(new object[0]); // This calls the wrong method because
                                               // `this` is known not to be null and Invoke
                                               // is a non-virtual method

Making a member virtual is a breaking change for anyone using the null-propagation operator with the method.

@danmoseley
Copy link
Member

danmoseley commented May 17, 2017

Related to dotnet/corefx#18138

@karelz
Copy link
Member

karelz commented May 18, 2017

Next step: We need PR with right wording ...

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

4 participants