Added type-specific overflow checks when computing enum discriminant values.

Moved such overflow checking into one place (in `rustc::middle::ty`,
since it needs to be run on-demand during `const_eval` in some
scenarios), and revised `rustc_typeck` accordingly.

(Note that we only check for overflow if program did not provide a
discriminant value explicitly.)

Fix #23030

Fix #23221

Fix #23235

src/librustc/diagnostics.rs

@@ -68,6 +68,8 @@ register_diagnostics! {
     E0019,
     E0020,
     E0022,
+    E0079, // enum variant: expected signed integer constant
+    E0080, // enum variant: constant evaluation error
     E0109,
     E0110,
     E0133,
@@ -128,7 +130,8 @@ register_diagnostics! {
     E0313, // lifetime of borrowed pointer outlives lifetime of captured variable
     E0314, // closure outlives stack frame
     E0315, // cannot invoke closure outside of its lifetime
-    E0316 // nested quantification of lifetimes
+    E0316, // nested quantification of lifetimes
+    E0370  // discriminant overflow
 }
 
 __build_diagnostic_array! { DIAGNOSTICS }

src/librustc/middle/ty.rs

@@ -73,6 +73,8 @@ use std::cmp;
 use std::fmt;
 use std::hash::{Hash, SipHasher, Hasher};
 use std::mem;
+use std::num::ToPrimitive;
+use std::num::wrapping::WrappingOps;
 use std::ops;
 use std::rc::Rc;
 use std::vec::IntoIter;
@@ -83,9 +85,11 @@ use syntax::ast::{CrateNum, DefId, Ident, ItemTrait, LOCAL_CRATE};
 use syntax::ast::{MutImmutable, MutMutable, Name, NamedField, NodeId};
 use syntax::ast::{StmtExpr, StmtSemi, StructField, UnnamedField, Visibility};
 use syntax::ast_util::{self, is_local, lit_is_str, local_def};
-use syntax::attr::{self, AttrMetaMethods};
+use syntax::attr::{self, AttrMetaMethods, SignedInt, UnsignedInt};
 use syntax::codemap::Span;
 use syntax::parse::token::{self, InternedString, special_idents};
+use syntax::print::pprust;
+use syntax::ptr::P;
 use syntax::{ast, ast_map};
 
 pub type Disr = u64;
@@ -5489,63 +5493,268 @@ pub fn type_is_empty(cx: &ctxt, ty: Ty) -> bool {
      }
 }
 
