GDScript: Emit a warning if a non unique enum value is added implicitly

HolonProduction commented 2 weeks ago

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

Related to godotengine/godot#96368

GDScript assigns values to enum variants based on the previous value + 1.

In the following case, this can lead to GDScript assigning a duplicate value implicitly:

enum Test {
TEST1 = 1,
TEST2 = 0,
TEST3,
}

# Test.TEST1 == Test.TEST3

This might be expected for users who are familiar with C, C++, C# or even TypeScript, but for users that come from languages that don't provide access to the underlying value and guarantee unique enum variations (java, kotlin, dart, php, rust to name a few) this can be very confusing. It could lead to bugs that are very hard to track down when users work with the assumption that enum variations are unique expect they explicitly declared a duplicate.

I'd also argue that a lot of beginners understand enums as a set of unique constants, so they would be affected by this confusion as well. (Though it would be nice to get the opinion of some actual beginners on this.)

Lastly if this is intended, it would be better to make it explicit to make clear that the enum contains duplicate values.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

A new error would be added to GDScript. It's standard severity would be ~error~ WARN but users who know about this behavior and want to use it can downgrade it to a warning or ignore it all together.

This error would be emitted if a value that GDScript picked implicitly for an enum variant is already present in the enum. The message would be something in the vein of: "Implicitly adding a duplicate value to an enum. If this is intended declare the value explicitly."

Some examples:

# Emits no error:

enum Test {
TEST1,
TEST2,
TEST3,
}

enum Test {
TEST1=0,
TEST2=1,
TEST3=2,
}

enum Test {
TEST1=1,
TEST2=0,
TEST3=1,
}

enum Test {
TEST1=2,
TEST2=0,
TEST3,
}

# Emits error:

enum Test {
TEST1=1,
TEST2=0,
TEST3, # Conflicts with TEST1
}

enum Test {
TEST1=0,
TEST2,
TEST3=0,
TEST4, # Conflicts with TEST2
}

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

if enum.already_processed_values.has(new_value) and new_value.is_implicit:
- add_error()

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

Needs access to GDScript internals.

AThousandShips commented 2 weeks ago

This should be a warning, set to WARN by default or it breaks compatibility

HolonProduction commented 2 weeks ago

From my experience beginners rarely pay attention to warnings (which might be related to the fact that they are not displayed inline like errors).

Also I think this amount of compatibility breakage for a case that is rarely encountered and can easily be made compatible by either changing a setting or making the duplicate explicit should be fine.

But if this is really a concern it could be set to WARN for existing projects (through the project converter).

AThousandShips commented 2 weeks ago

Having something be an error by default implies that it is bad practice as a rule, not something to watch out for

But I think going "people ignore warnings so we should make it an error" kinda makes all warnings useless, I don't think we should assume things like that, or that beginners even run into this problem or get confused by this, however making peoples' projects suddenly throw a bunch of errors for intentional behavior and have them go find a warning and turn it to WARN is going to cause frustration, since as you say "a case that is rarely encountered"

The suggestion to make the duplicate explicit isn't really feasible as mentioned on the issue report, that's specifically why someone would use implicit values, explicit values are error prone by default as you might change something and it breaks

HolonProduction commented 2 weeks ago

Having something be an error by default implies that it is bad practice

I personally think it is, relying on implicit enum values if you work with the underlying values is at least as error prone as working with explicit values.

For me it feels like an oversight, but I understand, that people with a c++ background might have very different stance on this than me. So it might very well be intended by the developers.

AThousandShips commented 2 weeks ago

For me it feels like an oversight

It could t really be an oversight if the intent was "make it the largest value plus one", that would have been implemented as such, no? It's just "we keep it simple"

I personally think it is, relying on implicit enum values if you work with the underlying values is at least as error prone as working with explicit values.

Not if you change something in the middle for some reason, then you have to do a lot of editing to make sure it's right, that's why implicit enum values are a thing in the first place, to avoid one of the most common errors in programming:

Copy-paste errors or duplicated code errors, where you copy something and forget to change the other place, or where you change something and forget to adjust everything else

Also changing 10 or 15 values manually is inviting carpal tunnel syndrome if you do it often, again why this convenience feature exists

HolonProduction commented 2 weeks ago

The suggestion to make the duplicate explicit isn't really feasible as mentioned on the issue report, that's specifically why someone would use implicit values

Maybe I'm misreading the original issue :thinking:

But to me it sounds like it was not intended to have a duplicate value in there. Maybe @Torguen could clear this up for us?

Not if you change something in the middle for some reason, then you have to do a lot of editing to make sure it's right, that's why implicit enum values are a thing in the first place, to avoid one of the most common errors in programming:

To clarify since I'm not sure if I'm misunderstood here: I think this is a totally valid use case for implicit enum values, they are great and save a lot of trouble.

And while your arguments apply to the way they are usually used, I don't think they apply to the specifc case were a duplicate is implicitly created. If this is intended, copy and paste errors can easily break the intended duplicate, while an explicit duplicate isn't easily broken.

If you have a use case that requires the implicit creation of a duplicate I'd be very interested to hear it, because I just can't think of one.

make it the largest value plus one

I specifically didn't mention this in this proposal since, as I said before, this is not what I expect, it is just a way what I expect could be implemented (that being unique enum variations).

HolonProduction commented 2 weeks ago

tl;dr

I think the best practice when it comes to possibility of errors and ease of changes would be the following:

