Rollup merge of rust-lang#100238 - Bryysen:master, r=cjgillot

Further improve error message for E0081

Closes rust-lang#97533

Author: matthiaskrgr

compiler/rustc_typeck/src/check/check.rs

@@ -32,7 +32,6 @@ use rustc_trait_selection::traits::error_reporting::InferCtxtExt as _;
 use rustc_trait_selection::traits::{self, ObligationCtxt};
 use rustc_ty_utils::representability::{self, Representability};
 
-use std::iter;
 use std::ops::ControlFlow;
 
 pub(super) fn check_abi(tcx: TyCtxt<'_>, hir_id: hir::HirId, span: Span, abi: Abi) {
@@ -1494,76 +1493,109 @@ fn check_enum<'tcx>(tcx: TyCtxt<'tcx>, vs: &'tcx [hir::Variant<'tcx>], def_id: L
         }
     }
 
-    let mut disr_vals: Vec<Discr<'tcx>> = Vec::with_capacity(vs.len());
-    // This tracks the previous variant span (in the loop) incase we need it for diagnostics
-    let mut prev_variant_span: Span = DUMMY_SP;
-    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 = format_discriminant_overflow(tcx, v, discr);
-            let display_discr_i = format_discriminant_overflow(tcx, variant_i, disr_vals[i]);
-            let no_disr = v.disr_expr.is_none();
-            let mut err = struct_span_err!(
-                tcx.sess,
-                sp,
-                E0081,
-                "discriminant value `{}` assigned more than once",
-                discr,
-            );
-
-            err.span_label(i_span, format!("first assignment of {display_discr_i}"));
-            err.span_label(span, format!("second assignment of {display_discr}"));
-
-            if no_disr {
-                err.span_label(
-                    prev_variant_span,
-                    format!(
-                        "assigned discriminant for `{}` was incremented from this discriminant",
-                        v.ident
-                    ),
-                );
-            }
-            err.emit();
-        }
-
-        disr_vals.push(discr);
-        prev_variant_span = v.span;
-    }
+    detect_discriminant_duplicate(tcx, def.discriminants(tcx).collect(), vs, sp);
 
     check_representable(tcx, sp, def_id);
     check_transparent(tcx, sp, def);
 }
 
-/// In the case that a discriminant is both a duplicate and an overflowing literal,
-/// we insert both the assigned discriminant and the literal it overflowed from into the formatted
-/// output. Otherwise we format the discriminant normally.
-fn format_discriminant_overflow<'tcx>(
+/// Part of enum check. Given the discriminants of an enum, errors if two or more discriminants are equal
+fn detect_discriminant_duplicate<'tcx>(
     tcx: TyCtxt<'tcx>,
