Skip to content

Fix MissingDoc quadratic behaviour #98153

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 18, 2022
Merged

Fix MissingDoc quadratic behaviour #98153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
969a2cc
Fix quadratic behaviour in the `MissingDoc` lint.
nnethercote Jun 15, 2022
c9e9725
Remove unused `hir_id` arg from `visit_attribute`.
nnethercote Jun 15, 2022
be45f10
Inline and remove `{enter,exit}_attrs` functions.
nnethercote Jun 15, 2022
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
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/intravisit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ pub trait Visitor<'v>: Sized {
fn visit_assoc_type_binding(&mut self, type_binding: &'v TypeBinding<'v>) {
walk_assoc_type_binding(self, type_binding)
}
fn visit_attribute(&mut self, _id: HirId, _attr: &'v Attribute) {}
fn visit_attribute(&mut self, _attr: &'v Attribute) {}
fn visit_associated_item_kind(&mut self, kind: &'v AssocItemKind) {
walk_associated_item_kind(self, kind);
}
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_incremental/src/persist/dirty_clean.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

use rustc_ast::{self as ast, Attribute, NestedMetaItem};
use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit;
use rustc_hir::Node as HirNode;
Expand Down Expand Up @@ -473,7 +472,7 @@ impl<'tcx> intravisit::Visitor<'tcx> for FindAllAttrs<'tcx> {
self.tcx.hir()
}

fn visit_attribute(&mut self, _: hir::HirId, attr: &'tcx Attribute) {
fn visit_attribute(&mut self, attr: &'tcx Attribute) {
if self.is_active_attr(attr) {
self.found_attrs.push(attr);
}
Expand Down
12 changes: 2 additions & 10 deletions compiler/rustc_lint/src/early.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,20 +63,12 @@ impl<'a, T: EarlyLintPass> EarlyContextAndPass<'a, T> {
let push = self.context.builder.push(attrs, is_crate_node, None);

self.check_id(id);
self.enter_attrs(attrs);
f(self);
self.exit_attrs(attrs);
self.context.builder.pop(push);
}

fn enter_attrs(&mut self, attrs: &'a [ast::Attribute]) {
debug!("early context: enter_attrs({:?})", attrs);
run_early_pass!(self, enter_lint_attrs, attrs);
}

fn exit_attrs(&mut self, attrs: &'a [ast::Attribute]) {
f(self);
debug!("early context: exit_attrs({:?})", attrs);
run_early_pass!(self, exit_lint_attrs, attrs);
self.context.builder.pop(push);
}
}

Expand Down
24 changes: 7 additions & 17 deletions compiler/rustc_lint/src/late.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,11 @@ impl<'tcx, T: LateLintPass<'tcx>> LateContextAndPass<'tcx, T> {
let attrs = self.context.tcx.hir().attrs(id);
let prev = self.context.last_node_with_lint_attrs;
self.context.last_node_with_lint_attrs = id;
self.enter_attrs(attrs);
debug!("late context: enter_attrs({:?})", attrs);
lint_callback!(self, enter_lint_attrs, attrs);
f(self);
self.exit_attrs(attrs);
debug!("late context: exit_attrs({:?})", attrs);
lint_callback!(self, exit_lint_attrs, attrs);
self.context.last_node_with_lint_attrs = prev;
}

Expand All @@ -81,16 +83,6 @@ impl<'tcx, T: LateLintPass<'tcx>> LateContextAndPass<'tcx, T> {
hir_visit::walk_mod(self, m, n);
lint_callback!(self, check_mod_post, m, s, n);
}

fn enter_attrs(&mut self, attrs: &'tcx [ast::Attribute]) {
debug!("late context: enter_attrs({:?})", attrs);
lint_callback!(self, enter_lint_attrs, attrs);
}

fn exit_attrs(&mut self, attrs: &'tcx [ast::Attribute]) {
debug!("late context: exit_attrs({:?})", attrs);
lint_callback!(self, exit_lint_attrs, attrs);
}
}

impl<'tcx, T: LateLintPass<'tcx>> hir_visit::Visitor<'tcx> for LateContextAndPass<'tcx, T> {
Expand Down Expand Up @@ -337,10 +329,8 @@ impl<'tcx, T: LateLintPass<'tcx>> hir_visit::Visitor<'tcx> for LateContextAndPas
hir_visit::walk_path(self, p);
}

