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

Disallow sealed overrides in structs #11320

Merged
merged 1 commit into from
May 24, 2016
Merged

Disallow sealed overrides in structs #11320

merged 1 commit into from
May 24, 2016

Conversation

lorcanmooney
Copy link
Contributor

@lorcanmooney lorcanmooney commented May 15, 2016

Fixes #11170. This reverts to the pre-Roslyn behaviour of generating an error for sealed overrides in a struct (that's assuming it's an acceptable breaking change).

@gafter gafter added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. 4 - In Review A fix for the issue is submitted for review. labels May 15, 2016
@jmarolf
Copy link
Contributor

jmarolf commented May 20, 2016

@dotnet/roslyn-compiler @dotnet/roslyn-compat-council

@AnthonyDGreen
Copy link
Contributor

What's the scenario motivating this change?

@gafter
Copy link
Member

gafter commented May 20, 2016

@AnthonyDGreen This is mainly a matter of complying with both the language specification and the previous native compiler behavior. If we're ever going to fix the missing diagnostic bug, fixing it sooner narrows the window in which customers could depend on it.

@lorcanmooney
Copy link
Contributor Author

There's a more practical problem with the current behaviour in environments with mixed VS versions. If I use VS2015 and change a class to a struct without manually checking for overrides (how I came across this in the first place) I can happily build locally and send the changes on their way. Best case scenario, a VS2013 CI system picks up the change and rejects it. Worst case, there's a VS2015 CI system (or no validation) and now anyone with an older VS/toolset gets a broken build.

@AnthonyDGreen
Copy link
Contributor

Ok.

@jaredpar
Copy link
Member

👍

@gafter
Copy link
Member

gafter commented May 23, 2016

Lets keep this on hold for another day to see if anyone on @dotnet/roslyn-compat-council raises any objections.

@gafter gafter self-assigned this May 23, 2016
@gafter
Copy link
Member

gafter commented May 23, 2016

I have verified that the native compiler rejects a sealed modifier on a method in a struct.

@gafter gafter added this to the 1.3 milestone May 23, 2016
@agocke
Copy link
Member

agocke commented May 23, 2016

LGTM

@gafter We're running up against ask mode, so I'd say we should get this in sooner rather than later.

@gafter
Copy link
Member

gafter commented May 24, 2016

LGTM

@gafter
Copy link
Member

gafter commented May 24, 2016

@MattGertz @jaredpar Tagging you for ask mode.

@MattGertz
Copy link
Contributor

Approved pending successful test completion.

@gafter
Copy link
Member

gafter commented May 24, 2016

@dotnet-bot test windows_debug_unit32_prtest please
@dotnet-bot test windows_debug_unit64_prtest please
@dotnet-bot test windows_eta_open_prtest please

@gafter gafter merged commit 4798c9b into dotnet:master May 24, 2016
@gafter 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 24, 2016
@AlekseyTs
Copy link
Contributor

LGTM

@lorcanmooney lorcanmooney deleted the issue11170 branch July 9, 2016 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved to merge Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants