Auto merge of #26630 - eefriedman:recursive-static, r=pnkfelix

***Edit: Fixed now.*** I'm pretty sure the way I'm using LLVMReplaceAllUsesWith here is
unsafe... but before I figure out how to fix that, I'd like a
reality-check: is this actually useful?

Author: bors

src/librustc/middle/check_static_recursion.rs

@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// This compiler pass detects static items that refer to themselves
+// This compiler pass detects constants that refer to themselves
 // recursively.
 
 use ast_map;
@@ -18,6 +18,7 @@ use util::nodemap::NodeMap;
 
 use syntax::{ast, ast_util};
 use syntax::codemap::Span;
+use syntax::feature_gate::emit_feature_err;
 use syntax::visit::Visitor;
 use syntax::visit;
 
@@ -125,8 +126,27 @@ impl<'a, 'ast: 'a> CheckItemRecursionVisitor<'a, 'ast> {
     }
     fn with_item_id_pushed<F>(&mut self, id: ast::NodeId, f: F)
           where F: Fn(&mut Self) {
-        if self.idstack.iter().any(|x| *x == id) {
-            span_err!(self.sess, *self.root_span, E0265, "recursive constant");
+        if self.idstack.iter().any(|&x| x == id) {
+            let any_static = self.idstack.iter().any(|&x| {
+                if let ast_map::NodeItem(item) = self.ast_map.get(x) {
+                    if let ast::ItemStatic(..) = item.node {
+                        true
+                    } else {
+                        false
+                    }
+                } else {
+                    false
+                }
+            });
+            if any_static {
+                if !self.sess.features.borrow().static_recursion {
+                    emit_feature_err(&self.sess.parse_sess.span_diagnostic,
+                                     "static_recursion",
+                                     *self.root_span, "recursive static");
+                }
+            } else {
+                span_err!(self.sess, *self.root_span, E0265, "recursive constant");
+            }
             return;
         }
         self.idstack.push(id);

src/librustc_trans/trans/base.rs

@@ -2090,7 +2090,7 @@ pub fn trans_item(ccx: &CrateContext, item: &ast::Item) {
           let mut v = TransItemVisitor{ ccx: ccx };
           v.visit_expr(&**expr);
 
-          let g = consts::trans_static(ccx, m, item.id);
+          let g = consts::trans_static(ccx, m, expr, item.id, &item.attrs);
           update_linkage(ccx, g, Some(item.id), OriginalTranslation);
       },
       ast::ItemForeignMod(ref foreign_mod) => {
@@ -2334,44 +2334,25 @@ pub fn get_item_val(ccx: &CrateContext, id: ast::NodeId) -> ValueRef {
             let sym = || exported_name(ccx, id, ty, &i.attrs);
 
             let v = match i.node {
-                ast::ItemStatic(_, _, ref expr) => {
+                ast::ItemStatic(..) => {
                     // If this static came from an external crate, then
                     // we need to get the symbol from csearch instead of
                     // using the current crate's name/version
                     // information in the hash of the symbol
                     let sym = sym();
                     debug!("making {}", sym);
 
-                    // We need the translated value here, because for enums the
-                    // LLVM type is not fully determined by the Rust type.
-                    let empty_substs = ccx.tcx().mk_substs(Substs::trans_empty());
-                    let (v, ty) = consts::const_expr(ccx, &**expr, empty_substs, None);
-                    ccx.static_values().borrow_mut().insert(id, v);
-                    unsafe {
-                        // boolean SSA values are i1, but they have to be stored in i8 slots,
-                        // otherwise some LLVM optimization passes don't work as expected
-                        let llty = if ty.is_bool() {
-                            llvm::LLVMInt8TypeInContext(ccx.llcx())
-                        } else {
-                            llvm::LLVMTypeOf(v)
-                        };
-
-                        // FIXME(nagisa): probably should be declare_global, because no definition
-                        // is happening here, but we depend on it being defined here from
-                        // const::trans_static. This all logic should be replaced.
-                        let g = declare::define_global(ccx, &sym[..],
-                                                       Type::from_ref(llty)).unwrap_or_else(||{
-                            ccx.sess().span_fatal(i.span, &format!("symbol `{}` is already defined",
-                                                                   sym))
-                        });
-
-                        if attr::contains_name(&i.attrs,
-                                               "thread_local") {
-                            llvm::set_thread_local(g, true);
-                        }
-                        ccx.item_symbols().borrow_mut().insert(i.id, sym);
-                        g
-                    }
+                    // Create the global before evaluating the initializer;
+                    // this is necessary to allow recursive statics.
+                    let llty = type_of(ccx, ty);
+                    let g = declare::define_global(ccx, &sym[..],
+                                                   llty).unwrap_or_else(|| {
+                        ccx.sess().span_fatal(i.span, &format!("symbol `{}` is already defined",
+                                                                sym))
+                    });
+
+                    ccx.item_symbols().borrow_mut().insert(i.id, sym);
+                    g
                 }
 
                 ast::ItemFn(_, _, _, abi, _, _) => {
@@ -2738,6 +2719,13 @@ pub fn trans_crate(tcx: &ty::ctxt, analysis: ty::CrateAnalysis) -> CrateTranslat
         if ccx.sess().opts.debuginfo != NoDebugInfo {
             debuginfo::finalize(&ccx);
         }
+        for &(old_g, new_g) in ccx.statics_to_rauw().borrow().iter() {
+            unsafe {
+                let bitcast = llvm::LLVMConstPointerCast(new_g, llvm::LLVMTypeOf(old_g));
+                llvm::LLVMReplaceAllUsesWith(old_g, bitcast);
+                llvm::LLVMDeleteGlobal(old_g);
+            }
+        }
     }
 
     // Translate the metadata.

src/librustc_trans/trans/consts.rs

@@ -37,8 +37,9 @@ use middle::subst::Substs;
 use middle::ty::{self, Ty};
 use util::nodemap::NodeMap;
 
+use std::ffi::{CStr, CString};
 use libc::c_uint;
-use syntax::{ast, ast_util};
+use syntax::{ast, ast_util, attr};
 use syntax::parse::token;
 use syntax::ptr::P;
 
@@ -898,37 +899,70 @@ fn const_expr_unadjusted<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
                                 "bad constant expression type in consts::const_expr"),
     }
 }
-
-pub fn trans_static(ccx: &CrateContext, m: ast::Mutability, id: ast::NodeId) -> ValueRef {
+pub fn trans_static(ccx: &CrateContext,
+                    m: ast::Mutability,
+                    expr: &ast::Expr,
+                    id: ast::NodeId,
+                    attrs: &Vec<ast::Attribute>)
+                    -> ValueRef {
     unsafe {
         let _icx = push_ctxt("trans_static");
         let g = base::get_item_val(ccx, id);
-        // At this point, get_item_val has already translated the
-        // constant's initializer to determine its LLVM type.
-        let v = ccx.static_values().borrow().get(&id).unwrap().clone();
+
+        let empty_substs = ccx.tcx().mk_substs(Substs::trans_empty());
+        let (v, _) = const_expr(ccx, expr, empty_substs, None);
+
         // boolean SSA values are i1, but they have to be stored in i8 slots,
         // otherwise some LLVM optimization passes don't work as expected
-        let v = if llvm::LLVMTypeOf(v) == Type::i1(ccx).to_ref() {
-            llvm::LLVMConstZExt(v, Type::i8(ccx).to_ref())
+        let mut val_llty = llvm::LLVMTypeOf(v);
+        let v = if val_llty == Type::i1(ccx).to_ref() {
+            val_llty = Type::i8(ccx).to_ref();
+            llvm::LLVMConstZExt(v, val_llty)
         } else {
             v
         };
+
+        let ty = ccx.tcx().node_id_to_type(id);
+        let llty = type_of::type_of(ccx, ty);
+        let g = if val_llty == llty.to_ref() {
+            g
+        } else {
+            // If we created the global with the wrong type,
+            // correct the type.
+            let empty_string = CString::new("").unwrap();
+            let name_str_ref = CStr::from_ptr(llvm::LLVMGetValueName(g));
+            let name_string = CString::new(name_str_ref.to_bytes()).unwrap();
+            llvm::LLVMSetValueName(g, empty_string.as_ptr());
+            let new_g = llvm::LLVMGetOrInsertGlobal(
+                ccx.llmod(), name_string.as_ptr(), val_llty);
+            // To avoid breaking any invariants, we leave around the old
+            // global for the moment; we'll replace all references to it
+            // with the new global later. (See base::trans_crate.)
+            ccx.statics_to_rauw().borrow_mut().push((g, new_g));
+            new_g
+        };
         llvm::LLVMSetInitializer(g, v);
 
         // As an optimization, all shared statics which do not have interior
         // mutability are placed into read-only memory.
         if m != ast::MutMutable {
-            let node_ty = ccx.tcx().node_id_to_type(id);
-            let tcontents = node_ty.type_contents(ccx.tcx());
+            let tcontents = ty.type_contents(ccx.tcx());
             if !tcontents.interior_unsafe() {
-                llvm::LLVMSetGlobalConstant(g, True);
+                llvm::LLVMSetGlobalConstant(g, llvm::True);
             }
         }
+
         debuginfo::create_global_var_metadata(ccx, id, g);
+
+        if attr::contains_name(attrs,
+                               "thread_local") {
+            llvm::set_thread_local(g, true);
+        }
         g
     }
 }
 
+
 fn get_static_val<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, did: ast::DefId,
                             ty: Ty<'tcx>) -> ValueRef {
     if ast_util::is_local(did) { return base::get_item_val(ccx, did.node) }

src/librustc_trans/trans/context.rs

@@ -118,9 +118,6 @@ pub struct LocalCrateContext<'tcx> {
     /// Cache of emitted const values
     const_values: RefCell<FnvHashMap<(ast::NodeId, &'tcx Substs<'tcx>), ValueRef>>,
 
-    /// Cache of emitted static values
-    static_values: RefCell<NodeMap<ValueRef>>,
-
     /// Cache of external const values
     extern_const_values: RefCell<DefIdMap<ValueRef>>,
 
@@ -129,6 +126,12 @@ pub struct LocalCrateContext<'tcx> {
     /// Cache of closure wrappers for bare fn's.
     closure_bare_wrapper_cache: RefCell<FnvHashMap<ValueRef, ValueRef>>,
 
+    /// List of globals for static variables which need to be passed to the
+    /// LLVM function ReplaceAllUsesWith (RAUW) when translation is complete.
+    /// (We have to make sure we don't invalidate any ValueRefs referring
+    /// to constants.)
+    statics_to_rauw: RefCell<Vec<(ValueRef, ValueRef)>>,
+
     lltypes: RefCell<FnvHashMap<Ty<'tcx>, Type>>,
     llsizingtypes: RefCell<FnvHashMap<Ty<'tcx>, Type>>,
     adt_reprs: RefCell<FnvHashMap<Ty<'tcx>, Rc<adt::Repr<'tcx>>>>,
@@ -449,10 +452,10 @@ impl<'tcx> LocalCrateContext<'tcx> {
                 const_unsized: RefCell::new(FnvHashMap()),
                 const_globals: RefCell::new(FnvHashMap()),
                 const_values: RefCell::new(FnvHashMap()),
-                static_values: RefCell::new(NodeMap()),
                 extern_const_values: RefCell::new(DefIdMap()),
                 impl_method_cache: RefCell::new(FnvHashMap()),
                 closure_bare_wrapper_cache: RefCell::new(FnvHashMap()),
+                statics_to_rauw: RefCell::new(Vec::new()),
                 lltypes: RefCell::new(FnvHashMap()),
                 llsizingtypes: RefCell::new(FnvHashMap()),
                 adt_reprs: RefCell::new(FnvHashMap()),
@@ -660,10 +663,6 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
         &self.local.const_values
     }
 
-    pub fn static_values<'a>(&'a self) -> &'a RefCell<NodeMap<ValueRef>> {
-        &self.local.static_values
-    }
-
     pub fn extern_const_values<'a>(&'a self) -> &'a RefCell<DefIdMap<ValueRef>> {
         &self.local.extern_const_values
     }
@@ -677,6 +676,10 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
         &self.local.closure_bare_wrapper_cache
     }
 