+trait IntTypeExt {
+    fn to_ty<'tcx>(&self, cx: &ctxt<'tcx>) -> Ty<'tcx>;
+    fn i64_to_disr(&self, val: i64) -> Option<Disr>;
+    fn u64_to_disr(&self, val: u64) -> Option<Disr>;
+    fn disr_incr(&self, val: Disr) -> Option<Disr>;
+    fn disr_string(&self, val: Disr) -> String;
+    fn disr_wrap_incr(&self, val: Option<Disr>) -> Disr;
+}
+
+impl IntTypeExt for attr::IntType {
+    fn to_ty<'tcx>(&self, cx: &ctxt<'tcx>) -> Ty<'tcx> {
+        match *self {
+            SignedInt(ast::TyI8)      => cx.types.i8,
+            SignedInt(ast::TyI16)     => cx.types.i16,
+            SignedInt(ast::TyI32)     => cx.types.i32,
+            SignedInt(ast::TyI64)     => cx.types.i64,
+            SignedInt(ast::TyIs)   => cx.types.int,
+            UnsignedInt(ast::TyU8)    => cx.types.u8,
+            UnsignedInt(ast::TyU16)   => cx.types.u16,
+            UnsignedInt(ast::TyU32)   => cx.types.u32,
+            UnsignedInt(ast::TyU64)   => cx.types.u64,
+            UnsignedInt(ast::TyUs) => cx.types.uint,
+        }
+    }
+
+    fn i64_to_disr(&self, val: i64) -> Option<Disr> {
+        match *self {
+            SignedInt(ast::TyI8)    => val.to_i8()  .map(|v| v as Disr),
+            SignedInt(ast::TyI16)   => val.to_i16() .map(|v| v as Disr),
+            SignedInt(ast::TyI32)   => val.to_i32() .map(|v| v as Disr),
+            SignedInt(ast::TyI64)   => val.to_i64() .map(|v| v as Disr),
+            UnsignedInt(ast::TyU8)  => val.to_u8()  .map(|v| v as Disr),
+            UnsignedInt(ast::TyU16) => val.to_u16() .map(|v| v as Disr),
+            UnsignedInt(ast::TyU32) => val.to_u32() .map(|v| v as Disr),
+            UnsignedInt(ast::TyU64) => val.to_u64() .map(|v| v as Disr),
+
+            UnsignedInt(ast::TyUs) |
+            SignedInt(ast::TyIs) => unreachable!(),
+        }
+    }
+
+    fn u64_to_disr(&self, val: u64) -> Option<Disr> {
+        match *self {
+            SignedInt(ast::TyI8)    => val.to_i8()  .map(|v| v as Disr),
+            SignedInt(ast::TyI16)   => val.to_i16() .map(|v| v as Disr),
+            SignedInt(ast::TyI32)   => val.to_i32() .map(|v| v as Disr),
+            SignedInt(ast::TyI64)   => val.to_i64() .map(|v| v as Disr),
+            UnsignedInt(ast::TyU8)  => val.to_u8()  .map(|v| v as Disr),
+            UnsignedInt(ast::TyU16) => val.to_u16() .map(|v| v as Disr),
+            UnsignedInt(ast::TyU32) => val.to_u32() .map(|v| v as Disr),
+            UnsignedInt(ast::TyU64) => val.to_u64() .map(|v| v as Disr),
+
+            UnsignedInt(ast::TyUs) |
+            SignedInt(ast::TyIs) => unreachable!(),
+        }
+    }
+
+    fn disr_incr(&self, val: Disr) -> Option<Disr> {
+        macro_rules! add1 {
+            ($e:expr) => { $e.and_then(|v|v.checked_add(1)).map(|v| v as Disr) }
+        }
+        match *self {
+            // SignedInt repr means we *want* to reinterpret the bits
+            // treating the highest bit of Disr as a sign-bit, so
+            // cast to i64 before range-checking.
+            SignedInt(ast::TyI8)    => add1!((val as i64).to_i8()),
+            SignedInt(ast::TyI16)   => add1!((val as i64).to_i16()),
+            SignedInt(ast::TyI32)   => add1!((val as i64).to_i32()),
+            SignedInt(ast::TyI64)   => add1!(Some(val as i64)),
+
+            UnsignedInt(ast::TyU8)  => add1!(val.to_u8()),
+            UnsignedInt(ast::TyU16) => add1!(val.to_u16()),
+            UnsignedInt(ast::TyU32) => add1!(val.to_u32()),
+            UnsignedInt(ast::TyU64) => add1!(Some(val)),
+
+            UnsignedInt(ast::TyUs) |
+            SignedInt(ast::TyIs) => unreachable!(),
+        }
+    }
+
+    // This returns a String because (1.) it is only used for
+    // rendering an error message and (2.) a string can represent the
+    // full range from `i64::MIN` through `u64::MAX`.
+    fn disr_string(&self, val: Disr) -> String {
+        match *self {
+            SignedInt(ast::TyI8)    => format!("{}", val as i8 ),
+            SignedInt(ast::TyI16)   => format!("{}", val as i16),
+            SignedInt(ast::TyI32)   => format!("{}", val as i32),
+            SignedInt(ast::TyI64)   => format!("{}", val as i64),
+            UnsignedInt(ast::TyU8)  => format!("{}", val as u8 ),
+            UnsignedInt(ast::TyU16) => format!("{}", val as u16),
+            UnsignedInt(ast::TyU32) => format!("{}", val as u32),
+            UnsignedInt(ast::TyU64) => format!("{}", val as u64),
+
+            UnsignedInt(ast::TyUs) |
+            SignedInt(ast::TyIs) => unreachable!(),
+        }
+    }
+
+    fn disr_wrap_incr(&self, val: Option<Disr>) -> Disr {
+        macro_rules! add1 {
+            ($e:expr) => { ($e).wrapping_add(1) as Disr }
+        }
+        let val = val.unwrap_or(ty::INITIAL_DISCRIMINANT_VALUE);
+        match *self {
+            SignedInt(ast::TyI8)    => add1!(val as i8 ),
+            SignedInt(ast::TyI16)   => add1!(val as i16),
+            SignedInt(ast::TyI32)   => add1!(val as i32),
+            SignedInt(ast::TyI64)   => add1!(val as i64),
+            UnsignedInt(ast::TyU8)  => add1!(val as u8 ),
+            UnsignedInt(ast::TyU16) => add1!(val as u16),
+            UnsignedInt(ast::TyU32) => add1!(val as u32),
+            UnsignedInt(ast::TyU64) => add1!(val as u64),
+
+            UnsignedInt(ast::TyUs) |
+            SignedInt(ast::TyIs) => unreachable!(),
+        }
+    }
+}
+
+/// Returns `(normalized_type, ty)`, where `normalized_type` is the
+/// IntType representation of one of {i64,i32,i16,i8,u64,u32,u16,u8},
+/// and `ty` is the original type (i.e. may include `isize` or
+/// `usize`).
+pub fn enum_repr_type<'tcx>(cx: &ctxt<'tcx>,
+                            opt_hint: Option<&attr::ReprAttr>)
+                            -> (attr::IntType, Ty<'tcx>)
+{
+    let repr_type = match opt_hint {
+        // Feed in the given type
+        Some(&attr::ReprInt(_, int_t)) => int_t,
+        // ... but provide sensible default if none provided
+        //
+        // NB. Historically `fn enum_variants` generate i64 here, while
+        // rustc_typeck::check would generate isize.
+        _ => SignedInt(ast::TyIs),
+    };
+
+    let repr_type_ty = repr_type.to_ty(cx);
+    let repr_type = match repr_type {
+        SignedInt(ast::TyIs) =>
+            SignedInt(cx.sess.target.int_type),
+        UnsignedInt(ast::TyUs) =>
+            UnsignedInt(cx.sess.target.uint_type),
+        other => other
+    };
+
+    (repr_type, repr_type_ty)
+}
+
+fn report_discrim_overflow(cx: &ctxt,
+                           variant_span: Span,
+                           variant_name: &str,
+                           repr_type: attr::IntType,
+                           prev_val: Disr) {
+    let computed_value = repr_type.disr_wrap_incr(Some(prev_val));
+    let computed_value = repr_type.disr_string(computed_value);
+    let prev_val = repr_type.disr_string(prev_val);
+    let repr_type = repr_type.to_ty(cx).user_string(cx);
+    span_err!(cx.sess, variant_span, E0370,
+              "enum discriminant overflowed on value after {}: {}; \
+               set explicitly via {} = {} if that is desired outcome",
+              prev_val, repr_type, variant_name, computed_value);
+}
+
+// This computes the discriminant values for the sequence of Variants
+// attached to a particular enum, taking into account the #[repr] (if
+// any) provided via the `opt_hint`.
+fn compute_enum_variants<'tcx>(cx: &ctxt<'tcx>,
+                               vs: &'tcx [P<ast::Variant>],
+                               opt_hint: Option<&attr::ReprAttr>)
+                               -> Vec<Rc<ty::VariantInfo<'tcx>>> {
+    let mut variants: Vec<Rc<ty::VariantInfo>> = Vec::new();
+    let mut prev_disr_val: Option<ty::Disr> = None;
+
+    let (repr_type, repr_type_ty) = ty::enum_repr_type(cx, opt_hint);
+
+    for v in vs {
+        // If the discriminant value is specified explicitly in the
+        // enum, check whether the initialization expression is valid,
+        // otherwise use the last value plus one.
+        let current_disr_val;
+
+        // This closure marks cases where, when an error occurs during
+        // the computation, attempt to assign a (hopefully) fresh
+        // value to avoid spurious error reports downstream.
+        let attempt_fresh_value = move || -> Disr {
+            repr_type.disr_wrap_incr(prev_disr_val)
+        };
+
+        match v.node.disr_expr {
+            Some(ref e) => {
+                debug!("disr expr, checking {}", pprust::expr_to_string(&**e));
+
+                // check_expr (from check_const pass) doesn't guarantee
+                // that the expression is in a form that eval_const_expr can
+                // handle, so we may still get an internal compiler error
+                //
+                // pnkfelix: The above comment was transcribed from
+                // the version of this code taken from rustc_typeck.
+                // Presumably the implication is that we need to deal
+                // with such ICE's as they arise.
+                //
+                // Since this can be called from `ty::enum_variants`
+                // anyway, best thing is to make `eval_const_expr`
+                // more robust (on case-by-case basis).
+
+                match const_eval::eval_const_expr_partial(cx, &**e, Some(repr_type_ty)) {
+                    Ok(const_eval::const_int(val)) => current_disr_val = val as Disr,
+                    Ok(const_eval::const_uint(val)) => current_disr_val = val as Disr,
+                    Ok(_) => {
+                        span_err!(cx.sess, e.span, E0079,
+                                  "expected signed integer constant");
+                        current_disr_val = attempt_fresh_value();
+                    }
+                    Err(ref err) => {
+                        span_err!(cx.sess, err.span, E0080,
+                                  "constant evaluation error: {}",
+                                  err.description());
+                        current_disr_val = attempt_fresh_value();
+                    }
+                }
+            },
+            None => {
+                current_disr_val = match prev_disr_val {
+                    Some(prev_disr_val) => {
+                        if let Some(v) = repr_type.disr_incr(prev_disr_val) {
+                            v
+                        } else {
+                            report_discrim_overflow(cx, v.span, v.node.name.as_str(),
+                                                    repr_type, prev_disr_val);
+                            attempt_fresh_value()
+                        }
+                    }
+                    None => ty::INITIAL_DISCRIMINANT_VALUE
+                }
+            }
+        }
+
+        let variant_info = Rc::new(VariantInfo::from_ast_variant(cx, &**v, current_disr_val));
+        prev_disr_val = Some(current_disr_val);
+
+        variants.push(variant_info);
+    }
+
+    return variants;
+}
+
 pub fn enum_variants<'tcx>(cx: &ctxt<'tcx>, id: ast::DefId)
                            -> Rc<Vec<Rc<VariantInfo<'tcx>>>> {
     memoized(&cx.enum_var_cache, id, |id: ast::DefId| {
         if ast::LOCAL_CRATE != id.krate {
             Rc::new(csearch::get_enum_variants(cx, id))
         } else {
-            /*
-              Although both this code and check_enum_variants in typeck/check
-              call eval_const_expr, it should never get called twice for the same
-              expr, since check_enum_variants also updates the enum_var_cache
-             */
             match cx.map.get(id.node) {
                 ast_map::NodeItem(ref item) => {
                     match item.node {
                         ast::ItemEnum(ref enum_definition, _) => {
-                            let mut last_discriminant: Option<Disr> = None;
-                            Rc::new(enum_definition.variants.iter().map(|variant| {
-
-                                let mut discriminant = INITIAL_DISCRIMINANT_VALUE;
-                                if let Some(ref e) = variant.node.disr_expr {
-                                    // Preserve all values, and prefer signed.
-                                    let ty = Some(cx.types.i64);
-                                    match const_eval::eval_const_expr_partial(cx, &**e, ty) {
-                                        Ok(const_eval::const_int(val)) => {
-                                            discriminant = val as Disr;
-                                        }
-                                        Ok(const_eval::const_uint(val)) => {
-                                            discriminant = val as Disr;
-                                        }
-                                        Ok(_) => {
-                                            span_err!(cx.sess, e.span, E0304,
-                                                      "expected signed integer constant");
-                                        }
-                                        Err(err) => {
-                                            span_err!(cx.sess, err.span, E0305,
-                                                      "constant evaluation error: {}",
-                                                      err.description());
-                                        }
-                                    }
-                                } else {
-                                    if let Some(val) = last_discriminant {
-                                        if let Some(v) = val.checked_add(1) {
-                                            discriminant = v
-                                        } else {
-                                            cx.sess.span_err(
-                                                variant.span,
-                                                &format!("Discriminant overflowed!"));
-                                        }
-                                    } else {
-                                        discriminant = INITIAL_DISCRIMINANT_VALUE;
-                                    }
-                                }
-
-                                last_discriminant = Some(discriminant);
-                                Rc::new(VariantInfo::from_ast_variant(cx, &**variant,
-                                                                      discriminant))
-                            }).collect())
+                            Rc::new(compute_enum_variants(
+                                cx,
+                                &enum_definition.variants,
+                                lookup_repr_hints(cx, id).get(0)))
                         }
                         _ => {
                             cx.sess.bug("enum_variants: id not bound to an enum")

src/librustc_typeck/check/mod.rs

@@ -85,7 +85,7 @@ use astconv::{self, ast_region_to_region, ast_ty_to_ty, AstConv, PathParamMode};
 use check::_match::pat_ctxt;
 use fmt_macros::{Parser, Piece, Position};
 use middle::astconv_util::{check_path_args, NO_TPS, NO_REGIONS};
-use middle::{const_eval, def};
+use middle::def;
 use middle::infer;
 use middle::mem_categorization as mc;
 use middle::mem_categorization::McResult;
@@ -94,7 +94,7 @@ use middle::privacy::{AllPublic, LastMod};
 use middle::region::{self, CodeExtent};
 use middle::subst::{self, Subst, Substs, VecPerParamSpace, ParamSpace, TypeSpace};
 use middle::traits;
-use middle::ty::{FnSig, GenericPredicates, VariantInfo, TypeScheme};
+use middle::ty::{FnSig, GenericPredicates, TypeScheme};
 use middle::ty::{Disr, ParamTy, ParameterEnvironment};
 use middle::ty::{self, HasProjectionTypes, RegionEscape, ToPolyTraitRef, Ty};
 use middle::ty::liberate_late_bound_regions;
@@ -4283,68 +4283,30 @@ pub fn check_enum_variants<'a,'tcx>(ccx: &CrateCtxt<'a,'tcx>,
     fn do_check<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>,
                           vs: &'tcx [P<ast::Variant>],
                           id: ast::NodeId,
-                          hint: attr::ReprAttr)
-                          -> Vec<Rc<ty::VariantInfo<'tcx>>> {
+                          hint: attr::ReprAttr) {
         #![allow(trivial_numeric_casts)]
 
         let rty = ty::node_id_to_type(ccx.tcx, id);
-        let mut variants: Vec<Rc<ty::VariantInfo>> = Vec::new();
         let mut disr_vals: Vec<ty::Disr> = Vec::new();
-        let mut prev_disr_val: Option<ty::Disr> = None;
 
+        let inh = static_inherited_fields(ccx);
+        let fcx = blank_fn_ctxt(ccx, &inh, ty::FnConverging(rty), id);
+
+        let (_, repr_type_ty) = ty::enum_repr_type(ccx.tcx, Some(&hint));
         for v in vs {
+            if let Some(ref e) = v.node.disr_expr {
+                check_const_with_ty(&fcx, e.span, e, repr_type_ty);
+            }
+        }
 
-            // If the discriminant value is specified explicitly in the enum check whether the
-            // initialization expression is valid, otherwise use the last value plus one.
-            let mut current_disr_val = match prev_disr_val {
-                Some(prev_disr_val) => {
-                    if let Some(v) = prev_disr_val.checked_add(1) {
-                        v
-                    } else {
-                        ty::INITIAL_DISCRIMINANT_VALUE
-                    }
-                }
-                None => ty::INITIAL_DISCRIMINANT_VALUE
-            };
+        let def_id = local_def(id);
 
-            match v.node.disr_expr {
-                Some(ref e) => {
-                    debug!("disr expr, checking {}", pprust::expr_to_string(&**e));
+        // ty::enum_variants guards against discriminant overflows, so
+        // we need not check for that.
+        let variants = ty::enum_variants(ccx.tcx, def_id);
 
-                    let inh = static_inherited_fields(ccx);
-                    let fcx = blank_fn_ctxt(ccx, &inh, ty::FnConverging(rty), e.id);
-                    let declty = match hint {
-                        attr::ReprAny | attr::ReprPacked |
-                        attr::ReprExtern => fcx.tcx().types.isize,
-
-                        attr::ReprInt(_, attr::SignedInt(ity)) => {
-                            ty::mk_mach_int(fcx.tcx(), ity)
-                        }
-                        attr::ReprInt(_, attr::UnsignedInt(ity)) => {
-                            ty::mk_mach_uint(fcx.tcx(), ity)
-                        },
-                    };
-                    check_const_with_ty(&fcx, e.span, &**e, declty);
-                    // check_expr (from check_const pass) doesn't guarantee
-                    // that the expression is in a form that eval_const_expr can
-                    // handle, so we may still get an internal compiler error
-
-                    match const_eval::eval_const_expr_partial(ccx.tcx, &**e, Some(declty)) {
-                        Ok(const_eval::const_int(val)) => current_disr_val = val as Disr,
-                        Ok(const_eval::const_uint(val)) => current_disr_val = val as Disr,
-                        Ok(_) => {
-                            span_err!(ccx.tcx.sess, e.span, E0079,
-                                "expected signed integer constant");
-                        }
-                        Err(ref err) => {
-                            span_err!(ccx.tcx.sess, err.span, E0080,
-                                      "constant evaluation error: {}",
-                                      err.description());
-                        }
-                    }
-                },
-                None => ()
-            };
+        for (v, variant) in vs.iter().zip(variants.iter()) {
+            let current_disr_val = variant.disr_val;
 
             // Check for duplicate discriminant values
             match disr_vals.iter().position(|&x| x == current_disr_val) {
@@ -4372,15 +4334,7 @@ pub fn check_enum_variants<'a,'tcx>(ccx: &CrateCtxt<'a,'tcx>,
                 }
             }
             disr_vals.push(current_disr_val);
-
-            let variant_info = Rc::new(VariantInfo::from_ast_variant(ccx.tcx, &**v,
-                                                                     current_disr_val));
-            prev_disr_val = Some(current_disr_val);
-
-            variants.push(variant_info);
         }
-
-        return variants;
     }
 
     let hint = *ty::lookup_repr_hints(ccx.tcx, ast::DefId { krate: ast::LOCAL_CRATE, node: id })
@@ -4396,10 +4350,7 @@ pub fn check_enum_variants<'a,'tcx>(ccx: &CrateCtxt<'a,'tcx>,
         };
     }
 
-    let variants = do_check(ccx, vs, id, hint);
-
-    // cache so that ty::enum_variants won't repeat this work
-    ccx.tcx.enum_var_cache.borrow_mut().insert(local_def(id), Rc::new(variants));
+    do_check(ccx, vs, id, hint);
 
     check_representable(ccx.tcx, sp, id, "enum");
 

src/librustc_typeck/diagnostics.rs

@@ -51,8 +51,6 @@ register_diagnostics! {
     E0075,
     E0076,
     E0077,
-    E0079,
-    E0080,
     E0081,
     E0082,
     E0083,