implicit values for unique variant (you explained why this is good far better then I ever could)
explicit values for non unique variants (I think your arguments don't really apply to this case, since breaking an intended duplicate can happen very easily with implicit duplicates)

AThousandShips commented 2 weeks ago

But I don't see why there's any reason to make this an error by default, adding this as a warning, assuming we think it's justified to add the extra code complexity, makes sense, but having it be an error sends a big message, and creates a lot of potential confusion

There's already work AFAIK to make warnings more visible, but otherwise behavior that isn't obviously wrong should be treated as a warning IMO, not an error

Further, there are common use cases for implicit values that are duplicates, I think you're missing a quirk here of directionality, which might explain why we don't align on this, consider this very common kind of code for example:

enum OperatorTypes {
    UnaryPlus,
    UnaryMinus,
    UnaryNot,

    BinaryPlus,
    BinaryMinus,
    BinaryTimes,
    BinaryDivide,

    TernaryDecide,

    FIRST_UNARY = UnaryPlus,
    LAST_UNARY = UnaryNot,

    FIRST_BINARY = BinaryPlus,
    LAST_BINARY = BinaryDivide,

    FIRST_TERNARY = TernaryDecide,
    LAST_TERNARY = TernaryDecide,
}

This would error, because you have implicit duplicates, but they are intentional, this is very common and is used by major projects like LLVM

Having it detect only cases where the implicit entry is declared after the duplicate doesn't work, and having it not occur if the duplicate is defined by the implicit case doesn't necessarily work either, because it might be defined by an expression like SECOND_TO_LAST_UNARY = LAST_UNARY - 1 or something (convoluted example but still stands)

HolonProduction commented 2 weeks ago

I think you're missing a quirk here of directionality, which might explain why we don't align on this

That in fact clears up to me what you are specifically talking about :+1:

Having it detect only cases where the implicit entry is declared after the duplicate doesn't work

This was kind of what I intended with my pseudo code, why wouldn't it work?

AThousandShips commented 2 weeks ago

This was kind of what I intended with my pseudo code, why wouldn't it work?

I mean where the entry the implicit is a duplicate of is declared later, like my code above, so like:

enum MyEnum {
    FOO,
    BAR, # Should this error?

    BUR = 2,
    BAZ = 1,
    BIZ, # Or should only this error?
}

HolonProduction commented 2 weeks ago

I'd say only BIZ should error, since it is an implicit duplicate of BUR. BAR on the other side is an implicit unique value (since no variation with the same value exists before it). It later gets an explicit duplicate in form of BAZ.

Given that the following isn't possible in GDScript:

enum Test {
TEST1 = TEST2,
TEST2,
}

I feel like this should be a working way to classify explicit and implicit duplicates. But maybe I'm missing something :shrug:

AThousandShips commented 2 weeks ago

Given that the following isn't possible in GDScript

I don't see how that changes anything

Also what about:

enum MyEnum {
    A = 1,
    B,
    C,

    D = 1,
    E,
    F
}

But to me it not considering later duplicates like you suggest makes this far less helpful and far more confusing, why is a duplicate okay in some cases and not others? What if:

enum MyEnum {
    A = 5,
    B,
    C,

    X = 6,
    Y = 7,
}

That's as likely to be wrong as:

enum MyEnum {
    X = 6,
    Y = 7,
    A = 5,
    B,
    C,
}

To clarify, I'm saying this might be a good warning, but not an error by default, and my example above is one of many legitimate cases of duplicates that exist in production code, and I'm confused by the IMO inconsistent examples of when to and not to apply this, I'd say:

It should be a warning
It should apply consistently

HolonProduction commented 2 weeks ago

This code is far more likely to be wrong than both of your examples:

enum EnemyState {
BURNING,
POISONED,
}

enum PlayerState {
POISONED = EnemyState.POISONED,
BURNING = EnemyState.BURNING,
HUNGRY,
}

Because the user never interacted with values at all, he just want's to ensure that two different enums are comparable. The fact that PlayerState.HUNGRY == PlayerState.POISONED is very unlikely to be intended.

I think the inconsistency is a fair tradeoff, to have an error in this very specific case.

AThousandShips commented 2 weeks ago

What I'm saying is:

Make it a warning
Make it apply to any implicit duplicate, no matter of order, to make the risk of confusion minimal and not be inconsistent

That should be enough, I'm not sure why not

HolonProduction commented 2 weeks ago

Make it a warning

My main concern was that the user group most likely to be surprised by this are beginners, and also in my experience (based on what I read on various help channels e.g. discord) those are most likely to ignore/not know about warnings.

But sure let's make it a warning and improve warning discoverability. (I'll update the proposal)

Make it apply to any duplicate, no matter of order, to make the risk of confusion minimal and not be inconsistent

This has nothing to do with the warning vs error topic for me. Enum duplicates are an intended feature and I don't think we should warn about them.

The unintended (at least by the user) thing are implicit duplicates, we want to warn about those.

This requires some sort of classification, which is bound to have some inconsistencies, but as long as it gracefully handles the most common cases, this is fair tradeoff to make for me.

If we were working from scratch I would prefer a policy of guaranteeing that implicit values are unique (because it feels more like what is needed in the context in which GDScript is usually used). But that really is beyond the scope of this proposal.

AThousandShips commented 2 weeks ago

This has nothing to do with the warning vs error topic for me. Enum duplicates are an intended feature and I don't think we should warn about them.

I meant any implicit duplicate, as that was the topic at hand

HolonProduction commented 2 weeks ago

Well it comes down to what is an implicit duplicate.

enum Test1{
TEST1,
TEST2 = TEST1,
}

For me this isn't one. And if it were one users might very well disable the warning altogether since they don't want to add a lot of @warning_ignores.

AThousandShips commented 2 weeks ago

And if it were one users might very well disable the warning altogether since they don't want to add a lot of @warning_ignores.

Yes that's my point, there's little reliable way to make this work and not miss cases, that's what I've been saying

godotengine / godot-proposals