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.

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

Do not lint use_self in proc macro expansion #9454

Merged
merged 2 commits into from
Sep 13, 2022
Merged

Do not lint use_self in proc macro expansion #9454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c433d54
Do not lint `use_self` in proc macro expansion
kraktus Sep 10, 2022
59ee6a8
Remove duplicate context check
kraktus Sep 10, 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
8 changes: 3 additions & 5 deletions clippy_lints/src/use_self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::ty::same_type_and_consts;
use clippy_utils::{meets_msrv, msrvs};
use clippy_utils::{is_from_proc_macro, meets_msrv, msrvs};
use if_chain::if_chain;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -87,7 +87,7 @@ impl_lint_pass!(UseSelf => [USE_SELF]);
const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element";

impl<'tcx> LateLintPass<'tcx> for UseSelf {
fn check_item(&mut self, _cx: &LateContext<'_>, item: &Item<'_>) {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &Item<'tcx>) {
if matches!(item.kind, ItemKind::OpaqueTy(_)) {
// skip over `ItemKind::OpaqueTy` in order to lint `foo() -> impl <..>`
return;
Expand All @@ -103,6 +103,7 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
if parameters.as_ref().map_or(true, |params| {
!params.parenthesized && !params.args.iter().any(|arg| matches!(arg, GenericArg::Lifetime(_)))
});
if !is_from_proc_macro(cx, item); // expensive, should be last check
Copy link
Member

Choose a reason for hiding this comment

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

Would item_path.span.from_expansion() be enough here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tried testing it, and from_expansion still produces the false positive.

Copy link
Member

Choose a reason for hiding this comment

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

huh, what about for item.span?

Copy link
Contributor

@Jarcho Jarcho Sep 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The issue is the proc macro using a real span instead of making a new one. Basically every lint can be triggered from a proc macro, but with the span of the item being linted coming from a token outside the proc macro.

Copy link
Member

Choose a reason for hiding this comment

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

The path span happened to do that but the whole item doesn't, so checking its span should be fine

Copy link
Member

Choose a reason for hiding this comment

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

It depends what span the proc macro sets. If it doesn't set a span with expansion info, but just uses the span given from the item/tokens it looks to clippy as if it is unexpanded code.

then {
StackItem::Check {
impl_id: item.def_id,
Expand Down Expand Up @@ -213,9 +214,6 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
hir_ty_to_ty(cx.tcx, hir_ty)
};
if same_type_and_consts(ty, cx.tcx.type_of(impl_id));
let hir = cx.tcx.hir();
// prevents false positive on `#[derive(serde::Deserialize)]`
if !hir.span(hir.get_parent_node(hir_ty.hir_id)).in_derive_expansion();
then {
span_lint(cx, hir_ty.span);
}
Expand Down
9 changes: 9 additions & 0 deletions tests/ui/use_self.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,3 +608,12 @@ mod issue8845 {
}
}
}

mod issue6902 {
use serde::Serialize;

#[derive(Serialize)]
pub enum Foo {
Bar = 1,
}
}
9 changes: 9 additions & 0 deletions tests/ui/use_self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,3 +608,12 @@ mod issue8845 {
}
}
}

mod issue6902 {
use serde::Serialize;

#[derive(Serialize)]
pub enum Foo {
Bar = 1,
}
}