-    variant: &hir::Variant<'_>,
-    dis: Discr<'tcx>,
-) -> String {
-    if let Some(expr) = &variant.disr_expr {
-        let body = &tcx.hir().body(expr.body).value;
-        if let hir::ExprKind::Lit(lit) = &body.kind
-            && let rustc_ast::LitKind::Int(lit_value, _int_kind) = &lit.node
-            && dis.val != *lit_value
-        {
-                    return format!("`{dis}` (overflowed from `{lit_value}`)");
+    mut discrs: Vec<(VariantIdx, Discr<'tcx>)>,
+    vs: &'tcx [hir::Variant<'tcx>],
+    self_span: Span,
+) {
+    // Helper closure to reduce duplicate code. This gets called everytime we detect a duplicate.
+    // Here `idx` refers to the order of which the discriminant appears, and its index in `vs`
+    let report = |dis: Discr<'tcx>,
+                  idx: usize,
+                  err: &mut DiagnosticBuilder<'_, ErrorGuaranteed>| {
+        let var = &vs[idx]; // HIR for the duplicate discriminant
+        let (span, display_discr) = match var.disr_expr {
+            Some(ref expr) => {
+                // In the case the discriminant is both a duplicate and overflowed, let the user know
+                if let hir::ExprKind::Lit(lit) = &tcx.hir().body(expr.body).value.kind
+                    && let rustc_ast::LitKind::Int(lit_value, _int_kind) = &lit.node
+                    && *lit_value != dis.val
+                {
+                    (tcx.hir().span(expr.hir_id), format!("`{dis}` (overflowed from `{lit_value}`)"))
+                // Otherwise, format the value as-is
+                } else {
+                    (tcx.hir().span(expr.hir_id), format!("`{dis}`"))
+                }
+            }
+            None => {
+                // At this point we know this discriminant is a duplicate, and was not explicitly
+                // assigned by the user. Here we iterate backwards to fetch the HIR for the last
+                // explictly assigned discriminant, and letting the user know that this was the
+                // increment startpoint, and how many steps from there leading to the duplicate
+                if let Some((n, hir::Variant { span, ident, .. })) =
+                    vs[..idx].iter().rev().enumerate().find(|v| v.1.disr_expr.is_some())
+                {
+                    let ve_ident = var.ident;
+                    let n = n + 1;
+                    let sp = if n > 1 { "variants" } else { "variant" };
+
+                    err.span_label(
+                        *span,
+                        format!("discriminant for `{ve_ident}` incremented from this startpoint (`{ident}` + {n} {sp} later => `{ve_ident}` = {dis})"),
+                    );
+                }
+
+                (vs[idx].span, format!("`{dis}`"))
+            }
+        };
+
+        err.span_label(span, format!("{display_discr} assigned here"));
+    };
+
+    // Here we loop through the discriminants, comparing each discriminant to another.
+    // When a duplicate is detected, we instatiate an error and point to both
+    // initial and duplicate value. The duplicate discriminant is then discarded by swapping
+    // it with the last element and decrementing the `vec.len` (which is why we have to evaluate
+    // `discrs.len()` anew every iteration, and why this could be tricky to do in a functional
+    // style as we are mutating `discrs` on the fly).
+    let mut i = 0;
+    while i < discrs.len() {
+        let hir_var_i_idx = discrs[i].0.index();
+        let mut error: Option<DiagnosticBuilder<'_, _>> = None;
+
+        let mut o = i + 1;
+        while o < discrs.len() {
+            let hir_var_o_idx = discrs[o].0.index();
+
+            if discrs[i].1.val == discrs[o].1.val {
+                let err = error.get_or_insert_with(|| {
+                    let mut ret = struct_span_err!(
+                        tcx.sess,
+                        self_span,
+                        E0081,
+                        "discriminant value `{}` assigned more than once",
+                        discrs[i].1,
+                    );
+
+                    report(discrs[i].1, hir_var_i_idx, &mut ret);
+
+                    ret
+                });
+
+                report(discrs[o].1, hir_var_o_idx, err);
+
+                // Safe to unwrap here, as we wouldn't reach this point if `discrs` was empty
+                discrs[o] = *discrs.last().unwrap();
+                discrs.pop();
+            } else {
+                o += 1;
+            }
         }
-    }
 
-    format!("`{dis}`")
+        if let Some(mut e) = error {
+            e.emit();
+        }
+
+        i += 1;
+    }
 }
 
 pub(super) fn check_type_params_are_used<'tcx>(

compiler/rustc_typeck/src/check/mod.rs

@@ -112,7 +112,6 @@ use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::intravisit::Visitor;
 use rustc_hir::{HirIdMap, ImplicitSelfKind, Node};
 use rustc_index::bit_set::BitSet;
-use rustc_index::vec::Idx;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::subst::{InternalSubsts, Subst, SubstsRef};

src/test/ui/enum/enum-discrim-autosizing.rs

@@ -6,9 +6,9 @@
 enum Eu64 {
     //~^ ERROR discriminant value `0` assigned more than once
     Au64 = 0,
-    //~^NOTE first assignment of `0`
+    //~^NOTE `0` assigned here
     Bu64 = 0x8000_0000_0000_0000
-    //~^NOTE second assignment of `0` (overflowed from `9223372036854775808`)
+    //~^NOTE `0` (overflowed from `9223372036854775808`) assigned here
 }
 
 fn main() {}

src/test/ui/enum/enum-discrim-autosizing.stderr

