Bug 14269 – Enum is not implicitly converted to base type when using ref

The following code compiles with 2.066.1 and lower but does not compile with 2.067 Beta 3. enum Bits: ulong { none = 0 } bool hasBit(ref ulong rBits, ulong rBit) { return cast(bool)(rBits & rBit); } void main() { Bits bits; hasBit(bits, Bits.none); }

I would argue that it shouldn't compile, but I'm a weird one.

According to Jonathan Davis it should not compile, as there are no implicit conversions with ref. It *is* a regression in behaviour, though.

It shouldn't compile but, same as with other such changes, it needs deprecation stage.

(In reply to Dicebot from comment #3) > It shouldn't compile but, same as with other such changes, it needs > deprecation stage. Deprecation is only done with feature changes. It's quite normal for "accepts-invalid" bugs to be fixed and just break existing code, because the code was considered broken to begin with.

> It's quite normal It is common, not normal. And this attitude needs to change if D wants to be considered stable for production usage for wider audience. Fortunately, so far seem that Martin shares this opinion and we may get a more reasonable approach in this release. This fix breaks 100% working code with no preliminary deprecation warning. Change itself may be completely justified on its own but not justified enough to break things _immediately_.

(In reply to Dicebot from comment #5) > And this attitude needs to change if D wants to be > considered stable for production usage for wider audience. "For the stability, we have to defer to fix bug by adding deprecation stage"? Deferring bugfix will clearly make compiler unstable. It sounds unreasonable to me.

> "For the stability, we have to defer to fix bug by adding deprecation stage" Yes, unless it compromises program correctness (memory corruption, wrong codegen etc). It is a normal practice in majority of established languages I am aware of. There are several reasons for such conservative approach: 1) make possible to use critical fixes immediately without scheduling time for addressing minor ones 2) make possible to use new version even if dependencies have not been updated (and are out of your control) 3) give easy quick estimate of what full upgrade will take effort-wise It all evolves around the idea of introducing time gap when everyone becomes aware of the issue but is not yet forces to address it immediately to use the compiler. For example, our internal D codebase consists from dozens of different projects with independent maintainers and complicated dependency chains. Any breaking change means that upgrade effort needs to be done in sync between all those projects, taking away time from possibly urgent business matters (and it is almost impossible to find a single moment when none of projects has any urgent matters). Breaking fix needs to address a very serious problem to be viewed as worth such trouble among the developers. Adding deprecation stage allows everyone to switch to new compiler with minor effort and address all deprecation messages on per-project basis as schedule for this specific project allows. I foresee this becoming a major issue once our D2 usage leaves experimental stage - this is why I am starting to make so much fuss early. I am very sorry if this sounds overly demanding but current state of affairs is really problematic.