fn visit_attribute(&mut self, hir_id: hir::HirId, attr: &'tcx ast::Attribute) {
self.with_lint_attrs(hir_id, |cx| {
lint_callback!(cx, check_attribute, attr);
})
fn visit_attribute(&mut self, attr: &'tcx ast::Attribute) {
lint_callback!(self, check_attribute, attr);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we have an allow(some lint about attributes) followed by an attribute that triggers that lint?
Can we make sure we have the same behaviour whatever the position of those attributes (including expression, patterns...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give a more specific example? I know this change fixes the performance problem and passes all the tests, but I don't know enough about linting to know if there is some edge case that it will miss.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only clippy uses late lints on attributes. They should probably made early lints, and drop support for late lints on attributes.

For instance:

#[warn(macro_use_imports)]
#[macro_use]
extern crate std;

and with swapped attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is attribute order supposed to be significant? Are attributes supposed to apply to other attributes on the same item, or do they just apply to the item? The latter makes more sense to me, but I honestly don't know.

They should probably made early lints, and drop support for late lints on attributes.

Sorry, I don't understand this. Is this an argument in favour of this PR's change, or against it, or is it orthogonal?

I tried your example, with this code (including slight necessary changes):

#[warn(clippy::macro_use_imports)]
#[macro_use]
extern crate std as other_std;

fn main() { }

With a standard rustc (not containing this PR's changes) I get much the same output from cargo clippy regardless of the order of the attributes:

warning: unused `#[macro_use]` import
 --> src/main.rs:2:1
  |
2 | #[macro_use]
  | ^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error: useless lint attribute
 --> src/main.rs:1:1
  |
1 | #[warn(clippy::macro_use_imports)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![warn(clippy::macro_use_imports)]`
  |
  = note: `#[deny(clippy::useless_attribute)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_attribute

If I add the ! as suggested then the error goes away, unsurprisingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is attribute order supposed to be significant? Are attributes supposed to apply to other attributes on the same item, or do they just apply to the item? The latter makes more sense to me, but I honestly don't know.

For now, the order of attributes is not significant. My question is mostly: are we changing behaviour?

They should probably made early lints, and drop support for late lints on attributes.

Sorry, I don't understand this. Is this an argument in favour of this PR's change, or against it, or is it orthogonal?

This is rather orthogonal. I'll post a follow-up issue if you don't mind.

I tried your example, with this code (including slight necessary changes):

#[warn(clippy::macro_use_imports)]
#[macro_use]
extern crate std as other_std;

fn main() { }

With a standard rustc (not containing this PR's changes) I get much the same output from cargo clippy regardless of the order of the attributes:

warning: unused `#[macro_use]` import
 --> src/main.rs:2:1
  |
2 | #[macro_use]
  | ^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error: useless lint attribute
 --> src/main.rs:1:1
  |
1 | #[warn(clippy::macro_use_imports)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![warn(clippy::macro_use_imports)]`
  |
  = note: `#[deny(clippy::useless_attribute)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_attribute

If I add the ! as suggested then the error goes away, unsurprisingly.

So, IIUC, the behaviour does not change. Great! Let's go ahead with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent! Do you want me to do anything else, or is it ready for an r+?

}
}

Expand Down Expand Up @@ -402,7 +392,7 @@ fn late_lint_mod_pass<'tcx, T: LateLintPass<'tcx>>(
// Visit the crate attributes
if hir_id == hir::CRATE_HIR_ID {
for attr in tcx.hir().attrs(hir::CRATE_HIR_ID).iter() {
cx.visit_attribute(hir_id, attr)
cx.visit_attribute(attr)
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_middle/src/hir/map/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,12 +577,11 @@ impl<'hir> Map<'hir> {
/// Walks the attributes in a crate.
pub fn walk_attributes(self, visitor: &mut impl Visitor<'hir>) {
let krate = self.krate();
for (owner, info) in krate.owners.iter_enumerated() {
for info in krate.owners.iter() {
if let MaybeOwner::Owner(info) = info {
for (local_id, attrs) in info.attrs.map.iter() {
let id = HirId { owner, local_id: *local_id };
for attrs in info.attrs.map.values() {
for a in *attrs {
visitor.visit_attribute(id, a)
visitor.visit_attribute(a)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/hir_stats.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ impl<'v> hir_visit::Visitor<'v> for StatCollector<'v> {
hir_visit::walk_assoc_type_binding(self, type_binding)
}

fn visit_attribute(&mut self, _: hir::HirId, attr: &'v ast::Attribute) {
fn visit_attribute(&mut self, attr: &'v ast::Attribute) {
self.record("Attribute", Id::Attr(attr.id), attr);
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/lib_features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ impl<'tcx> Visitor<'tcx> for LibFeatureCollector<'tcx> {
self.tcx.hir()
}

fn visit_attribute(&mut self, _: rustc_hir::HirId, attr: &'tcx Attribute) {
fn visit_attribute(&mut self, attr: &'tcx Attribute) {
if let Some((feature, stable, span)) = self.extract(attr) {
self.collect_feature(feature, stable, span);
}
Expand Down