From faf73028c9afe5790e9b4d83a348d93e4a098852 Mon Sep 17 00:00:00 2001 From: Sean Patrick Santos Date: Mon, 22 Jun 2015 20:55:57 -0600 Subject: [PATCH 1/2] Fix issue #23302, ICE on recursively defined enum variant discriminant. --- src/librustc/diagnostics.rs | 1 - src/librustc/middle/check_static_recursion.rs | 170 ++++++++++++++---- src/libsyntax/visit.rs | 7 +- src/test/compile-fail/issue-23302.rs | 20 +++ 4 files changed, 162 insertions(+), 36 deletions(-) create mode 100644 src/test/compile-fail/issue-23302.rs diff --git a/src/librustc/diagnostics.rs b/src/librustc/diagnostics.rs index 0f83cdff5372c..e2b4dc4175db6 100644 --- a/src/librustc/diagnostics.rs +++ b/src/librustc/diagnostics.rs @@ -1012,7 +1012,6 @@ register_diagnostics! { E0138, E0139, E0264, // unknown external lang item - E0266, // expected item E0269, // not all control paths return a value E0270, // computation may converge in a function marked as diverging E0272, // rustc_on_unimplemented attribute refers to non-existent type parameter diff --git a/src/librustc/middle/check_static_recursion.rs b/src/librustc/middle/check_static_recursion.rs index 41beabc9588d2..1d0cfe508c7c2 100644 --- a/src/librustc/middle/check_static_recursion.rs +++ b/src/librustc/middle/check_static_recursion.rs @@ -13,68 +13,86 @@ use ast_map; use session::Session; -use middle::def::{DefStatic, DefConst, DefAssociatedConst, DefMap}; +use middle::def::{DefStatic, DefConst, DefAssociatedConst, DefVariant, DefMap}; +use util::nodemap::NodeMap; use syntax::{ast, ast_util}; use syntax::codemap::Span; use syntax::visit::Visitor; use syntax::visit; +use std::cell::RefCell; + struct CheckCrateVisitor<'a, 'ast: 'a> { sess: &'a Session, def_map: &'a DefMap, - ast_map: &'a ast_map::Map<'ast> + ast_map: &'a ast_map::Map<'ast>, + discriminant_map: RefCell>>, } -impl<'v, 'a, 'ast> Visitor<'v> for CheckCrateVisitor<'a, 'ast> { - fn visit_item(&mut self, it: &ast::Item) { +impl<'a, 'ast: 'a> Visitor<'ast> for CheckCrateVisitor<'a, 'ast> { + fn visit_item(&mut self, it: &'ast ast::Item) { match it.node { - ast::ItemStatic(_, _, ref expr) | - ast::ItemConst(_, ref expr) => { + ast::ItemStatic(..) | + ast::ItemConst(..) => { let mut recursion_visitor = CheckItemRecursionVisitor::new(self, &it.span); recursion_visitor.visit_item(it); - visit::walk_expr(self, &*expr) }, - _ => visit::walk_item(self, it) + ast::ItemEnum(ref enum_def, ref generics) => { + // We could process the whole enum, but handling the variants + // with discriminant expressions one by one gives more specific, + // less redundant output. + for variant in &enum_def.variants { + if let Some(_) = variant.node.disr_expr { + let mut recursion_visitor = + CheckItemRecursionVisitor::new(self, &variant.span); + recursion_visitor.populate_enum_discriminants(enum_def); + recursion_visitor.visit_variant(variant, generics); + } + } + } + _ => {} } + visit::walk_item(self, it) } - fn visit_trait_item(&mut self, ti: &ast::TraitItem) { + fn visit_trait_item(&mut self, ti: &'ast ast::TraitItem) { match ti.node { ast::ConstTraitItem(_, ref default) => { - if let Some(ref expr) = *default { + if let Some(_) = *default { let mut recursion_visitor = CheckItemRecursionVisitor::new(self, &ti.span); recursion_visitor.visit_trait_item(ti); - visit::walk_expr(self, &*expr) } } - _ => visit::walk_trait_item(self, ti) + _ => {} } + visit::walk_trait_item(self, ti) } - fn visit_impl_item(&mut self, ii: &ast::ImplItem) { + fn visit_impl_item(&mut self, ii: &'ast ast::ImplItem) { match ii.node { - ast::ConstImplItem(_, ref expr) => { + ast::ConstImplItem(..) => { let mut recursion_visitor = CheckItemRecursionVisitor::new(self, &ii.span); recursion_visitor.visit_impl_item(ii); - visit::walk_expr(self, &*expr) } - _ => visit::walk_impl_item(self, ii) + _ => {} } + visit::walk_impl_item(self, ii) } } pub fn check_crate<'ast>(sess: &Session, - krate: &ast::Crate, + krate: &'ast ast::Crate, def_map: &DefMap, ast_map: &ast_map::Map<'ast>) { let mut visitor = CheckCrateVisitor { sess: sess, def_map: def_map, - ast_map: ast_map + ast_map: ast_map, + discriminant_map: RefCell::new(NodeMap()), }; visit::walk_crate(&mut visitor, krate); sess.abort_if_errors(); @@ -85,23 +103,25 @@ struct CheckItemRecursionVisitor<'a, 'ast: 'a> { sess: &'a Session, ast_map: &'a ast_map::Map<'ast>, def_map: &'a DefMap, - idstack: Vec + discriminant_map: &'a RefCell>>, + idstack: Vec, } impl<'a, 'ast: 'a> CheckItemRecursionVisitor<'a, 'ast> { - fn new(v: &CheckCrateVisitor<'a, 'ast>, span: &'a Span) + fn new(v: &'a CheckCrateVisitor<'a, 'ast>, span: &'a Span) -> CheckItemRecursionVisitor<'a, 'ast> { CheckItemRecursionVisitor { root_span: span, sess: v.sess, ast_map: v.ast_map, def_map: v.def_map, - idstack: Vec::new() + discriminant_map: &v.discriminant_map, + idstack: Vec::new(), } } fn with_item_id_pushed(&mut self, id: ast::NodeId, f: F) where F: Fn(&mut Self) { - if self.idstack.iter().any(|x| x == &(id)) { + if self.idstack.iter().any(|x| *x == id) { span_err!(self.sess, *self.root_span, E0265, "recursive constant"); return; } @@ -109,29 +129,94 @@ impl<'a, 'ast: 'a> CheckItemRecursionVisitor<'a, 'ast> { f(self); self.idstack.pop(); } + // If a variant has an expression specifying its discriminant, then it needs + // to be checked just like a static or constant. However, if there are more + // variants with no explicitly specified discriminant, those variants will + // increment the same expression to get their values. + // + // So for every variant, we need to track whether there is an expression + // somewhere in the enum definition that controls its discriminant. We do + // this by starting from the end and searching backward. + fn populate_enum_discriminants(&self, enum_definition: &'ast ast::EnumDef) { + // Get the map, and return if we already processed this enum or if it + // has no variants. + let mut discriminant_map = self.discriminant_map.borrow_mut(); + match enum_definition.variants.first() { + None => { return; } + Some(variant) if discriminant_map.contains_key(&variant.node.id) => { + return; + } + _ => {} + } + + // Go through all the variants. + let mut variant_stack: Vec = Vec::new(); + for variant in enum_definition.variants.iter().rev() { + variant_stack.push(variant.node.id); + // When we find an expression, every variant currently on the stack + // is affected by that expression. + if let Some(ref expr) = variant.node.disr_expr { + for id in &variant_stack { + discriminant_map.insert(*id, Some(expr)); + } + variant_stack.clear() + } + } + // If we are at the top, that always starts at 0, so any variant on the + // stack has a default value and does not need to be checked. + for id in &variant_stack { + discriminant_map.insert(*id, None); + } + } } -impl<'a, 'ast, 'v> Visitor<'v> for CheckItemRecursionVisitor<'a, 'ast> { - fn visit_item(&mut self, it: &ast::Item) { +impl<'a, 'ast: 'a> Visitor<'ast> for CheckItemRecursionVisitor<'a, 'ast> { + fn visit_item(&mut self, it: &'ast ast::Item) { self.with_item_id_pushed(it.id, |v| visit::walk_item(v, it)); } - fn visit_trait_item(&mut self, ti: &ast::TraitItem) { + fn visit_enum_def(&mut self, enum_definition: &'ast ast::EnumDef, + generics: &'ast ast::Generics) { + self.populate_enum_discriminants(enum_definition); + visit::walk_enum_def(self, enum_definition, generics); + } + + fn visit_variant(&mut self, variant: &'ast ast::Variant, + _: &'ast ast::Generics) { + let variant_id = variant.node.id; + let maybe_expr; + if let Some(get_expr) = self.discriminant_map.borrow().get(&variant_id) { + // This is necessary because we need to let the `discriminant_map` + // borrow fall out of scope, so that we can reborrow farther down. + maybe_expr = (*get_expr).clone(); + } else { + self.sess.span_bug(variant.span, + "`check_static_recursion` attempted to visit \ + variant with unknown discriminant") + } + // If `maybe_expr` is `None`, that's because no discriminant is + // specified that affects this variant. Thus, no risk of recursion. + if let Some(expr) = maybe_expr { + self.with_item_id_pushed(expr.id, |v| visit::walk_expr(v, expr)); + } + } + + fn visit_trait_item(&mut self, ti: &'ast ast::TraitItem) { self.with_item_id_pushed(ti.id, |v| visit::walk_trait_item(v, ti)); } - fn visit_impl_item(&mut self, ii: &ast::ImplItem) { + fn visit_impl_item(&mut self, ii: &'ast ast::ImplItem) { self.with_item_id_pushed(ii.id, |v| visit::walk_impl_item(v, ii)); } - fn visit_expr(&mut self, e: &ast::Expr) { + fn visit_expr(&mut self, e: &'ast ast::Expr) { match e.node { ast::ExprPath(..) => { match self.def_map.borrow().get(&e.id).map(|d| d.base_def) { Some(DefStatic(def_id, _)) | Some(DefAssociatedConst(def_id, _)) | - Some(DefConst(def_id)) if - ast_util::is_local(def_id) => { + Some(DefConst(def_id)) + if ast_util::is_local(def_id) => { match self.ast_map.get(def_id.node) { ast_map::NodeItem(item) => self.visit_item(item), @@ -141,11 +226,28 @@ impl<'a, 'ast, 'v> Visitor<'v> for CheckItemRecursionVisitor<'a, 'ast> { self.visit_impl_item(item), ast_map::NodeForeignItem(_) => {}, _ => { - span_err!(self.sess, e.span, E0266, - "expected item, found {}", - self.ast_map.node_to_string(def_id.node)); - return; - }, + self.sess.span_bug( + e.span, + &format!("expected item, found {}", + self.ast_map.node_to_string(def_id.node))); + } + } + } + // For variants, we only want to check expressions that + // affect the specific variant used, but we need to check + // the whole enum definition to see what expression that + // might be (if any). + Some(DefVariant(enum_id, variant_id, false)) + if ast_util::is_local(enum_id) => { + if let ast::ItemEnum(ref enum_def, ref generics) = + self.ast_map.expect_item(enum_id.local_id()).node { + self.populate_enum_discriminants(enum_def); + let variant = self.ast_map.expect_variant(variant_id.local_id()); + self.visit_variant(variant, generics); + } else { + self.sess.span_bug(e.span, + "`check_static_recursion` found \ + non-enum in DefVariant"); } } _ => () diff --git a/src/libsyntax/visit.rs b/src/libsyntax/visit.rs index 710928a00c11d..649052d123c88 100644 --- a/src/libsyntax/visit.rs +++ b/src/libsyntax/visit.rs @@ -90,6 +90,11 @@ pub trait Visitor<'v> : Sized { walk_struct_def(self, s) } fn visit_struct_field(&mut self, s: &'v StructField) { walk_struct_field(self, s) } + fn visit_enum_def(&mut self, enum_definition: &'v EnumDef, + generics: &'v Generics) { + walk_enum_def(self, enum_definition, generics) + } + fn visit_variant(&mut self, v: &'v Variant, g: &'v Generics) { walk_variant(self, v, g) } /// Visits an optional reference to a lifetime. The `span` is the span of some surrounding @@ -268,7 +273,7 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item) { } ItemEnum(ref enum_definition, ref type_parameters) => { visitor.visit_generics(type_parameters); - walk_enum_def(visitor, enum_definition, type_parameters) + visitor.visit_enum_def(enum_definition, type_parameters) } ItemDefaultImpl(_, ref trait_ref) => { visitor.visit_trait_ref(trait_ref) diff --git a/src/test/compile-fail/issue-23302.rs b/src/test/compile-fail/issue-23302.rs new file mode 100644 index 0000000000000..62f82ecae4409 --- /dev/null +++ b/src/test/compile-fail/issue-23302.rs @@ -0,0 +1,20 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +enum X { + A = X::A as isize, //~ ERROR E0265 +} + +enum Y { + A = Y::B as isize, //~ ERROR E0265 + B, +} + +fn main() { } From b952c0e4e9f3c169c6647fd05fef68ae1119865a Mon Sep 17 00:00:00 2001 From: Sean Patrick Santos Date: Wed, 8 Jul 2015 20:51:47 -0600 Subject: [PATCH 2/2] Add comments about the checks for recursive variant definition, as requested by @nrc. --- src/librustc/middle/check_static_recursion.rs | 4 ++++ src/test/compile-fail/issue-23302.rs | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/librustc/middle/check_static_recursion.rs b/src/librustc/middle/check_static_recursion.rs index 1d0cfe508c7c2..312a4708e8dde 100644 --- a/src/librustc/middle/check_static_recursion.rs +++ b/src/librustc/middle/check_static_recursion.rs @@ -27,6 +27,10 @@ struct CheckCrateVisitor<'a, 'ast: 'a> { sess: &'a Session, def_map: &'a DefMap, ast_map: &'a ast_map::Map<'ast>, + // `discriminant_map` is a cache that associates the `NodeId`s of local + // variant definitions with the discriminant expression that applies to + // each one. If the variant uses the default values (starting from `0`), + // then `None` is stored. discriminant_map: RefCell>>, } diff --git a/src/test/compile-fail/issue-23302.rs b/src/test/compile-fail/issue-23302.rs index 62f82ecae4409..7ac8cf45edbef 100644 --- a/src/test/compile-fail/issue-23302.rs +++ b/src/test/compile-fail/issue-23302.rs @@ -8,10 +8,14 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Check that an enum with recursion in the discriminant throws +// the appropriate error (rather than, say, blowing the stack). enum X { A = X::A as isize, //~ ERROR E0265 } +// Since `Y::B` here defaults to `Y::A+1`, this is also a +// recursive definition. enum Y { A = Y::B as isize, //~ ERROR E0265 B,