Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make some lints incremental #57293

Merged
merged 4 commits into from
Mar 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ rustc_dep_node_append!([define_dep_nodes!][ <'tcx>
[] UnsafetyCheckResult(DefId),
[] UnsafeDeriveOnReprPacked(DefId),

[] LintMod(DefId),
[] CheckModAttrs(DefId),
[] CheckModLoops(DefId),
[] CheckModUnstableApiUsage(DefId),
Expand Down
11 changes: 6 additions & 5 deletions src/librustc/hir/map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -580,17 +580,17 @@ impl<'hir> Map<'hir> {
&self.forest.krate.attrs
}

pub fn get_module(&self, module: DefId) -> (&'hir Mod, Span, NodeId)
{
pub fn get_module(&self, module: DefId) -> (&'hir Mod, Span, HirId) {
let node_id = self.as_local_node_id(module).unwrap();
let hir_id = self.node_to_hir_id(node_id);
self.read(node_id);
match self.find_entry(node_id).unwrap().node {
Node::Item(&Item {
span,
node: ItemKind::Mod(ref m),
..
}) => (m, span, node_id),
Node::Crate => (&self.forest.krate.module, self.forest.krate.span, node_id),
}) => (m, span, hir_id),
Node::Crate => (&self.forest.krate.module, self.forest.krate.span, hir_id),
_ => panic!("not a module")
}
}
Expand Down Expand Up @@ -1013,7 +1013,7 @@ impl<'hir> Map<'hir> {
/// corresponding to the Node ID
pub fn attrs(&self, id: NodeId) -> &'hir [ast::Attribute] {
self.read(id); // reveals attributes on the node
let attrs = match self.find(id) {
let attrs = match self.find_entry(id).map(|entry| entry.node) {
Some(Node::Local(l)) => Some(&l.attrs[..]),
Some(Node::Item(i)) => Some(&i.attrs[..]),
Some(Node::ForeignItem(fi)) => Some(&fi.attrs[..]),
Expand All @@ -1027,6 +1027,7 @@ impl<'hir> Map<'hir> {
// Unit/tuple structs/variants take the attributes straight from
// the struct/variant definition.
Some(Node::Ctor(..)) => return self.attrs(self.get_parent(id)),
Some(Node::Crate) => Some(&self.forest.krate.attrs[..]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually has a separate API from regular attribute access, in several places, it would be nice if we could get rid of the other APIs.

_ => None
};
attrs.unwrap_or(&[])
Expand Down
86 changes: 77 additions & 9 deletions src/librustc/lint/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use crate::rustc_serialize::{Decoder, Decodable, Encoder, Encodable};
use crate::session::{config, early_error, Session};
use crate::ty::{self, TyCtxt, Ty};
use crate::ty::layout::{LayoutError, LayoutOf, TyLayout};
use crate::ty::query::Providers;
use crate::util::nodemap::FxHashMap;
use crate::util::common::time;

Expand All @@ -36,8 +37,9 @@ use syntax::edition;
use syntax_pos::{MultiSpan, Span, symbol::{LocalInternedString, Symbol}};
use errors::DiagnosticBuilder;
use crate::hir;
use crate::hir::def_id::LOCAL_CRATE;
use crate::hir::def_id::{DefId, LOCAL_CRATE};
use crate::hir::intravisit as hir_visit;
use crate::hir::intravisit::Visitor;
use syntax::util::lev_distance::find_best_match_for_name;
use syntax::visit as ast_visit;

Expand All @@ -55,6 +57,7 @@ pub struct LintStore {
pre_expansion_passes: Option<Vec<EarlyLintPassObject>>,
early_passes: Option<Vec<EarlyLintPassObject>>,
late_passes: Option<Vec<LateLintPassObject>>,
late_module_passes: Option<Vec<LateLintPassObject>>,

/// Lints indexed by name.
by_name: FxHashMap<String, TargetLint>,
Expand Down Expand Up @@ -150,6 +153,7 @@ impl LintStore {
pre_expansion_passes: Some(vec![]),
early_passes: Some(vec![]),
late_passes: Some(vec![]),
late_module_passes: Some(vec![]),
by_name: Default::default(),
future_incompatible: Default::default(),
lint_groups: Default::default(),
Expand Down Expand Up @@ -199,9 +203,14 @@ impl LintStore {
pub fn register_late_pass(&mut self,
sess: Option<&Session>,
from_plugin: bool,
per_module: bool,
pass: LateLintPassObject) {
self.push_pass(sess, from_plugin, &pass);
self.late_passes.as_mut().unwrap().push(pass);
if per_module {
self.late_module_passes.as_mut().unwrap().push(pass);
} else {
self.late_passes.as_mut().unwrap().push(pass);
}
}

// Helper method for register_early/late_pass
Expand Down Expand Up @@ -508,6 +517,7 @@ pub struct LateContext<'a, 'tcx: 'a> {
pub tcx: TyCtxt<'a, 'tcx, 'tcx>,

/// Side-tables for the body we are in.
// FIXME: Make this lazy to avoid running the TypeckTables query?
pub tables: &'a ty::TypeckTables<'tcx>,

/// Parameter environment for the item we are in.
Expand All @@ -523,6 +533,9 @@ pub struct LateContext<'a, 'tcx: 'a> {

/// Generic type parameters in scope for the item we are in.
pub generics: Option<&'tcx hir::Generics>,

/// We are only looking at one module
only_module: bool,
}

/// Context for lint checking of the AST, after expansion, before lowering to
Expand Down Expand Up @@ -803,6 +816,12 @@ impl<'a, 'tcx> LateContext<'a, 'tcx> {
pub fn current_lint_root(&self) -> hir::HirId {
self.last_node_with_lint_attrs
}

fn process_mod(&mut self, m: &'tcx hir::Mod, s: Span, n: hir::HirId) {
run_lints!(self, check_mod, m, s, n);
hir_visit::walk_mod(self, m, n);
run_lints!(self, check_mod_post, m, s, n);
}
}

impl<'a, 'tcx> LayoutOf for LateContext<'a, 'tcx> {
Expand Down Expand Up @@ -934,9 +953,9 @@ impl<'a, 'tcx> hir_visit::Visitor<'tcx> for LateContext<'a, 'tcx> {
}

fn visit_mod(&mut self, m: &'tcx hir::Mod, s: Span, n: hir::HirId) {
run_lints!(self, check_mod, m, s, n);
hir_visit::walk_mod(self, m, n);
run_lints!(self, check_mod_post, m, s, n);
if !self.only_module {
self.process_mod(m, s, n);
}
}

fn visit_local(&mut self, l: &'tcx hir::Local) {
Expand Down Expand Up @@ -1203,11 +1222,48 @@ impl<'a, T: EarlyLintPass> ast_visit::Visitor<'a> for EarlyContextAndPass<'a, T>
}
}

pub fn lint_mod<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, module_def_id: DefId) {
let access_levels = &tcx.privacy_access_levels(LOCAL_CRATE);

/// Performs lint checking on a crate.
///
/// Consumes the `lint_store` field of the `Session`.
pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
let store = &tcx.sess.lint_store;
let passes = store.borrow_mut().late_module_passes.take();
michaelwoerister marked this conversation as resolved.
Show resolved Hide resolved

let mut cx = LateContext {
tcx,
tables: &ty::TypeckTables::empty(None),
param_env: ty::ParamEnv::empty(),
access_levels,
lint_sess: LintSession {
lints: store.borrow(),
passes,
},
last_node_with_lint_attrs: tcx.hir().as_local_hir_id(module_def_id).unwrap(),
generics: None,
only_module: true,
};

let (module, span, hir_id) = tcx.hir().get_module(module_def_id);
cx.process_mod(module, span, hir_id);

// Visit the crate attributes
if hir_id == hir::CRATE_HIR_ID {
walk_list!(cx, visit_attribute, cx.tcx.hir().attrs_by_hir_id(hir::CRATE_HIR_ID));
}

// Put the lint store levels and passes back in the session.
let passes = cx.lint_sess.passes;
drop(cx.lint_sess.lints);
store.borrow_mut().late_module_passes = passes;
}

pub(crate) fn provide(providers: &mut Providers<'_>) {
*providers = Providers {
lint_mod,
..*providers
};
}

fn lint_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
let access_levels = &tcx.privacy_access_levels(LOCAL_CRATE);

let krate = tcx.hir().krate();
Expand All @@ -1225,6 +1281,7 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
},
last_node_with_lint_attrs: hir::CRATE_HIR_ID,
generics: None,
only_module: false,
};

// Visit the whole crate.
Expand All @@ -1244,6 +1301,17 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
tcx.sess.lint_store.borrow_mut().late_passes = passes;
}

/// Performs lint checking on a crate.
pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
// Run per-module lints
for &module in tcx.hir().krate().modules.keys() {
tcx.ensure().lint_mod(tcx.hir().local_def_id(module));
}

// Run whole crate non-incremental lints
lint_crate(tcx);
}

struct EarlyLintPassObjects<'a> {
lints: &'a mut [EarlyLintPassObject],
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,7 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for LintLevelMapBuilder<'a, 'tcx> {

pub fn provide(providers: &mut Providers<'_>) {
providers.lint_levels = lint_levels;
context::provide(providers);
}

/// Returns whether `span` originates in a foreign crate's external macro.
Expand Down
2 changes: 2 additions & 0 deletions src/librustc/ty/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ rustc_query_append! { [define_queries!][ <'tcx>
},

Other {
[] fn lint_mod: LintMod(DefId) -> (),

/// Checks the attributes in the module
[] fn check_mod_attrs: CheckModAttrs(DefId) -> (),

Expand Down
1 change: 1 addition & 0 deletions src/librustc/ty/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,7 @@ pub fn force_from_dep_node<'tcx>(
DepKind::MirBorrowCheck => { force!(mir_borrowck, def_id!()); }
DepKind::UnsafetyCheckResult => { force!(unsafety_check_result, def_id!()); }
DepKind::UnsafeDeriveOnReprPacked => { force!(unsafe_derive_on_repr_packed, def_id!()); }
DepKind::LintMod => { force!(lint_mod, def_id!()); }
DepKind::CheckModAttrs => { force!(check_mod_attrs, def_id!()); }
DepKind::CheckModLoops => { force!(check_mod_loops, def_id!()); }
DepKind::CheckModUnstableApiUsage => { force!(check_mod_unstable_api_usage, def_id!()); }
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_interface/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ pub fn register_plugins<'a>(
ls.register_early_pass(Some(sess), true, false, pass);
}
for pass in late_lint_passes {
ls.register_late_pass(Some(sess), true, pass);
ls.register_late_pass(Some(sess), true, false, pass);
}

for (name, (to, deprecated_name)) in lint_groups {
Expand Down
1 change: 1 addition & 0 deletions src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1360,6 +1360,7 @@ fn check_const(cx: &LateContext<'_, '_>, body_id: hir::BodyId) {
promoted: None
};
// trigger the query once for all constants since that will already report the errors
// FIXME: Use ensure here
let _ = cx.tcx.const_eval(param_env.and(cid));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that this regression is related to clean not running the const_eval call here.

}

Expand Down
55 changes: 45 additions & 10 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,37 +125,72 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
store.register_early_pass(sess, false, true, box BuiltinCombinedEarlyLintPass::new());
}

late_lint_methods!(declare_combined_late_lint_pass, [BuiltinCombinedLateLintPass, [
late_lint_methods!(declare_combined_late_lint_pass, [BuiltinCombinedModuleLateLintPass, [
HardwiredLints: HardwiredLints,
WhileTrue: WhileTrue,
ImproperCTypes: ImproperCTypes,
VariantSizeDifferences: VariantSizeDifferences,
BoxPointers: BoxPointers,
UnusedAttributes: UnusedAttributes,
PathStatements: PathStatements,

// Depends on referenced function signatures in expressions
UnusedResults: UnusedResults,
NonSnakeCase: NonSnakeCase,

NonUpperCaseGlobals: NonUpperCaseGlobals,
NonShorthandFieldPatterns: NonShorthandFieldPatterns,
UnusedAllocation: UnusedAllocation,

// Depends on types used in type definitions
MissingCopyImplementations: MissingCopyImplementations,
UnstableFeatures: UnstableFeatures,
InvalidNoMangleItems: InvalidNoMangleItems,

PluginAsLibrary: PluginAsLibrary,

// Depends on referenced function signatures in expressions
MutableTransmutes: MutableTransmutes,

// Depends on types of fields, checks if they implement Drop
UnionsWithDropFields: UnionsWithDropFields,
UnreachablePub: UnreachablePub,
UnnameableTestItems: UnnameableTestItems::new(),

TypeAliasBounds: TypeAliasBounds,
UnusedBrokenConst: UnusedBrokenConst,

TrivialConstraints: TrivialConstraints,
TypeLimits: TypeLimits::new(),

NonSnakeCase: NonSnakeCase,
InvalidNoMangleItems: InvalidNoMangleItems,

// Depends on access levels
UnreachablePub: UnreachablePub,

ExplicitOutlivesRequirements: ExplicitOutlivesRequirements,
]], ['tcx]);

store.register_late_pass(sess, false, true, box BuiltinCombinedModuleLateLintPass::new());

late_lint_methods!(declare_combined_late_lint_pass, [BuiltinCombinedLateLintPass, [
// FIXME: Look into regression when this is used as a module lint
// May Depend on constants elsewhere
UnusedBrokenConst: UnusedBrokenConst,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I narrow down the regression to this lint, and moving that to a non-module pass should get rid of it.


// Uses attr::is_used which is untracked, can't be an incremental module pass.
UnusedAttributes: UnusedAttributes,

// Needs to run after UnusedAttributes as it marks all `feature` attributes as used.
UnstableFeatures: UnstableFeatures,

// Tracks state across modules
UnnameableTestItems: UnnameableTestItems::new(),

// Tracks attributes of parents
MissingDoc: MissingDoc::new(),

// Depends on access levels
// FIXME: Turn the computation of types which implement Debug into a query
// and change this to a module lint pass
MissingDebugImplementations: MissingDebugImplementations::new(),
ExplicitOutlivesRequirements: ExplicitOutlivesRequirements,
]], ['tcx]);

store.register_late_pass(sess, false, box BuiltinCombinedLateLintPass::new());
store.register_late_pass(sess, false, false, box BuiltinCombinedLateLintPass::new());

add_lint_group!(sess,
"nonstandard_style",
Expand Down
8 changes: 6 additions & 2 deletions src/librustc_lint/nonstandard_style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,11 +267,15 @@ impl LintPass for NonSnakeCase {
}

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NonSnakeCase {
fn check_crate(&mut self, cx: &LateContext<'_, '_>, cr: &hir::Crate) {
fn check_mod(&mut self, cx: &LateContext<'_, '_>, _: &'tcx hir::Mod, _: Span, id: hir::HirId) {
if id != hir::CRATE_HIR_ID {
return;
}

let crate_ident = if let Some(name) = &cx.tcx.sess.opts.crate_name {
Some(Ident::from_str(name))
} else {
attr::find_by_name(&cr.attrs, "crate_name")
attr::find_by_name(&cx.tcx.hir().attrs_by_hir_id(hir::CRATE_HIR_ID), "crate_name")
.and_then(|attr| attr.meta())
.and_then(|meta| {
meta.name_value_literal().and_then(|lit| {
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_privacy/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1790,8 +1790,7 @@ fn check_mod_privacy<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, module_def_id: DefId) {
current_item: hir::DUMMY_HIR_ID,
empty_tables: &empty_tables,
};
let (module, span, node_id) = tcx.hir().get_module(module_def_id);
let hir_id = tcx.hir().node_to_hir_id(node_id);
let (module, span, hir_id) = tcx.hir().get_module(module_def_id);
intravisit::walk_mod(&mut visitor, module, hir_id);

// Check privacy of explicitly written types and traits as well as
Expand Down
Loading