From 4e04d57efa665ae62a2af4c2d785809f15c17599 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 27 Mar 2015 01:43:14 +0100 Subject: [PATCH] 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 | 5 +- src/librustc/middle/ty.rs | 305 ++++++++++++++++++++++++----- src/librustc_typeck/check/mod.rs | 85 ++------ src/librustc_typeck/diagnostics.rs | 2 - 4 files changed, 279 insertions(+), 118 deletions(-) diff --git a/src/librustc/diagnostics.rs b/src/librustc/diagnostics.rs index 70303bb3410b7..0a29ed90ad461 100644 --- a/src/librustc/diagnostics.rs +++ b/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 } diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 161fae11ea6d4..c40f31d837a90 100644 --- a/src/librustc/middle/ty.rs +++ b/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; + fn u64_to_disr(&self, val: u64) -> Option; + fn disr_incr(&self, val: Disr) -> Option; + fn disr_string(&self, val: Disr) -> String; + fn disr_wrap_incr(&self, val: Option) -> 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 { + 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 { + 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 { + 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 { + 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], + opt_hint: Option<&attr::ReprAttr>) + -> Vec>> { + let mut variants: Vec> = Vec::new(); + let mut prev_disr_val: Option = 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>>> { 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 = 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") diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 16501ec280791..fbff4e8478882 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/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], id: ast::NodeId, - hint: attr::ReprAttr) - -> Vec>> { + hint: attr::ReprAttr) { #![allow(trivial_numeric_casts)] let rty = ty::node_id_to_type(ccx.tcx, id); - let mut variants: Vec> = Vec::new(); let mut disr_vals: Vec = Vec::new(); - let mut prev_disr_val: Option = None; - for v in vs { + let inh = static_inherited_fields(ccx); + let fcx = blank_fn_ctxt(ccx, &inh, ty::FnConverging(rty), id); - // 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 (_, 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); + } + } - match v.node.disr_expr { - Some(ref e) => { - debug!("disr expr, checking {}", pprust::expr_to_string(&**e)); + let def_id = local_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, + // ty::enum_variants guards against discriminant overflows, so + // we need not check for that. + let variants = ty::enum_variants(ccx.tcx, def_id); - 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"); diff --git a/src/librustc_typeck/diagnostics.rs b/src/librustc_typeck/diagnostics.rs index 7d01bece01cb8..a8d93c8bd111a 100644 --- a/src/librustc_typeck/diagnostics.rs +++ b/src/librustc_typeck/diagnostics.rs @@ -51,8 +51,6 @@ register_diagnostics! { E0075, E0076, E0077, - E0079, - E0080, E0081, E0082, E0083,