From b384b18d5d2417616d043602fd023ee48fd4b934 Mon Sep 17 00:00:00 2001 From: kennytm Date: Tue, 16 May 2017 03:16:18 +0800 Subject: [PATCH] Refactor: Move the mutable parts out of LintStore. Fix #42007. * #42007 happens because the Session LintStore is emptied when linting. * The Session LintStore is emptied because the checker (Early/LateContext) wants ownership. * The checker wants ownership because it wants to mutate the pass objects and lint levels. The ownership of the whole store is not essential, only the lint levels and pass objects need to be owned. Therefore, these parts are extracted out of the LintStore into a separate structure `LintSession`. The "check crates" methods can operate on `&mut LintSession` instead of `&mut LintStore`. This is a minor BREAKING CHANGE for lint writers since the `LintContext` trait is changed: the `mut_lints` and `level_stack` methods are removed. But no one outside of `librustc/lint/context.rs` is using these functions, so it should be safe. --- src/librustc/lint/context.rs | 251 ++++++++++++------- src/test/run-pass/auxiliary/issue_42007_s.rs | 14 ++ src/test/run-pass/issue-42007.rs | 19 ++ 3 files changed, 198 insertions(+), 86 deletions(-) create mode 100644 src/test/run-pass/auxiliary/issue_42007_s.rs create mode 100644 src/test/run-pass/issue-42007.rs diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 9d5ba2c8f9501..e7c8d3285c8fe 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -40,6 +40,7 @@ use std::cmp; use std::default::Default as StdDefault; use std::mem; use std::fmt; +use std::cell::{Ref, RefCell}; use syntax::attr; use syntax::ast; use syntax::symbol::Symbol; @@ -61,8 +62,8 @@ pub struct LintStore { lints: Vec<(&'static Lint, bool)>, /// Trait objects for each lint pass. - /// This is only `None` while iterating over the objects. See the definition - /// of run_lints. + /// This is only `None` while performing a lint pass. See the definition + /// of `LintSession::new`. early_passes: Option>, late_passes: Option>, @@ -70,7 +71,7 @@ pub struct LintStore { by_name: FxHashMap, /// Current levels of each lint, and where they were set. - levels: FxHashMap, + levels: LintLevels, /// Map of registered lint groups to what lints they expand to. The bool /// is true if the lint group was added by a plugin. @@ -79,11 +80,36 @@ pub struct LintStore { /// Extra info for future incompatibility lints, descibing the /// issue or RFC that caused the incompatibility. future_incompatible: FxHashMap, +} + + +#[derive(Default)] +struct LintLevels { + /// Current levels of each lint, and where they were set. + levels: FxHashMap, /// Maximum level a lint can be lint_cap: Option, } + +pub struct LintSession<'a, PassObject> { + /// Reference to the store of registered lints. + lints: Ref<'a, LintStore>, + + /// The current lint levels. + levels: LintLevels, + + /// When recursing into an attributed node of the ast which modifies lint + /// levels, this stack keeps track of the previous lint levels of whatever + /// was modified. + stack: Vec<(LintId, LevelSource)>, + + /// Trait objects for each lint pass. + passes: Option>, +} + + /// When you call `add_lint` on the session, you wind up storing one /// of these, which records a "potential lint" at a particular point. #[derive(PartialEq, RustcEncodable, RustcDecodable)] @@ -157,34 +183,15 @@ enum FindLintError { } impl LintStore { - fn get_level_source(&self, lint: LintId) -> LevelSource { - match self.levels.get(&lint) { - Some(&s) => s, - None => (Allow, Default), - } - } - - fn set_level(&mut self, lint: LintId, mut lvlsrc: LevelSource) { - if let Some(cap) = self.lint_cap { - lvlsrc.0 = cmp::min(lvlsrc.0, cap); - } - if lvlsrc.0 == Allow { - self.levels.remove(&lint); - } else { - self.levels.insert(lint, lvlsrc); - } - } - pub fn new() -> LintStore { LintStore { lints: vec![], early_passes: Some(vec![]), late_passes: Some(vec![]), by_name: FxHashMap(), - levels: FxHashMap(), + levels: LintLevels::default(), future_incompatible: FxHashMap(), lint_groups: FxHashMap(), - lint_cap: None, } } @@ -236,9 +243,7 @@ impl LintStore { } } - if lint.default_level != Allow { - self.levels.insert(id, (lint.default_level, Default)); - } + self.levels.set(id, (lint.default_level, Default)); } } @@ -310,7 +315,7 @@ impl LintStore { let lint_flag_val = Symbol::intern(&lint_name); match self.find_lint(&lint_name[..], sess, None) { - Ok(lint_id) => self.set_level(lint_id, (level, CommandLine(lint_flag_val))), + Ok(lint_id) => self.levels.set(lint_id, (level, CommandLine(lint_flag_val))), Err(FindLintError::Removed) => { } Err(_) => { match self.lint_groups.iter().map(|(&x, pair)| (x, pair.0.clone())) @@ -318,10 +323,9 @@ impl LintStore { Vec>>() .get(&lint_name[..]) { Some(v) => { - v.iter() - .map(|lint_id: &LintId| - self.set_level(*lint_id, (level, CommandLine(lint_flag_val)))) - .collect::>(); + for lint_id in v { + self.levels.set(*lint_id, (level, CommandLine(lint_flag_val))); + } } None => { // The lint or lint group doesn't exist. @@ -333,15 +337,73 @@ impl LintStore { } } - self.lint_cap = sess.opts.lint_cap; + self.levels.set_lint_cap(sess.opts.lint_cap); + } +} + + +impl LintLevels { + fn get_source(&self, lint: LintId) -> LevelSource { + match self.levels.get(&lint) { + Some(&s) => s, + None => (Allow, Default), + } + } + + fn set(&mut self, lint: LintId, mut lvlsrc: LevelSource) { if let Some(cap) = self.lint_cap { - for level in self.levels.iter_mut().map(|p| &mut (p.1).0) { - *level = cmp::min(*level, cap); + lvlsrc.0 = cmp::min(lvlsrc.0, cap); + } + if lvlsrc.0 == Allow { + self.levels.remove(&lint); + } else { + self.levels.insert(lint, lvlsrc); + } + } + + fn set_lint_cap(&mut self, lint_cap: Option) { + self.lint_cap = lint_cap; + if let Some(cap) = lint_cap { + for (_, level) in &mut self.levels { + level.0 = cmp::min(level.0, cap); } } } } + +impl<'a, PassObject: LintPassObject> LintSession<'a, PassObject> { + /// Creates a new `LintSession`, by moving out the `LintStore`'s initial + /// lint levels and pass objects. These can be restored using the `restore` + /// method. + fn new(store: &'a RefCell) -> LintSession<'a, PassObject> { + let mut s = store.borrow_mut(); + let levels = mem::replace(&mut s.levels, LintLevels::default()); + let passes = PassObject::take_passes(&mut *s); + drop(s); + LintSession { + lints: store.borrow(), + stack: Vec::new(), + levels, + passes, + } + } + + /// Restores the levels back to the original lint store. + fn restore(self, store: &RefCell) { + drop(self.lints); + let mut s = store.borrow_mut(); + s.levels = self.levels; + PassObject::restore_passes(&mut *s, self.passes); + } + + fn get_source(&self, lint_id: LintId) -> LevelSource { + self.levels.get_source(lint_id) + } +} + + + /// Context for lint checking after type checking. pub struct LateContext<'a, 'tcx: 'a> { /// Type context we're checking in. @@ -356,13 +418,8 @@ pub struct LateContext<'a, 'tcx: 'a> { /// Items accessible from the crate being checked. pub access_levels: &'a AccessLevels, - /// The store of registered lints. - lints: LintStore, - - /// When recursing into an attributed node of the ast which modifies lint - /// levels, this stack keeps track of the previous lint levels of whatever - /// was modified. - level_stack: Vec<(LintId, LevelSource)>, + /// The store of registered lints and the lint levels. + lint_sess: LintSession<'tcx, LateLintPassObject>, } /// Context for lint checking of the AST, after expansion, before lowering to @@ -374,24 +431,19 @@ pub struct EarlyContext<'a> { /// The crate being checked. pub krate: &'a ast::Crate, - /// The store of registered lints. - lints: LintStore, - - /// When recursing into an attributed node of the ast which modifies lint - /// levels, this stack keeps track of the previous lint levels of whatever - /// was modified. - level_stack: Vec<(LintId, LevelSource)>, + /// The store of registered lints and the lint levels. + lint_sess: LintSession<'a, EarlyLintPassObject>, } /// Convenience macro for calling a `LintPass` method on every pass in the context. macro_rules! run_lints { ($cx:expr, $f:ident, $ps:ident, $($args:expr),*) => ({ // Move the vector of passes out of `$cx` so that we can // iterate over it mutably while passing `$cx` to the methods. - let mut passes = $cx.mut_lints().$ps.take().unwrap(); + let mut passes = $cx.lint_sess_mut().passes.take().unwrap(); for obj in &mut passes { obj.$f($cx, $($args),*); } - $cx.mut_lints().$ps = Some(passes); + $cx.lint_sess_mut().passes = Some(passes); }) } /// Parse the lint attributes into a vector, with `Err`s for malformed lint @@ -522,25 +574,55 @@ pub fn raw_struct_lint<'a, S>(sess: &'a Session, err } + +pub trait LintPassObject: Sized { + fn take_passes(store: &mut LintStore) -> Option>; + fn restore_passes(store: &mut LintStore, passes: Option>); +} + +impl LintPassObject for EarlyLintPassObject { + fn take_passes(store: &mut LintStore) -> Option> { + store.early_passes.take() + } + + fn restore_passes(store: &mut LintStore, passes: Option>) { + store.early_passes = passes; + } +} + +impl LintPassObject for LateLintPassObject { + fn take_passes(store: &mut LintStore) -> Option> { + store.late_passes.take() + } + + fn restore_passes(store: &mut LintStore, passes: Option>) { + store.late_passes = passes; + } +} + + pub trait LintContext<'tcx>: Sized { + type PassObject: LintPassObject; + fn sess(&self) -> &Session; fn lints(&self) -> &LintStore; - fn mut_lints(&mut self) -> &mut LintStore; - fn level_stack(&mut self) -> &mut Vec<(LintId, LevelSource)>; + fn lint_sess(&self) -> &LintSession<'tcx, Self::PassObject>; + fn lint_sess_mut(&mut self) -> &mut LintSession<'tcx, Self::PassObject>; fn enter_attrs(&mut self, attrs: &'tcx [ast::Attribute]); fn exit_attrs(&mut self, attrs: &'tcx [ast::Attribute]); /// Get the level of `lint` at the current position of the lint /// traversal. fn current_level(&self, lint: &'static Lint) -> Level { - self.lints().levels.get(&LintId::of(lint)).map_or(Allow, |&(lvl, _)| lvl) + self.lint_sess().get_source(LintId::of(lint)).0 } fn level_src(&self, lint: &'static Lint) -> Option { - self.lints().levels.get(&LintId::of(lint)).map(|ls| match ls { + let ref levels = self.lint_sess().levels; + levels.levels.get(&LintId::of(lint)).map(|ls| match ls { &(Warn, _) => { let lint_id = LintId::of(builtin::WARNINGS); - let warn_src = self.lints().get_level_source(lint_id); + let warn_src = levels.get_source(lint_id); if warn_src.0 != Warn { warn_src } else { @@ -674,7 +756,7 @@ pub trait LintContext<'tcx>: Sized { let lint_attr_name = result.expect("lint attribute should be well-formed").0; for (lint_id, level, span) in v { - let (now, now_source) = self.lints().get_level_source(lint_id); + let (now, now_source) = self.lint_sess().get_source(lint_id); if now == Forbid && level != Forbid { let lint_name = lint_id.to_string(); let mut diag_builder = struct_span_err!(self.sess(), span, E0453, @@ -693,10 +775,10 @@ pub trait LintContext<'tcx>: Sized { } }.emit() } else if now != level { - let src = self.lints().get_level_source(lint_id).1; - self.level_stack().push((lint_id, (now, src))); + let cx = self.lint_sess_mut(); + cx.stack.push((lint_id, (now, now_source))); pushed += 1; - self.mut_lints().set_level(lint_id, (level, Node(lint_attr_name, span))); + cx.levels.set(lint_id, (level, Node(lint_attr_name, span))); } } } @@ -706,9 +788,10 @@ pub trait LintContext<'tcx>: Sized { self.exit_attrs(attrs); // rollback + let cx = self.lint_sess_mut(); for _ in 0..pushed { - let (lint, lvlsrc) = self.level_stack().pop().unwrap(); - self.mut_lints().set_level(lint, lvlsrc); + let (lint, lvlsrc) = cx.stack.pop().unwrap(); + cx.levels.set(lint, lvlsrc); } } } @@ -717,35 +800,32 @@ pub trait LintContext<'tcx>: Sized { impl<'a> EarlyContext<'a> { fn new(sess: &'a Session, krate: &'a ast::Crate) -> EarlyContext<'a> { - // We want to own the lint store, so move it out of the session. Remember - // to put it back later... - let lint_store = mem::replace(&mut *sess.lint_store.borrow_mut(), - LintStore::new()); EarlyContext { sess: sess, krate: krate, - lints: lint_store, - level_stack: vec![], + lint_sess: LintSession::new(&sess.lint_store), } } } impl<'a, 'tcx> LintContext<'tcx> for LateContext<'a, 'tcx> { + type PassObject = LateLintPassObject; + /// Get the overall compiler `Session` object. fn sess(&self) -> &Session { &self.tcx.sess } fn lints(&self) -> &LintStore { - &self.lints + &*self.lint_sess.lints } - fn mut_lints(&mut self) -> &mut LintStore { - &mut self.lints + fn lint_sess(&self) -> &LintSession<'tcx, Self::PassObject> { + &self.lint_sess } - fn level_stack(&mut self) -> &mut Vec<(LintId, LevelSource)> { - &mut self.level_stack + fn lint_sess_mut(&mut self) -> &mut LintSession<'tcx, Self::PassObject> { + &mut self.lint_sess } fn enter_attrs(&mut self, attrs: &'tcx [ast::Attribute]) { @@ -760,21 +840,23 @@ impl<'a, 'tcx> LintContext<'tcx> for LateContext<'a, 'tcx> { } impl<'a> LintContext<'a> for EarlyContext<'a> { + type PassObject = EarlyLintPassObject; + /// Get the overall compiler `Session` object. fn sess(&self) -> &Session { &self.sess } fn lints(&self) -> &LintStore { - &self.lints + &*self.lint_sess.lints } - fn mut_lints(&mut self) -> &mut LintStore { - &mut self.lints + fn lint_sess(&self) -> &LintSession<'a, Self::PassObject> { + &self.lint_sess } - fn level_stack(&mut self) -> &mut Vec<(LintId, LevelSource)> { - &mut self.level_stack + fn lint_sess_mut(&mut self) -> &mut LintSession<'a, Self::PassObject> { + &mut self.lint_sess } fn enter_attrs(&mut self, attrs: &'a [ast::Attribute]) { @@ -1191,7 +1273,7 @@ fn check_lint_name_attribute(cx: &LateContext, attr: &ast::Attribute) { continue; } Ok((lint_name, _, span)) => { - match check_lint_name(&cx.lints, &lint_name.as_str()) { + match check_lint_name(&cx.lint_sess.lints, &lint_name.as_str()) { CheckLintNameResult::Ok => (), CheckLintNameResult::Warning(ref msg) => { cx.span_lint(builtin::RENAMED_AND_REMOVED_LINTS, @@ -1246,15 +1328,12 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { let krate = tcx.hir.krate(); - // We want to own the lint store, so move it out of the session. - let lint_store = mem::replace(&mut *tcx.sess.lint_store.borrow_mut(), LintStore::new()); let mut cx = LateContext { tcx: tcx, tables: &ty::TypeckTables::empty(), krate: krate, access_levels: access_levels, - lints: lint_store, - level_stack: vec![], + lint_sess: LintSession::new(&tcx.sess.lint_store), }; // Visit the whole crate. @@ -1278,8 +1357,8 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { } } - // Put the lint store back in the session. - mem::replace(&mut *tcx.sess.lint_store.borrow_mut(), cx.lints); + // Put the lint store levels and passes back in the session. + cx.lint_sess.restore(&tcx.sess.lint_store); } pub fn check_ast_crate(sess: &Session, krate: &ast::Crate) { @@ -1302,8 +1381,8 @@ pub fn check_ast_crate(sess: &Session, krate: &ast::Crate) { run_lints!(cx, check_crate_post, early_passes, krate); }); - // Put the lint store back in the session. - mem::replace(&mut *sess.lint_store.borrow_mut(), cx.lints); + // Put the lint store levels and passes back in the session. + cx.lint_sess.restore(&sess.lint_store); // If we missed any lints added to the session, then there's a bug somewhere // in the iteration code. diff --git a/src/test/run-pass/auxiliary/issue_42007_s.rs b/src/test/run-pass/auxiliary/issue_42007_s.rs new file mode 100644 index 0000000000000..b965e916f98f0 --- /dev/null +++ b/src/test/run-pass/auxiliary/issue_42007_s.rs @@ -0,0 +1,14 @@ +// Copyright 2017 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. + +#[repr(u8)] +pub enum E { + B = 1 as u8, +} \ No newline at end of file diff --git a/src/test/run-pass/issue-42007.rs b/src/test/run-pass/issue-42007.rs new file mode 100644 index 0000000000000..cc7e3bc372cc6 --- /dev/null +++ b/src/test/run-pass/issue-42007.rs @@ -0,0 +1,19 @@ +// Copyright 2017 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. + +// aux-build:issue_42007_s.rs + +extern crate issue_42007_s; + +enum I { + E(issue_42007_s::E), +} + +fn main() {}