Fix Issue 18578 - First enum value assigned 0 instead of EnumBaseType.init #7996

LemonBoy · 2018-03-10T18:29:51Z

The value of an EnumMember is given by its AssignExpression. If there is no AssignExpression and it is the first EnumMember, its value is the .init property of the EnumMember's type.

dlang-bot · 2018-03-10T18:29:52Z

Thanks for your pull request and interest in making D better, @LemonBoy! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#7996"

thewilsonator · 2018-03-11T07:14:58Z

This changes the value of a in and subsequently causes b to give an error. This is the cause of the Semaphore failure.

enum X : char
{
    a,
    b
}

ditto for wchar and dchar.

LemonBoy · 2018-03-11T17:13:08Z

I see, the use of float as base type also gives different results since float.init is NaN.
I wonder how many dub packages are broken by this change, the testing pipeline shows a single case so far.

wilzbach · 2018-03-12T05:49:40Z

I wonder how many dub packages are broken by this change, the testing pipeline shows a single case so far.

According to Jenkins (the project tester) this triggers a segfault in libasync.

wilzbach · 2018-03-13T16:52:29Z

Also fails in Phobos with:

std/stdio.d(3818): Error: enum member `std.stdio.__unittest_L3808_C9.EC.B` initialization with `EC.A+1` causes overflow for type `char`

marler8997 · 2018-03-16T10:55:36Z

https://issues.dlang.org/show_bug.cgi?id=18578

LemonBoy · 2018-03-20T11:52:15Z

Hmm, this change may end up breaking some stuff...let's do a deprecation cycle first (or add some warnings/errors) ?

wilzbach · 2018-03-20T11:56:13Z

let's do a deprecation cycle first (or add some warnings/errors)

Warnings are often set to errors in some downstream projects, so deprecations are probably better here.

WalterBright · 2018-03-26T19:22:19Z

It clearly will need a deprecation cycle. If defaultInitLiteral does not return 0, then issue the deprecation.

WalterBright · 2018-03-26T19:31:34Z

Please include test case from https://github.com/dlang/dmd/pull/8090/files

LemonBoy · 2018-03-28T07:44:03Z

I've rebased this on top of #8090, what's missing is to adjust the failures in phobos.

marler8997 · 2018-03-28T16:35:04Z

This PR should fix the phobos issues: dlang/phobos#6368

n8sh · 2018-03-28T18:31:00Z

Maybe it would be better to change the language specification than to change this, because this utterly breaks for floating point types. See further on in the specification:

If there is no AssignExpression and it is not the first EnumMember, it is given the value of the previous EnumMember+1. If the value of the previous EnumMember is EnumBaseType.max, it is an error. If the value of the previous EnumMember+1 is the same as the value of the previous EnumMember, it is an error. (This can happen with floating point types.)

So you'll get an enum whose every member is NaN or just a compile-time error, depending on which kind of equality is meant by "the same as".

It also breaks for any char enum with more than one member, since char.max is char.init. Same for wchar.

marler8997 · 2018-03-28T18:40:05Z

Yeah maybe there is a better way to define how enum values are determined. However, we are talking about a case that doesn't make much sense in the first place, i.e.

enum Foo : float
{
    a, b
}

What would you expect the value of a and b to be? This is such an odd usage of enums that having them default to NaN may actually be fine since this doesn't really make sense in the first place. You could make a case for the char type...but I can't really think of a use case where you would use char as a base type for an enum...

n8sh · 2018-03-28T18:44:42Z

This is such an odd usage of enums that having them default to NaN may actually be fine since this doesn't really make sense in the first place.

That is worse than failing to compile IMO.

marler8997 · 2018-03-28T18:47:59Z

That is worse than failing to compile IMO.

Maybe...having all the members of an enum default to the same values does sound counter-intuitive to what an enum does, namely, enumerate a set of values. But I can't think of a use case where you would ever enumerate a set of values where float is the base type, unless you were explicitly assigning values to them, can you?

marler8997 · 2018-03-28T18:49:28Z

Oh wait...I think I misunderstood you a bit. I guess you were saying it would be better to fail in this case rather than assigning every value to NaN. Yes I agree that failing in that case would be better.

JackStouffer · 2018-03-29T00:25:21Z

Please don't forget to update dlang.org's deprecation page as well. Please see dlang/DIPs#108 for an in draft process for deprecations of this nature.

RazvanN7 · 2018-04-10T15:35:42Z

ping @LemonBoy . what's the status on this?

Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.

thewilsonator · 2018-11-22T14:02:39Z

@RazvanN7 status is rebased and green.

Geod24 · 2018-11-22T14:08:34Z

src/dmd/dsymbolsem.d

