KEMBAR78
delete enum support for bool as an underlying type by pedrobsaila · Pull Request #79962 · dotnet/runtime · GitHub
Skip to content

delete enum support for bool as an underlying type #79962

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jan 13, 2023
Merged

delete enum support for bool as an underlying type #79962

merged 14 commits into from
Jan 13, 2023

Conversation

@pedrobsaila
Copy link
Contributor

Fixes #79224

@ghost ghost added area-System.Runtime community-contribution Indicates that the PR has been added by a community member labels Dec 25, 2022
@ghost
Copy link

ghost commented Dec 25, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #79224

Author: pedrobsaila
Assignees: -
Labels:

area-System.Runtime
Milestone: -

@vargaz
Copy link
Contributor

vargaz commented Dec 28, 2022

The mono changes look ok to me.

@build-analysis build-analysis bot mentioned this pull request Dec 28, 2022
Closed
@build-analysis build-analysis bot mentioned this pull request Dec 30, 2022
Closed
stephentoub
stephentoub approved these changes Jan 3, 2023
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. @jkotas, your suggestion was about considering removing bool; we later got data confirming it's very rarely (but not 100% never) used. Still want to nuke it?

jkotas
jkotas reviewed Jan 6, 2023
View reviewed changes
docs/design/specs/Ecma-335-Augments.md Outdated

### II.14.3 Enums (page 168)

Enum support for bool as underlying type was deleted (see [issue 79224](https://github.com/dotnet/runtime/issues/79224)).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is deleting the bool backed enum support from the reflection stack.

The bool backed enums are still going to work loaded fine by the type loader. I think that it is the right tradeoff between compat and simplicity. The type loader support for bool backed enums is very cheap and simple.

ECMA-335 and this doc is concerned about the behavior of the core runtime (that is not changing), not about behavior of BCL APIs. I think this note should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the 'type loader' ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type loader is a component responsible for loading types.

For example, Console.WriteLine(typeof(BoolEnum)); is still going to work fine even with these changes because the type loader still works fine for bool enums with these changes.

@jkotas
Copy link
Member

jkotas commented Jan 6, 2023 •
edited
Loading

Still want to nuke it?

Yes, I think it is fine to nuke it from parsing, formatting and conversions. I would keep it in the core type loader - it does not cost anything there.

jkotas
jkotas reviewed Jan 10, 2023
View reviewed changes
jkotas
jkotas reviewed Jan 10, 2023
View reviewed changes
jkotas
jkotas approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@jkotas jkotas merged commit f32ba52 into dotnet:main Jan 13, 2023
@pedrobsaila pedrobsaila deleted the 79224 branch January 13, 2023 08:30
@akoeplinger
Copy link
Member

Can we update https://github.com/dotnet/runtime/blob/main/docs/design/specs/Ecma-335-Augments.md with this?

@stephentoub
Copy link
Member

Can we update https://github.com/dotnet/runtime/blob/main/docs/design/specs/Ecma-335-Augments.md with this?

https://github.com/dotnet/runtime/pull/79962/files/a8bd8383cb3a54a4864de06d077ac967fa7a6aab#diff-30929ad6de186623e8903e6516a190745da62d62df407351f1efee396cfafd7b

@akoeplinger
Copy link
Member

ah ok, thanks

@stephentoub stephentoub added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Jan 20, 2023
@ghost
Copy link

ghost commented Jan 20, 2023 •
edited by jkotas
Loading

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@stephentoub stephentoub mentioned this pull request Jan 20, 2023
Closed
jkotas added a commit to jkotas/runtime that referenced this pull request Jan 20, 2023
@jkotas
Fix Enum.ToObject for bool to Enum
0ebea2e 
Partial revert of dotnet#79962

Fixes dotnet#80890
@jkotas jkotas mentioned this pull request Jan 20, 2023
Merged
@jkotas
Copy link
Member

jkotas commented Jan 20, 2023

Breaking change doc issue dotnet/docs#33647

stephentoub pushed a commit that referenced this pull request Jan 20, 2023
@jkotas
Fix Enum.ToObject for bool to Enum (#80893)
ce934fd 
Partial revert of #79962

Fixes #80890
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@jkotas @mdh1418
Fix Enum.ToObject for bool to Enum (dotnet#80893)
7012da2 
Partial revert of dotnet#79962

Fixes dotnet#80890
@jaredpar jaredpar mentioned this pull request Jan 30, 2023
Open
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
@ericstj ericstj added this to the 8.0.0 milestone Jun 28, 2023
@jkotas jkotas removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 16, 2023
@jkotas
Copy link
Member

jkotas commented Oct 16, 2023

Breaking change doc created by dotnet/docs#36505

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@jkotas jkotas jkotas approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

@vargaz vargaz Awaiting requested review from vargaz

@lambdageek lambdageek Awaiting requested review from lambdageek

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

+1 more reviewer

@MichalPetryka MichalPetryka MichalPetryka left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jkotas jkotas

Labels

area-System.Runtime breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider deleting enum support for bool as an underlying type from all runtimes

7 participants

@pedrobsaila @vargaz @jkotas @akoeplinger @stephentoub @MichalPetryka @ericstj