Confusing error message on negative enum-discriminants

[Bryysen]

Given the following code:

Playground

#[repr(i8)]
enum DisEnum {
    First = -1,
    Second = -2,
    Third = -3,
    Last,
}

The current output is:

error[E0081]: discriminant value `254` already exists
 --> src/lib.rs:8:5
  |
6 |     Second = -2,
  |              -- first use of `254`
7 |     Third = -3,
8 |     Last,
  |     ^^^^ enum already has `254`

For more information about this error, try `rustc --explain E0081`.
error: could not compile `playground` due to previous error
<rustc output>

Ideally the output should look like:

error[E0081]: discriminant value `254` already exists
 --> src/lib.rs:8:5
  |
6 |     Second = -2,
  |              -- first use of `-2`
7 |     Third = -3,
8 |     Last,
  |     ^^^^ enum already has `-2`

For more information about this error, try `rustc --explain E0081`.
error: could not compile `playground` due to previous error
<proposed output>

I don't have any understanding of the internal machinery of the diagnostics, but it seems that the value is being cast to the unsigned variant of the integer before displayed (in this case i8 -> u8). Of course, the discriminant does its job just fine, and explicitly casting DisEnum to an i8 also works as expected, only the diagnostics are a bit unfortunate.

Changing #[repr(i8)] to #[repr(isize)] in the example causes it to underflow close to usize::MAX which may seem a little intimidating to a newer user who accidentally stumbles on this 😄

The output is the same on both Stable and Nightly

[Bryysen]

Gave it some more thought, and I'm guessing #[repr(i8)] just evaluates to #[repr(u8)] (and vice-versa for the other integer types) under the hood, but due to consistent wrapping behavior and two's complement, there is no difference to the user since they have to explicitly cast the enum to the respective integer to read it anyway. Well except for this error message I guess.

[Bryysen]

So I got curious and decided to check out what causes this. Looking around a bit trying to find which part of type-checking had to do with enums led me to this function. At the end we there is a part where we go through all the discriminants and ensure none of them are duplicates, and if there are duplicates we emit the E0081 error pointing to the duplicates and all that.

The code

  let mut disr_vals: Vec<Discr<'tcx>> = Vec::with_capacity(vs.len());
  for ((_, discr), v) in iter::zip(def.discriminants(tcx), vs) {
      // Check for duplicate discriminant values
      if let Some(i) = disr_vals.iter().position(|&x| x.val == discr.val) {
          let variant_did = def.variant(VariantIdx::new(i)).def_id;
          let variant_i_hir_id = tcx.hir().local_def_id_to_hir_id(variant_did.expect_local());
          let variant_i = tcx.hir().expect_variant(variant_i_hir_id);
          let i_span = match variant_i.disr_expr {
              Some(ref expr) => tcx.hir().span(expr.hir_id),
              None => tcx.def_span(variant_did),
          };
          let span = match v.disr_expr {
              Some(ref expr) => tcx.hir().span(expr.hir_id),
              None => v.span,
          };
          let display_discr = display_discriminant_value(tcx, v, discr.val);
          let display_discr_i = display_discriminant_value(tcx, variant_i, disr_vals[i].val);
                                                                                                                  
          struct_span_err!(                                                  
              tcx.sess,                              
              span,                                                   
              E0081,
              "discriminant value `{}` already exists",
              discr.val,
          )
          .span_label(i_span, format!("first use of {display_discr_i}"))
          .span_label(span, format!("enum already has {display_discr}"))
          .emit();
      }
      disr_vals.push(discr);
  }

  check_representable(tcx, sp, def_id);
  check_transparent(tcx, sp, def);

However, the type of the discr is being tracked correctly with respect to any repr(integer). I added a print statement and sure enough:

Ty: Discr {
    val: 255,
    ty: i8,
}
error[E0081]: discriminant value `255` already exists
 --> tst.rs:5:5
  |
3 |     First = -1,
  |             -- first use of `255`
4 |     Second = -2,
5 |     Last
  |     ^^^^ enum already has `255`

error: aborting due to previous error

The type of the discriminant is understood to be i8, but the value is expressed as a u8. If I can find a nice solution to this i'll submit a PR.

[clarfonthey]