+    pub fn statics_to_rauw<'a>(&'a self) -> &'a RefCell<Vec<(ValueRef, ValueRef)>> {
+        &self.local.statics_to_rauw
+    }
+
     pub fn lltypes<'a>(&'a self) -> &'a RefCell<FnvHashMap<Ty<'tcx>, Type>> {
         &self.local.lltypes
     }

src/librustc_typeck/check/mod.rs

@@ -115,6 +115,7 @@ use syntax::attr::AttrMetaMethods;
 use syntax::ast::{self, DefId, Visibility};
 use syntax::ast_util::{self, local_def};
 use syntax::codemap::{self, Span};
+use syntax::feature_gate::emit_feature_err;
 use syntax::owned_slice::OwnedSlice;
 use syntax::parse::token;
 use syntax::print::pprust;
@@ -4009,9 +4010,7 @@ fn check_const_with_ty<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
 
 /// Checks whether a type can be represented in memory. In particular, it
 /// identifies types that contain themselves without indirection through a
-/// pointer, which would mean their size is unbounded. This is different from
-/// the question of whether a type can be instantiated. See the definition of
-/// `check_instantiable`.
+/// pointer, which would mean their size is unbounded.
 pub fn check_representable(tcx: &ty::ctxt,
                            sp: Span,
                            item_id: ast::NodeId,
@@ -4036,31 +4035,19 @@ pub fn check_representable(tcx: &ty::ctxt,
     return true
 }
 
-/// Checks whether a type can be created without an instance of itself.
-/// This is similar but different from the question of whether a type
-/// can be represented.  For example, the following type:
-///
-///     enum foo { None, Some(foo) }
-///
-/// is instantiable but is not representable.  Similarly, the type
-///
-///     enum foo { Some(@foo) }
-///
-/// is representable, but not instantiable.
+/// Checks whether a type can be constructed at runtime without
+/// an existing instance of that type.
 pub fn check_instantiable(tcx: &ty::ctxt,
                           sp: Span,
-                          item_id: ast::NodeId)
-                          -> bool {
+                          item_id: ast::NodeId) {
     let item_ty = tcx.node_id_to_type(item_id);
-    if !item_ty.is_instantiable(tcx) {
-        span_err!(tcx.sess, sp, E0073,
-            "this type cannot be instantiated without an \
-             instance of itself");
-        fileline_help!(tcx.sess, sp, "consider using `Option<{:?}>`",
-             item_ty);
-        false
-    } else {
-        true
+    if !item_ty.is_instantiable(tcx) &&
+            !tcx.sess.features.borrow().static_recursion {
+        emit_feature_err(&tcx.sess.parse_sess.span_diagnostic,
+                         "static_recursion",
+                         sp,
+                         "this type cannot be instantiated at runtime \
+                          without an instance of itself");
     }
 }
 
@@ -4199,11 +4186,6 @@ pub fn check_enum_variants<'a,'tcx>(ccx: &CrateCtxt<'a,'tcx>,
     do_check(ccx, vs, id, hint);
 
     check_representable(ccx.tcx, sp, id, "enum");
-
-    // Check that it is possible to instantiate this enum:
-    //
-    // This *sounds* like the same that as representable, but it's
-    // not.  See def'n of `check_instantiable()` for details.
     check_instantiable(ccx.tcx, sp, id);
 }
 

src/libsyntax/feature_gate.rs

@@ -160,6 +160,9 @@ const KNOWN_FEATURES: &'static [(&'static str, &'static str, Status)] = &[
 
     // Allows using #[prelude_import] on glob `use` items.
     ("prelude_import", "1.2.0", Active),
+
+    // Allows the definition recursive static items.
+    ("static_recursion", "1.3.0", Active),
 ];
 // (changing above list without updating src/doc/reference.md makes @cmr sad)
 
@@ -338,6 +341,7 @@ pub struct Features {
     /// #![feature] attrs for non-language (library) features
     pub declared_lib_features: Vec<(InternedString, Span)>,
     pub const_fn: bool,
+    pub static_recursion: bool
 }
 
 impl Features {
@@ -362,6 +366,7 @@ impl Features {
             declared_stable_lang_features: Vec::new(),
             declared_lib_features: Vec::new(),
             const_fn: false,
+            static_recursion: false
         }
     }
 }
@@ -859,6 +864,7 @@ fn check_crate_inner<F>(cm: &CodeMap, span_handler: &SpanHandler,
         declared_stable_lang_features: accepted_features,
         declared_lib_features: unknown_features,
         const_fn: cx.has_feature("const_fn"),
+        static_recursion: cx.has_feature("static_recursion")
     }
 }
 

src/test/compile-fail/const-recursive.rs

@@ -8,9 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// error-pattern: recursive constant
-static a: isize = b;
-static b: isize = a;
+const a: isize = b; //~ ERROR recursive constant
+const b: isize = a; //~ ERROR recursive constant
 
 fn main() {
 }