@@ -5,10 +5,10 @@ LL | enum Eu64 {
    | ^^^^^^^^^
 LL |
 LL |     Au64 = 0,
-   |            - first assignment of `0`
+   |            - `0` assigned here
 LL |
 LL |     Bu64 = 0x8000_0000_0000_0000
-   |            --------------------- second assignment of `0` (overflowed from `9223372036854775808`)
+   |            --------------------- `0` (overflowed from `9223372036854775808`) assigned here
 
 error: aborting due to previous error
 

src/test/ui/error-codes/E0081.rs

@@ -1,30 +1,53 @@
 enum Enum {
     //~^ ERROR discriminant value `3` assigned more than once
     P = 3,
-    //~^ NOTE first assignment of `3`
+    //~^ NOTE `3` assigned here
     X = 3,
-    //~^ NOTE second assignment of `3`
+    //~^ NOTE `3` assigned here
     Y = 5
 }
 
 #[repr(u8)]
 enum EnumOverflowRepr {
     //~^ ERROR discriminant value `1` assigned more than once
     P = 257,
-    //~^ NOTE first assignment of `1` (overflowed from `257`)
+    //~^ NOTE `1` (overflowed from `257`) assigned here
     X = 513,
-    //~^ NOTE second assignment of `1` (overflowed from `513`)
+    //~^ NOTE `1` (overflowed from `513`) assigned here
 }
 
 #[repr(i8)]
 enum NegDisEnum {
     //~^ ERROR discriminant value `-1` assigned more than once
     First = -1,
-    //~^ NOTE first assignment of `-1`
+    //~^ NOTE `-1` assigned here
     Second = -2,
-    //~^ NOTE assigned discriminant for `Last` was incremented from this discriminant
+    //~^ NOTE discriminant for `Last` incremented from this startpoint (`Second` + 1 variant later => `Last` = -1)
     Last,
-    //~^ NOTE second assignment of `-1`
+    //~^ NOTE `-1` assigned here
+}
+
+enum MultipleDuplicates {
+    //~^ ERROR discriminant value `0` assigned more than once
+    //~^^ ERROR discriminant value `-2` assigned more than once
+    V0,
+    //~^ NOTE `0` assigned here
+    V1 = 0,
+    //~^ NOTE `0` assigned here
+    V2,
+    V3,
+    V4 = 0,
+    //~^ NOTE `0` assigned here
+    V5 = -2,
+    //~^ NOTE discriminant for `V7` incremented from this startpoint (`V5` + 2 variants later => `V7` = 0)
+    //~^^ NOTE `-2` assigned here
+    V6,
+    V7,
+    //~^ NOTE `0` assigned here
+    V8 = -3,
+    //~^ NOTE discriminant for `V9` incremented from this startpoint (`V8` + 1 variant later => `V9` = -2)
+    V9,
+    //~^ NOTE `-2` assigned here
 }
 
 fn main() {

src/test/ui/error-codes/E0081.stderr

@@ -5,10 +5,10 @@ LL | enum Enum {
    | ^^^^^^^^^
 LL |
 LL |     P = 3,
-   |         - first assignment of `3`
+   |         - `3` assigned here
 LL |
 LL |     X = 3,
-   |         - second assignment of `3`
+   |         - `3` assigned here
 
 error[E0081]: discriminant value `1` assigned more than once
   --> $DIR/E0081.rs:11:1
@@ -17,10 +17,10 @@ LL | enum EnumOverflowRepr {
    | ^^^^^^^^^^^^^^^^^^^^^
 LL |
 LL |     P = 257,
-   |         --- first assignment of `1` (overflowed from `257`)
+   |         --- `1` (overflowed from `257`) assigned here
 LL |
 LL |     X = 513,
-   |         --- second assignment of `1` (overflowed from `513`)
+   |         --- `1` (overflowed from `513`) assigned here
 
 error[E0081]: discriminant value `-1` assigned more than once
   --> $DIR/E0081.rs:20:1
@@ -29,14 +29,50 @@ LL | enum NegDisEnum {
    | ^^^^^^^^^^^^^^^
 LL |
 LL |     First = -1,
-   |             -- first assignment of `-1`
+   |             -- `-1` assigned here
 LL |
 LL |     Second = -2,
-   |     ----------- assigned discriminant for `Last` was incremented from this discriminant
+   |     ----------- discriminant for `Last` incremented from this startpoint (`Second` + 1 variant later => `Last` = -1)
 LL |
 LL |     Last,
-   |     ---- second assignment of `-1`
+   |     ---- `-1` assigned here
 
-error: aborting due to 3 previous errors
+error[E0081]: discriminant value `0` assigned more than once
+  --> $DIR/E0081.rs:30:1
+   |
+LL | enum MultipleDuplicates {
+   | ^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     V0,
+   |     -- `0` assigned here
+LL |
+LL |     V1 = 0,
+   |          - `0` assigned here
+...
+LL |     V4 = 0,
+   |          - `0` assigned here
+LL |
+LL |     V5 = -2,
+   |     ------- discriminant for `V7` incremented from this startpoint (`V5` + 2 variants later => `V7` = 0)
+...
+LL |     V7,
+   |     -- `0` assigned here
+
+error[E0081]: discriminant value `-2` assigned more than once
+  --> $DIR/E0081.rs:30:1
+   |
+LL | enum MultipleDuplicates {
+   | ^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     V5 = -2,
+   |          -- `-2` assigned here
+...
+LL |     V8 = -3,
+   |     ------- discriminant for `V9` incremented from this startpoint (`V8` + 1 variant later => `V9` = -2)
+LL |
+LL |     V9,
+   |     -- `-2` assigned here
+
+error: aborting due to 5 previous errors
 
 For more information about this error, try `rustc --explain E0081`.