+            {
+                deprecation(em.loc,
+                            "Enum `%s` has base type `%s` with non-zero .init; " ~
+                            "initialize the enum member explicitly",


The message should indicate why. Namely that the default value will change in a later release.
Also, there's no changelog entry.

Geod24 · 2018-11-22T14:10:18Z

Removed auto-merge to give time to address feedback. I think it's important to tell the user why they need to take a certain action when something is deprecated. Also, deprecations need a changelog entry.

n8sh · 2018-11-22T19:15:28Z

I urge you all again to reconsider this. "The first element of an enum with base type T defaults to T.init" is very reasonable. "For every T, T.init should be an invalid value if possible" is reasonable. But combining the two leads to something very unreasonable.

Removed auto-merge to give time to address feedback. I think it's important to tell the user why they need to take a certain action when something is deprecated. Also, deprecations need a changelog entry.

This is going to break existing code. People will right ask why a char enum's first element being 0xff is useful or helps anyone write better programs. This new behavior is a bear trap.

Geod24 · 2018-11-22T19:53:49Z

People will right ask why a char enum's first element being 0xff is useful or helps anyone write better programs.

At the same time, I don't think enums based on char are that common. Why wouldn't you use [u]byte instead ? The language does support it, but it's by no means a first class citizen.

WalterBright · 2018-11-23T09:11:54Z

I suspect we should just fix the spec to match the behavior. This won't break existing code, and seems to be what people expect anyway.

marler8997 · 2018-11-23T15:06:13Z

Write a DIP

thewilsonator · 2018-11-24T12:27:30Z

@WalterBright in reality this affects enums of char, wchar, dchar, float double and real none of which enums of are particularly common nor useful. OTOH for char, it is what C would do. Please confirm that you want to change the spec, I don't think a DIP is necessary for this, this is making the spec match the implementation.

Note that the special enum __c_wchar_t seems to already have this behaviour without this PR.

dlang-bot added the Severity:Bug Fix label Mar 10, 2018

LemonBoy force-pushed the enum-init branch from 21831d4 to dd82f43 Compare March 10, 2018 18:30

ibuclaw self-assigned this Mar 11, 2018

marler8997 mentioned this pull request Mar 26, 2018

Fix Issue 18578 - First enum value assigned 0 instead of EnumBaseType.init #8090

Merged

WalterBright changed the title ~~Initialize the enum as written in the specification~~ Fix Issue 18578 - First enum value assigned 0 instead of EnumBaseType.init Mar 26, 2018

LemonBoy force-pushed the enum-init branch 4 times, most recently from 21fad14 to 6181a6f Compare March 27, 2018 16:24

marler8997 mentioned this pull request Mar 28, 2018

Accomodate Enum init deprecation dlang/phobos#6368

Merged

Deprecate the implicit conversion to zero of enum initializers

1066b4d

Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.

thewilsonator force-pushed the enum-init branch from 6181a6f to 1066b4d Compare November 22, 2018 13:01

thewilsonator self-assigned this Nov 22, 2018

RazvanN7 approved these changes Nov 22, 2018

View reviewed changes

RazvanN7 added the Merge:auto-merge label Nov 22, 2018

Geod24 reviewed Nov 22, 2018

View reviewed changes

Geod24 removed the Merge:auto-merge label Nov 22, 2018

dlang-bot added the Review:stalled label May 20, 2021

dlang-bot added the Review:Needs Rebase label Jul 10, 2022

dlang-bot added the Review:Needs Work label Nov 3, 2022

ntrel mentioned this pull request Sep 9, 2023

[spec] First enum member is converted from 0 to base type dlang/dlang.org#3694

Merged

dlang-bot added Merge:needs rebase stalled labels Dec 4, 2024

Uh oh!

Fix Issue 18578 - First enum value assigned 0 instead of EnumBaseType.init #7996

Are you sure you want to change the base?

Fix Issue 18578 - First enum value assigned 0 instead of EnumBaseType.init #7996

Uh oh!

Conversation

LemonBoy commented Mar 10, 2018

Uh oh!

dlang-bot commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator commented Mar 11, 2018

Uh oh!

LemonBoy commented Mar 11, 2018

Uh oh!

wilzbach commented Mar 12, 2018

Uh oh!

wilzbach commented Mar 13, 2018

Uh oh!

marler8997 commented Mar 16, 2018

Uh oh!

LemonBoy commented Mar 20, 2018

Uh oh!

wilzbach commented Mar 20, 2018

Uh oh!

WalterBright commented Mar 26, 2018

Uh oh!

WalterBright commented Mar 26, 2018

Uh oh!

LemonBoy commented Mar 28, 2018

Uh oh!

marler8997 commented Mar 28, 2018

Uh oh!

n8sh commented Mar 28, 2018

Uh oh!

marler8997 commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n8sh commented Mar 28, 2018

Uh oh!

marler8997 commented Mar 28, 2018

Uh oh!

marler8997 commented Mar 28, 2018

Uh oh!

JackStouffer commented Mar 29, 2018

Uh oh!

RazvanN7 commented Apr 10, 2018

Uh oh!

thewilsonator commented Nov 22, 2018

Uh oh!

Geod24 Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Nov 22, 2018

Uh oh!

n8sh commented Nov 22, 2018

Uh oh!

Geod24 commented Nov 22, 2018

Uh oh!

WalterBright commented Nov 23, 2018

Uh oh!

marler8997 commented Nov 23, 2018

Uh oh!

thewilsonator commented Nov 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

dlang-bot commented Mar 10, 2018 •

edited

Loading

marler8997 commented Mar 28, 2018 •

edited

Loading