Auto merge of #38093 - mikhail-m1:stack-overflow, r=arielb1

fix stack overflow by enum and cont issue #36163

some paths were skipped while checking for recursion.

I fixed bug reproduces on win64 cargo test. In previous PR #36458 time complexity was exponential in case of linked const values. Now it's linear.

r? @alexcrichton

Author: bors

src/librustc_passes/static_recursion.rs

@@ -15,24 +15,23 @@ use rustc::dep_graph::DepNode;
 use rustc::hir::map as ast_map;
 use rustc::session::{CompileResult, Session};
 use rustc::hir::def::{Def, CtorKind};
-use rustc::util::nodemap::NodeMap;
+use rustc::util::nodemap::{NodeMap, NodeSet};
 
 use syntax::ast;
 use syntax::feature_gate::{GateIssue, emit_feature_err};
 use syntax_pos::Span;
 use rustc::hir::intravisit::{self, Visitor, NestedVisitorMap};
 use rustc::hir;
 
-use std::cell::RefCell;
-
 struct CheckCrateVisitor<'a, 'ast: 'a> {
     sess: &'a Session,
     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<NodeMap<Option<&'ast hir::Expr>>>,
+    discriminant_map: NodeMap<Option<&'ast hir::Expr>>,
+    detected_recursive_ids: NodeSet,
 }
 
 impl<'a, 'ast: 'a> Visitor<'ast> for CheckCrateVisitor<'a, 'ast> {
@@ -90,46 +89,49 @@ impl<'a, 'ast: 'a> Visitor<'ast> for CheckCrateVisitor<'a, 'ast> {
     }
 }
 
-pub fn check_crate<'ast>(sess: &Session,
-                         ast_map: &ast_map::Map<'ast>)
-                         -> CompileResult {
+pub fn check_crate<'ast>(sess: &Session, ast_map: &ast_map::Map<'ast>) -> CompileResult {
     let _task = ast_map.dep_graph.in_task(DepNode::CheckStaticRecursion);
 
     let mut visitor = CheckCrateVisitor {
         sess: sess,
         ast_map: ast_map,
-        discriminant_map: RefCell::new(NodeMap()),
+        discriminant_map: NodeMap(),
+        detected_recursive_ids: NodeSet(),
     };
     sess.track_errors(|| {
         // FIXME(#37712) could use ItemLikeVisitor if trait items were item-like
         ast_map.krate().visit_all_item_likes(&mut visitor.as_deep_visitor());
     })
 }
 
-struct CheckItemRecursionVisitor<'a, 'ast: 'a> {
-    root_span: &'a Span,
-    sess: &'a Session,
-    ast_map: &'a ast_map::Map<'ast>,
-    discriminant_map: &'a RefCell<NodeMap<Option<&'ast hir::Expr>>>,
+struct CheckItemRecursionVisitor<'a, 'b: 'a, 'ast: 'b> {
+    root_span: &'b Span,
+    sess: &'b Session,
+    ast_map: &'b ast_map::Map<'ast>,
+    discriminant_map: &'a mut NodeMap<Option<&'ast hir::Expr>>,
     idstack: Vec<ast::NodeId>,
+    detected_recursive_ids: &'a mut NodeSet,
 }
 