Honestly, I think this error could probably be reworded as well; since Last doesn't have an explicitly defined discriminant, we should also clarify that it's used by counting up from the previously defined discriminant, which here is Second = -2.

Of course, that doesn't have to be part of the first PR, but I figure it's worth mentioning here.

[Bryysen]

Yeah that would improve the error quality in this case. I haven't checked if we can infer from context whether the discriminant has been explicitly assigned or not, but I assume we can, and in that case whenever we detect a duplicate we could add a small note saying that that the implicit discriminant is just a += 1 from the previous discriminant as you say. Doesn't hurt to do that for both positive and negative discriminant duplicates.

Anyway I've got something in the works, I'll explore adding this note aswell as soon as I can get back to my computer with some spare time. @rustbot claim

[clarfonthey]

You should be able to assume that it's been previously assigned, since if every discriminant were inferred, there wouldn't be conflicts, unless the number of variants overflows the repr type (which I think has a separate error? I'm not actually sure)

And I think a good example to test here would be:

enum Sneaky {
    Three = 3,
    Two = 2,
    One,
}

Since the discriminant for One would be actually 3, and fail.

[Bryysen]

Oh, I meant inferring if whether the duplicate discriminant was explicitly set or not. In the above example, that would be detecting if Last was assigned explicitly or not. But yeah it was possible, HIR has all the information we need.

So here's what I currently have. An enum like this:

#[repr(i8)]                                                                                         
enum MyEnum {                                                   
    First = -1,    
    Second = -2,                                                               
    Third = -3,    
    Last,                                                                      
}

Errors as such:

error[E0081]: discriminant value `-2` assigned more than once
 --> tst.rs:6:5
  |
4 |     Second = -2,
  |              -- first assignment of `-2`
5 |     Third = -3,
6 |     Last
  |     ^^^^ second assignment of `-2`
  |
  = note: because `Last` has no explicit discriminant, it gets assigned the increment of the previous discriminant

error: aborting due to previous error

Not sure about the note. We could split it out with its own span and point to which variant the duplicate increment (Last in this case) is incremented from, but i feel like that is unnecessarily verbose. Thoughts?

Also I reworded some of the original error message, hopefully it's clearer this way? If not i can revert that.

Edit: Sorry @clarfonthey i didn't read your comment properly 😃 . Yeah i guess we could iterate backwards until we hit the first defined discriminant, and point to that in the note.

[clarfonthey]

Yeah, in that case I'd say something like: discriminant is computed as (specified) + N variants later = (duplicate)

[Bryysen]

Ok, I think I'll leave the notification for another PR 😞 . The reason is that in the duplicate-detection loop, we are only storing the Discr type as we go, and not the HIR representation of the variant. This means that (unless I'm missing something) we can't iterate backwards after we hit a duplicate with no assigned discriminant, to the last discriminant assigned explicitly.

As an example, for this enum:

enum Tricky {                                                                                
    V1 = 10,                                                  
    V2 = 7,                                                                  
    V3,                                                                                 
    V4,                                                                          
    V5,                                                                               
} 

In the loop, once we get to V5 we compare it to the previously approved discriminants, and detect that V5 is a duplicate of V1. We want to point to V2 as the "increment startpoint", but we've ditched the HIR information at this point for the approved discriminants (which V2 is part of). Ofcourse we could just loop through all of the variants all over with the HIR included, but that is really inelegant (and ineffecient). So what we should do is re-write the loop but I don't feel like messing with that right now.

[clarfonthey]

Yeah, I think it's totally reasonable to just fix the extremely obvious bug of reinterpreting the integer before tackling the larger issue of the message being weird to begin with. I mostly just wanted to add my thoughts here for a potential solution so anyone else looking to help with this has some ideas to work with.

[clarfonthey]

(Should I create a separate issue for my other suggestions, or would it make sense to reopen this issue until those errors are also fixed?)

[Bryysen]

It would make sense to me to make a new issue specifically for the stuff we missed from this PR? There are probably good reasons to just go through the entire check_enum function. Here's one of the small (possible) imrovements: the check in this loop could probably be done in this loop so that we don't have to do two passes?

[clarfonthey]

Alright, filed! Future discussion can go there.