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

Clippy: Move some attribute lints to be early pass (post expansion) #132598

Merged
merged 1 commit into from
Nov 5, 2024
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
4 changes: 2 additions & 2 deletions src/tools/clippy/clippy_lints/src/attrs/allow_attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::is_from_proc_macro;
use rustc_ast::{AttrStyle, Attribute};
use rustc_errors::Applicability;
use rustc_lint::{LateContext, LintContext};
use rustc_lint::{EarlyContext, LintContext};
use rustc_middle::lint::in_external_macro;

// Separate each crate's features.
pub fn check<'cx>(cx: &LateContext<'cx>, attr: &'cx Attribute) {
pub fn check<'cx>(cx: &EarlyContext<'cx>, attr: &'cx Attribute) {
if !in_external_macro(cx.sess(), attr.span)
&& let AttrStyle::Outer = attr.style
&& let Some(ident) = attr.ident()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ use super::{ALLOW_ATTRIBUTES_WITHOUT_REASON, Attribute};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::is_from_proc_macro;
use rustc_ast::{MetaItemInner, MetaItemKind};
use rustc_lint::{LateContext, LintContext};
use rustc_lint::{EarlyContext, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_span::sym;
use rustc_span::symbol::Symbol;

pub(super) fn check<'cx>(cx: &LateContext<'cx>, name: Symbol, items: &[MetaItemInner], attr: &'cx Attribute) {
pub(super) fn check<'cx>(cx: &EarlyContext<'cx>, name: Symbol, items: &[MetaItemInner], attr: &'cx Attribute) {
// Check if the reason is present
if let Some(item) = items.last().and_then(MetaItemInner::meta_item)
&& let MetaItemKind::NameValue(_) = &item.kind
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ use super::BLANKET_CLIPPY_RESTRICTION_LINTS;
use super::utils::extract_clippy_lint;
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then};
use rustc_ast::MetaItemInner;
use rustc_lint::{LateContext, Level, LintContext};
use rustc_lint::{EarlyContext, Level, LintContext};
use rustc_span::symbol::Symbol;
use rustc_span::{DUMMY_SP, sym};