(In reply to Dicebot from comment #7) > unless it compromises program correctness (memory corruption, wrong > codegen etc). In 2.066 and earlier, the program correctness had been compromised and generated wrong code. So the bug should be fixed immediately in the 2.067 release. I'm not sure why you think the bug is trivial.

So if we examine why we don't allow ref to implicitly convert, it mostly has to do with preventing one from replacing the data referred to with a value that's valid for the base type, but invalid for the derived type. This is especially important for classes and pointers, because the pointer means completely different things as a base or derived type. However, we have somewhat of a unique situation with enum: 1. An enum is GUARANTEED to be the same size as it's "base" type, and have the same representation. 2. An enum is GUARANTEED to behave exactly the same way in terms of operators as its base type. 3. An enum is frequently used as a simple typedef, allowing one to define a new type of "int" e.g. with extra static values. I think you can make cases either way. The deprecation of typedef has kind of forced people into using enum as a "poor man's typedef". But clearly, there is no inherent danger in allowing ref of base type to bind to an enum, as the compiler already cannot make any assumptions about enums that are different from the base type. I would tend to say we should revert the restriction, even though I see and somewhat agree with the reasoning behind it. It seems inconsistent to say "We allow you to do enumvar |= somelong" and still type it as the enum, but we can't allow you arbitrary access via long. I would love to see a mechanism to allow enums to be further classified. In other words, what is allowed with this enum type? How is it intended to be used? This would give more clarity to how enum types are supposed to be used, and how the compiler can restrict usage.

Personally, I think that all operations that the compiler can't guarantee will result in a valid enum value for an enum type should result in the base type of the enum rather than the enum type - without that, stuff like final switch is completely broken - but there's definitely dissent on that, and it's been argued about in the newsgroup.

(In reply to Jonathan M Davis from comment #10) > Personally, I think that all operations that the compiler can't guarantee > will result in a valid enum value for an enum type should result in the base > type of the enum rather than the enum type - without that, stuff like final > switch is completely broken - but there's definitely dissent on that, and > it's been argued about in the newsgroup. I agree with Jonathan, of course. Fixing final switch is important.

(In reply to Jonathan M Davis from comment #10) > Personally, I think that all operations that the compiler can't guarantee > will result in a valid enum value for an enum type should result in the base > type of the enum rather than the enum type - without that, stuff like final > switch is completely broken - but there's definitely dissent on that, and > it's been argued about in the newsgroup. The OP's case has nothing to do with final switch. The reality is that people use enums for reasons other than final switch or even just individual values (e.g. bitfields). But I don't see how final switch can be "fixed", I can't imagine a case where it's worth not checking for the default case in a final switch.

If the implicitly-converted value is an rvalue (as it is), where is it stored to take the reference of? Since it is not possible to take the address of an rvalue simply because it may be sitting in a register, I can't see how this "fix" can be reverted without special-casing the compiler. What kinds of rvalues should we start taking references of? It is unfortunate that this breaks compilation of wrong code but I think the extent of code change should be minimal. Anyway, 2.066.1 is still available. :) Ali

(In reply to Ali Cehreli from comment #13) > If the implicitly-converted value is an rvalue (as it is), where is it > stored to take the reference of? No, an enum is a derivative. It's just like you can pass a class reference as Object without consequence. Where you run into issues is if you have a REFERENCE to a class reference. So the implicit casting is not because enum converts to long (for example), it's because enum IS a long. And in addition, enum does not allow you to really treat it any differently than the base type. You can't add any features that "wouldn't work" for a long, for instance.

(In reply to Dicebot from comment #3) > It shouldn't compile but, same as with other such changes, it needs > deprecation stage. That would imply that deprecations are ever done in D ;-) How long has it been now since it was decided that delete will be deprecated?

(In reply to Kenji Hara from comment #8) > (In reply to Dicebot from comment #7) > > unless it compromises program correctness (memory corruption, wrong > > codegen etc). > > In 2.066 and earlier, the program correctness had been compromised and > generated wrong code. So the bug should be fixed immediately in the 2.067 > release. > > I'm not sure why you think the bug is trivial. Slightly modified version of original snippet: enum Bits: ulong { none = 0 } void hasBit(ref ulong rBits,) { rBits = 424242; } void main() { Bits bits; hasBit(bits); assert(bits == 424242); } What in your opinion compromises program correctness here? This code works in 2.066 and works as intended by author, reinterpreting memory taken by `bits` as ulong (as those are binary identical). There is not undefined behavior, no memory corruption, no bug in application logic. Only issue is the fact that D type system was not supposed to work in such permissive way.

Also, please, can we discuss the final switch / enum operation issue somewhere else? It is unrelated and only makes more difficult to follow relevant discussion.

I think this boils down to whether an enum's conversion to its base type is an implicit cast (an rvalue), versus the enum itself (an lvalue). (Steven says it's the latter.) Also, does anyone know what bug fix this was originally about? Perhaps that bug was invalid? Ali

(In reply to Ali Cehreli from comment #18) > I think this boils down to whether an enum's conversion to its base type is > an implicit cast (an rvalue), versus the enum itself (an lvalue). (Steven > says it's the latter.) > > Also, does anyone know what bug fix this was originally about? Perhaps that > bug was invalid? > > Ali Kenji is likely to know but I will do the bisection if he won't chime in soon :)

(In reply to Dicebot from comment #19) > (In reply to Ali Cehreli from comment #18) > > Also, does anyone know what bug fix this was originally about? Perhaps that > > bug was invalid? > > > > Ali > > Kenji is likely to know but I will do the bisection if he won't chime in > soon :) https://issues.dlang.org/show_bug.cgi?id=13783

(In reply to Ali Cehreli from comment #18) > I think this boils down to whether an enum's conversion to its base type is > an implicit cast (an rvalue), versus the enum itself (an lvalue). (Steven > says it's the latter.) Since this change has broken code, the question to answer is, does this break code that should be broken? Final switch does not show a case of invalid code that is now fixed by this change, and I haven't seen any other challenges of breakage. So if it doesn't break anything worth breaking, why are we doing it? To me, this doesn't break any code that is invalid. Regardless of whether it's philosophically correct or not, avoiding breaking only valid code should take precedence. That is why I said I understand the reason and somewhat agree with the reason, but I still think it should be reverted.

I think that allowing an implicit conversion to be used with a ref parameter is a clear violation of the type system. If we want to fix it via deprecation rather than immediately making it an error like we'd normally do with an "accepts-invalid" bug on the theory that the code actually works in spite of violating the type system, then fine. But I don't think that it's at all a good idea to leave the code as valid long term.

(In reply to Jonathan M Davis from comment #22) > I think that allowing an implicit conversion to be used with a ref parameter > is a clear violation of the type system. If we want to fix it via > deprecation rather than immediately making it an error like we'd normally do > with an "accepts-invalid" bug on the theory that the code actually works in > spite of violating the type system, then fine. But I don't think that it's > at all a good idea to leave the code as valid long term. This was my point from the very first comment here ;) Yes, this is bad code and disallowing it will make type system more uniform. But it isn't inherently broken and thus deprecation stage is mandatory. I will look into relevant code closer to the weekend if no one else will have done it by that point.

(In reply to Jonathan M Davis from comment #22) > I think that allowing an implicit conversion to be used with a ref parameter > is a clear violation of the type system. class C {} void foo(Object o); void main() { C c = new C; foo(c); // works, and passed by ref! } I think clearly there is a similar relationship with enums and their base type.

(In reply to Steven Schveighoffer from comment #24) > foo(c); // works, and passed by ref! but it's in no way passed by ref! O_O

class C {} void foo (Object o) { o = new Object(); } void main () { C c = new C; auto n = c; foo(c); assert(c == n); // assertion passed }

ah, sorry, replace that assert with `assert(c is n);` for clarity.

The Object contents are passed by ref, just like the enum contents are. The difference is that the ref is implicit for the object. In other words, a ref to a C object is implicitly passable as a ref to a base Object. A ref to a C object *reference* is not passable as a ref to a base Object reference for correct reasons. The equivalent to your code for enums would be: void foo (int *o) { o = new int; } enum E : int; void main () { E *c = new E; auto n = c; foo(c); assert(c == n); // assertion passed }

(In reply to Steven Schveighoffer from comment #24) > (In reply to Jonathan M Davis from comment #22) > > I think that allowing an implicit conversion to be used with a ref parameter > > is a clear violation of the type system. > > class C {} > > void foo(Object o); > > void main() > { > C c = new C; > foo(c); // works, and passed by ref! > } > > I think clearly there is a similar relationship with enums and their base > type. In that case, you're not passing by ref. You're dealing with a reference type - and a polymorphic one at that. There is nothing polymorphic about enums unless they happen to have a class type as their base type. And this code does _not_ compile: class C {} void foo(ref Object o); void main() { C c = new C; foo(c); } ref and class references are not the same thing at all.

This is a simple "accepts-wrong-code" bug that needs to be fixed with no deprecation. Any code that relies on this bug was wrong.

(In reply to Andrei Alexandrescu from comment #30) > This is a simple "accepts-wrong-code" bug that needs to be fixed with no > deprecation. Any code that relies on this bug was wrong. I don't know if this makes things clear. What should be wrong code? The situation in the unreleased compiler or the situation in 2.066.1?

(In reply to Steven Schveighoffer from comment #31) > (In reply to Andrei Alexandrescu from comment #30) > > This is a simple "accepts-wrong-code" bug that needs to be fixed with no > > deprecation. Any code that relies on this bug was wrong. > > I don't know if this makes things clear. What should be wrong code? The > situation in the unreleased compiler or the situation in 2.066.1? Code that attempts to convert an lvalue of enum type to a ref to its base type is wrong. The compiler shouldn't accept it. Correcting it qualifies as a simple bug fix.

(In reply to Andrei Alexandrescu from comment #32) > Code that attempts to convert an lvalue of enum type to a ref to its base > type is wrong. The compiler shouldn't accept it. Correcting it qualifies as > a simple bug fix. OK, so that is what already happened. I don't agree with it, but I don't see that it is worth having a battle over.

Here's the change that did it: https://github.com/D-Programming-Language/dmd/pull/4177/files Essentially, it's the removal of the toBasetype() call. The problem with deprecating rather than erroring is this function is not checking for errors, it does match levels, which can cause various changes upstream.

After small e-mail conversation with Walter we came to compromise that this specific issue will remain closed as deprecation can possibly be more problematic than the fix but this won't be considered a normal practice to do and deprecations for non-critical bug fixes won't be rejected.