-impl<'a, 'ast: 'a> CheckItemRecursionVisitor<'a, 'ast> {
-    fn new(v: &'a CheckCrateVisitor<'a, 'ast>,
-           span: &'a Span)
-           -> CheckItemRecursionVisitor<'a, 'ast> {
+impl<'a, 'b: 'a, 'ast: 'b> CheckItemRecursionVisitor<'a, 'b, 'ast> {
+    fn new(v: &'a mut CheckCrateVisitor<'b, 'ast>, span: &'b Span) -> Self {
         CheckItemRecursionVisitor {
             root_span: span,
             sess: v.sess,
             ast_map: v.ast_map,
-            discriminant_map: &v.discriminant_map,
+            discriminant_map: &mut v.discriminant_map,
             idstack: Vec::new(),
+            detected_recursive_ids: &mut v.detected_recursive_ids,
         }
     }
     fn with_item_id_pushed<F>(&mut self, id: ast::NodeId, f: F, span: Span)
         where F: Fn(&mut Self)
     {
         if self.idstack.iter().any(|&x| x == id) {
+            if self.detected_recursive_ids.contains(&id) {
+                return;
+            }
+            self.detected_recursive_ids.insert(id);
             let any_static = self.idstack.iter().any(|&x| {
                 if let ast_map::NodeItem(item) = self.ast_map.get(x) {
                     if let hir::ItemStatic(..) = item.node {
@@ -168,15 +170,14 @@ impl<'a, 'ast: 'a> CheckItemRecursionVisitor<'a, 'ast> {
     // 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 hir::EnumDef) {
+    fn populate_enum_discriminants(&mut self, enum_definition: &'ast hir::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.data.id()) => {
+            Some(variant) if self.discriminant_map.contains_key(&variant.node.data.id()) => {
                 return;
             }
             _ => {}
@@ -190,24 +191,23 @@ impl<'a, 'ast: 'a> CheckItemRecursionVisitor<'a, 'ast> {
             // 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));
+                    self.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);
+            self.discriminant_map.insert(*id, None);
         }
     }
 }
 
-impl<'a, 'ast: 'a> Visitor<'ast> for CheckItemRecursionVisitor<'a, 'ast> {
+impl<'a, 'b: 'a, 'ast: 'b> Visitor<'ast> for CheckItemRecursionVisitor<'a, 'b, 'ast> {
     fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'ast> {
         NestedVisitorMap::OnlyBodies(&self.ast_map)
     }
-
     fn visit_item(&mut self, it: &'ast hir::Item) {
         self.with_item_id_pushed(it.id, |v| intravisit::walk_item(v, it), it.span);
     }
@@ -227,7 +227,7 @@ impl<'a, 'ast: 'a> Visitor<'ast> for CheckItemRecursionVisitor<'a, 'ast> {
                      _: ast::NodeId) {
         let variant_id = variant.node.data.id();
         let maybe_expr;
-        if let Some(get_expr) = self.discriminant_map.borrow().get(&variant_id) {
+        if let Some(get_expr) = self.discriminant_map.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();
@@ -251,51 +251,46 @@ impl<'a, 'ast: 'a> Visitor<'ast> for CheckItemRecursionVisitor<'a, 'ast> {
         self.with_item_id_pushed(ii.id, |v| intravisit::walk_impl_item(v, ii), ii.span);
     }
 
-    fn visit_expr(&mut self, e: &'ast hir::Expr) {
-        match e.node {
-            hir::ExprPath(hir::QPath::Resolved(_, ref path)) => {
-                match path.def {
-                    Def::Static(def_id, _) |
-                    Def::AssociatedConst(def_id) |
-                    Def::Const(def_id) => {
-                        if let Some(node_id) = self.ast_map.as_local_node_id(def_id) {
-                            match self.ast_map.get(node_id) {
-                                ast_map::NodeItem(item) => self.visit_item(item),
-                                ast_map::NodeTraitItem(item) => self.visit_trait_item(item),
-                                ast_map::NodeImplItem(item) => self.visit_impl_item(item),
-                                ast_map::NodeForeignItem(_) => {}
-                                _ => {
-                                    span_bug!(e.span,
-                                              "expected item, found {}",
-                                              self.ast_map.node_to_string(node_id));
-                                }
-                            }
+    fn visit_path(&mut self, path: &'ast hir::Path, _: ast::NodeId) {
+        match path.def {
+            Def::Static(def_id, _) |
+            Def::AssociatedConst(def_id) |
+            Def::Const(def_id) => {
+                if let Some(node_id) = self.ast_map.as_local_node_id(def_id) {
+                    match self.ast_map.get(node_id) {
+                        ast_map::NodeItem(item) => self.visit_item(item),
+                        ast_map::NodeTraitItem(item) => self.visit_trait_item(item),
+                        ast_map::NodeImplItem(item) => self.visit_impl_item(item),
+                        ast_map::NodeForeignItem(_) => {}
+                        _ => {
+                            span_bug!(path.span,
+                                      "expected item, found {}",
+                                      self.ast_map.node_to_string(node_id));
                         }
                     }
-                    // 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).
-                    Def::VariantCtor(variant_id, CtorKind::Const) => {
-                        if let Some(variant_id) = self.ast_map.as_local_node_id(variant_id) {
-                            let variant = self.ast_map.expect_variant(variant_id);
-                            let enum_id = self.ast_map.get_parent(variant_id);
-                            let enum_item = self.ast_map.expect_item(enum_id);
-                            if let hir::ItemEnum(ref enum_def, ref generics) = enum_item.node {
-                                self.populate_enum_discriminants(enum_def);
-                                self.visit_variant(variant, generics, enum_id);
-                            } else {
-                                span_bug!(e.span,
-                                          "`check_static_recursion` found \
-                                           non-enum in Def::VariantCtor");
-                            }
-                        }
+                }
+            }
+            // 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).
+            Def::VariantCtor(variant_id, CtorKind::Const) => {
+                if let Some(variant_id) = self.ast_map.as_local_node_id(variant_id) {
+                    let variant = self.ast_map.expect_variant(variant_id);
+                    let enum_id = self.ast_map.get_parent(variant_id);
+                    let enum_item = self.ast_map.expect_item(enum_id);
+                    if let hir::ItemEnum(ref enum_def, ref generics) = enum_item.node {
+                        self.populate_enum_discriminants(enum_def);
+                        self.visit_variant(variant, generics, enum_id);
+                    } else {
+                        span_bug!(path.span,
+                                  "`check_static_recursion` found \
+                                    non-enum in Def::VariantCtor");
                     }
-                    _ => (),
                 }
             }
             _ => (),
         }
-        intravisit::walk_expr(self, e);
+        intravisit::walk_path(self, path);
     }
 }

src/test/compile-fail/issue-36163.rs

@@ -0,0 +1,26 @@
+// Copyright 2012 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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+const A: i32 = Foo::B; //~ ERROR E0265
+                       //~^ NOTE recursion not allowed in constant
+
+enum Foo {
+    B = A, //~ ERROR E0265
+           //~^ NOTE recursion not allowed in constant
+}
+
+enum Bar {
+    C = Bar::C, //~ ERROR E0265
+                //~^ NOTE recursion not allowed in constant
+}
+
+const D: i32 = A;
+
+fn main() {}