pub(super) fn check(cx: &LateContext<'_>, name: Symbol, items: &[MetaItemInner]) {
pub(super) fn check(cx: &EarlyContext<'_>, name: Symbol, items: &[MetaItemInner]) {
for lint in items {
if let Some(lint_name) = extract_clippy_lint(lint) {
if lint_name.as_str() == "restriction" && name != sym::allow {
Expand All @@ -23,7 +23,7 @@ pub(super) fn check(cx: &LateContext<'_>, name: Symbol, items: &[MetaItemInner])
}
}

pub(super) fn check_command_line(cx: &LateContext<'_>) {
pub(super) fn check_command_line(cx: &EarlyContext<'_>) {
for (name, level) in &cx.sess().opts.lint_opts {
if name == "clippy::restriction" && *level > Level::Allow {
span_lint_and_then(
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/clippy_lints/src/attrs/deprecated_semver.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use super::DEPRECATED_SEMVER;
use clippy_utils::diagnostics::span_lint;
use rustc_ast::{LitKind, MetaItemLit};
use rustc_lint::LateContext;
use rustc_lint::EarlyContext;
use rustc_span::Span;
use semver::Version;

pub(super) fn check(cx: &LateContext<'_>, span: Span, lit: &MetaItemLit) {
pub(super) fn check(cx: &EarlyContext<'_>, span: Span, lit: &MetaItemLit) {
if let LitKind::Str(is, _) = lit.kind {
if is.as_str() == "TBD" || Version::parse(is.as_str()).is_ok() {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ use super::DUPLICATED_ATTRIBUTES;
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_ast::{Attribute, MetaItem};
use rustc_data_structures::fx::FxHashMap;
use rustc_lint::LateContext;
use rustc_lint::EarlyContext;
use rustc_span::{Span, sym};
use std::collections::hash_map::Entry;

fn emit_if_duplicated(
cx: &LateContext<'_>,
cx: &EarlyContext<'_>,
attr: &MetaItem,
attr_paths: &mut FxHashMap<String, Span>,
complete_path: String,
Expand All @@ -26,7 +26,7 @@ fn emit_if_duplicated(
}

fn check_duplicated_attr(
cx: &LateContext<'_>,
cx: &EarlyContext<'_>,
attr: &MetaItem,
attr_paths: &mut FxHashMap<String, Span>,
parent: &mut Vec<String>,
Expand Down Expand Up @@ -65,7 +65,7 @@ fn check_duplicated_attr(
}
}

pub fn check(cx: &LateContext<'_>, attrs: &[Attribute]) {
pub fn check(cx: &EarlyContext<'_>, attrs: &[Attribute]) {
let mut attr_paths = FxHashMap::default();

for attr in attrs {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use clippy_utils::diagnostics::span_lint;
use rustc_ast::{AttrKind, AttrStyle, Attribute};
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::sync::Lrc;
use rustc_lint::{LateContext, LintContext};
use rustc_lint::{EarlyContext, LintContext};
use rustc_span::source_map::SourceMap;
use rustc_span::{SourceFile, Span, Symbol};

Expand Down Expand Up @@ -32,7 +32,7 @@ impl From<&AttrKind> for SimpleAttrKind {
}
}

pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute]) {
pub(super) fn check(cx: &EarlyContext<'_>, item_span: Span, attrs: &[Attribute]) {
let mut inner_attr_kind: FxHashSet<SimpleAttrKind> = FxHashSet::default();
let mut outer_attr_kind: FxHashSet<SimpleAttrKind> = FxHashSet::default();

Expand Down Expand Up @@ -64,7 +64,7 @@ pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute])
}
}

fn lint_mixed_attrs(cx: &LateContext<'_>, attrs: &[Attribute]) {
fn lint_mixed_attrs(cx: &EarlyContext<'_>, attrs: &[Attribute]) {
let mut attrs_iter = attrs.iter().filter(|attr| !attr.span.from_expansion());
let span = if let (Some(first), Some(last)) = (attrs_iter.next(), attrs_iter.last()) {
first.span.with_hi(last.span.hi())
Expand Down
128 changes: 76 additions & 52 deletions src/tools/clippy/clippy_lints/src/attrs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ mod utils;

use clippy_config::Conf;
use clippy_config::msrvs::{self, Msrv};
use rustc_ast::{Attribute, MetaItemInner, MetaItemKind};
use rustc_hir::{ImplItem, Item, ItemKind, TraitItem};
use rustc_ast::{Attribute, MetaItemInner, MetaItemKind, self as ast};
use rustc_hir::{ImplItem, Item, TraitItem};
use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::sym;
Expand Down Expand Up @@ -414,15 +414,7 @@ pub struct Attributes {
}

impl_lint_pass!(Attributes => [
ALLOW_ATTRIBUTES,
ALLOW_ATTRIBUTES_WITHOUT_REASON,
INLINE_ALWAYS,
DEPRECATED_SEMVER,
USELESS_ATTRIBUTE,
BLANKET_CLIPPY_RESTRICTION_LINTS,
SHOULD_PANIC_WITHOUT_EXPECT,
MIXED_ATTRIBUTES_STYLE,
DUPLICATED_ATTRIBUTES,
]);

impl Attributes {
Expand All @@ -434,53 +426,11 @@ impl Attributes {
}

impl<'tcx> LateLintPass<'tcx> for Attributes {
fn check_crate(&mut self, cx: &LateContext<'tcx>) {
blanket_clippy_restriction_lints::check_command_line(cx);
duplicated_attributes::check(cx, cx.tcx.hir().krate_attrs());
}

fn check_attribute(&mut self, cx: &LateContext<'tcx>, attr: &'tcx Attribute) {
if let Some(items) = &attr.meta_item_list() {
if let Some(ident) = attr.ident() {
if is_lint_level(ident.name, attr.id) {
blanket_clippy_restriction_lints::check(cx, ident.name, items);
}
if matches!(ident.name, sym::allow) && self.msrv.meets(msrvs::LINT_REASONS_STABILIZATION) {
allow_attributes::check(cx, attr);
}
if matches!(ident.name, sym::allow | sym::expect) && self.msrv.meets(msrvs::LINT_REASONS_STABILIZATION)
{
allow_attributes_without_reason::check(cx, ident.name, items, attr);
}
if items.is_empty() || !attr.has_name(sym::deprecated) {
return;
}
for item in items {
if let MetaItemInner::MetaItem(mi) = &item
&& let MetaItemKind::NameValue(lit) = &mi.kind
&& mi.has_name(sym::since)
{
deprecated_semver::check(cx, item.span(), lit);
}
}
}
}
if attr.has_name(sym::should_panic) {
should_panic_without_expect::check(cx, attr);
}
}

fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
let attrs = cx.tcx.hir().attrs(item.hir_id());
if is_relevant_item(cx, item) {
inline_always::check(cx, item.span, item.ident.name, attrs);
}
match item.kind {
ItemKind::ExternCrate(..) | ItemKind::Use(..) => useless_attribute::check(cx, item, attrs),
_ => {},
}
mixed_attributes_style::check(cx, item.span, attrs);
duplicated_attributes::check(cx, attrs);
}

fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {
Expand Down Expand Up @@ -526,3 +476,77 @@ impl EarlyLintPass for EarlyAttributes {

extract_msrv_attr!(EarlyContext);
}

pub struct PostExpansionEarlyAttributes {
msrv: Msrv,
}

impl PostExpansionEarlyAttributes {
pub fn new(conf: &'static Conf) -> Self {
Self {
msrv: conf.msrv.clone(),
}
}
}

impl_lint_pass!(PostExpansionEarlyAttributes => [
ALLOW_ATTRIBUTES,
ALLOW_ATTRIBUTES_WITHOUT_REASON,
DEPRECATED_SEMVER,
USELESS_ATTRIBUTE,
BLANKET_CLIPPY_RESTRICTION_LINTS,
SHOULD_PANIC_WITHOUT_EXPECT,
MIXED_ATTRIBUTES_STYLE,
DUPLICATED_ATTRIBUTES,
]);

impl EarlyLintPass for PostExpansionEarlyAttributes {
fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &ast::Crate) {
blanket_clippy_restriction_lints::check_command_line(cx);
duplicated_attributes::check(cx, &krate.attrs);
}

fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &Attribute) {
if let Some(items) = &attr.meta_item_list() {
if let Some(ident) = attr.ident() {
if matches!(ident.name, sym::allow) && self.msrv.meets(msrvs::LINT_REASONS_STABILIZATION) {
allow_attributes::check(cx, attr);
}
if matches!(ident.name, sym::allow | sym::expect) && self.msrv.meets(msrvs::LINT_REASONS_STABILIZATION)
{
allow_attributes_without_reason::check(cx, ident.name, items, attr);
}
if is_lint_level(ident.name, attr.id) {
blanket_clippy_restriction_lints::check(cx, ident.name, items);
}
if items.is_empty() || !attr.has_name(sym::deprecated) {
return;
}
for item in items {
if let MetaItemInner::MetaItem(mi) = &item
&& let MetaItemKind::NameValue(lit) = &mi.kind
&& mi.has_name(sym::since)
{
deprecated_semver::check(cx, item.span(), lit);
}
}
}
}

if attr.has_name(sym::should_panic) {
should_panic_without_expect::check(cx, attr);
}
}

fn check_item(&mut self, cx: &EarlyContext<'_>, item: &'_ ast::Item) {
match item.kind {
ast::ItemKind::ExternCrate(..) | ast::ItemKind::Use(..) => useless_attribute::check(cx, item, &item.attrs),
_ => {},
}

mixed_attributes_style::check(cx, item.span, &item.attrs);
duplicated_attributes::check(cx, &item.attrs);
}

extract_msrv_attr!(EarlyContext);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ use rustc_ast::token::{Token, TokenKind};
use rustc_ast::tokenstream::TokenTree;
use rustc_ast::{AttrArgs, AttrArgsEq, AttrKind};
use rustc_errors::Applicability;
use rustc_lint::LateContext;
use rustc_lint::EarlyContext;
use rustc_span::sym;

pub(super) fn check(cx: &LateContext<'_>, attr: &Attribute) {
pub(super) fn check(cx: &EarlyContext<'_>, attr: &Attribute) {
if let AttrKind::Normal(normal_attr) = &attr.kind {
if let AttrArgs::Eq(_, AttrArgsEq::Hir(_)) = &normal_attr.item.args {
if let AttrArgs::Eq(_, AttrArgsEq::Ast(_)) = &normal_attr.item.args {
// `#[should_panic = ".."]` found, good
return;
}
Expand Down
8 changes: 4 additions & 4 deletions src/tools/clippy/clippy_lints/src/attrs/useless_attribute.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
use super::utils::{extract_clippy_lint, is_lint_level, is_word};
use super::{Attribute, USELESS_ATTRIBUTE};
use super::USELESS_ATTRIBUTE;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::{SpanRangeExt, first_line_of_span};
use rustc_ast::MetaItemInner;
use rustc_errors::Applicability;
use rustc_hir::{Item, ItemKind};
use rustc_lint::{LateContext, LintContext};
use rustc_ast::{Item, ItemKind, Attribute};
use rustc_lint::{EarlyContext, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_span::sym;

pub(super) fn check(cx: &LateContext<'_>, item: &Item<'_>, attrs: &[Attribute]) {
pub(super) fn check(cx: &EarlyContext<'_>, item: &Item, attrs: &[Attribute]) {
let skip_unused_imports = attrs.iter().any(|attr| attr.has_name(sym::macro_use));

for attr in attrs {
Expand Down
2 changes: 2 additions & 0 deletions src/tools/clippy/clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,8 @@ use rustc_lint::{Lint, LintId};
pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
// NOTE: Do not add any more pre-expansion passes. These should be removed eventually.
store.register_pre_expansion_pass(move || Box::new(attrs::EarlyAttributes::new(conf)));

store.register_early_pass(move || Box::new(attrs::PostExpansionEarlyAttributes::new(conf)));
}

#[derive(Default)]
Expand Down
5 changes: 3 additions & 2 deletions src/tools/clippy/clippy_utils/src/check_proc_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use rustc_hir::{
ImplItem, ImplItemKind, IsAuto, Item, ItemKind, Lit, LoopSource, MatchSource, MutTy, Node, Path, QPath, Safety,
TraitItem, TraitItemKind, Ty, TyKind, UnOp, UnsafeSource, Variant, VariantData, YieldSource,
};
use rustc_lint::{LateContext, LintContext};
use rustc_lint::{LateContext, LintContext, EarlyContext};
use rustc_middle::ty::TyCtxt;
use rustc_session::Session;
use rustc_span::symbol::{Ident, kw};
Expand Down Expand Up @@ -429,11 +429,12 @@ impl_with_search_pat!((_cx: LateContext<'tcx>, self: ImplItem<'_>) => impl_item_
impl_with_search_pat!((_cx: LateContext<'tcx>, self: FieldDef<'_>) => field_def_search_pat(self));
impl_with_search_pat!((_cx: LateContext<'tcx>, self: Variant<'_>) => variant_search_pat(self));
impl_with_search_pat!((_cx: LateContext<'tcx>, self: Ty<'_>) => ty_search_pat(self));
impl_with_search_pat!((_cx: LateContext<'tcx>, self: Attribute) => attr_search_pat(self));
impl_with_search_pat!((_cx: LateContext<'tcx>, self: Ident) => ident_search_pat(*self));
impl_with_search_pat!((_cx: LateContext<'tcx>, self: Lit) => lit_search_pat(&self.node));
impl_with_search_pat!((_cx: LateContext<'tcx>, self: Path<'_>) => path_search_pat(self));

impl_with_search_pat!((_cx: EarlyContext<'tcx>, self: Attribute) => attr_search_pat(self));

impl<'cx> WithSearchPat<'cx> for (&FnKind<'cx>, &Body<'cx>, HirId, Span) {
type Context = LateContext<'cx>;

Expand Down
10 changes: 1 addition & 9 deletions src/tools/clippy/tests/ui/allow_attributes.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,5 @@ error: #[allow] attribute found
LL | #[allow(unused)]
| ^^^^^ help: replace it with: `expect`

error: #[allow] attribute found
--> tests/ui/allow_attributes.rs:52:7
|
LL | #[allow(unused)]
| ^^^^^ help: replace it with: `expect`
|
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 4 previous errors
jdonszelmann marked this conversation as resolved.
Show resolved Hide resolved
error: aborting due to 3 previous errors

11 changes: 1 addition & 10 deletions src/tools/clippy/tests/ui/allow_attributes_without_reason.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,5 @@ LL | #[allow(unused)]
|
= help: try adding a reason at the end with `, reason = ".."`

error: `allow` attribute without specifying a reason
--> tests/ui/allow_attributes_without_reason.rs:46:5
|
LL | #[allow(unused)]
| ^^^^^^^^^^^^^^^^
|
= help: try adding a reason at the end with `, reason = ".."`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: aborting due to 6 previous errors
error: aborting due to 5 previous errors

Loading
Loading