Add relative_path_in_macro_definition lint (#14472) #14645

[KunWuChan]

Add relative_path_in_macro_definition lint (#14472) #14645

Implements the relative_path_in_macro_definition lint to warn about relative paths to core or kernel in macro definitions, as requested in #14472.
Includes tests in tests/ui/relative_path_in_macro_definition.rs and updates existing tests (incompatible_msrv.rs, swap_ptr_to_ref_unfixable.rs, arithmetic_side_effects.rs) with #![allow] to avoid interference.

Fixes New lint: relative_path_in_macro_definition #14472
Tests pass: cargo test, cargo test --test compile-test, cargo test --test integration
cargo fmt clean

changelog: add 'relative_path_in_macro_definition' lint.

Signed-off-by: Kunwu Chan [email protected]
Co-developed-by: Grace Deng [email protected]
Signed-off-by: Grace Deng [email protected]

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[llogiq]

This is a good starting point. I'd like the message to be more succinct and relate to the current situation though.

[llogiq]

We no longer need ident.as_str(). Look at clippy_utils::sym.

[KunWuChan]

Thank you for your feedback and suggestion to avoid ident.as_str() and refer to clippy_utils::sym. I've encountered a compilation error with the current implementation:
error[E0425]: cannot find value kernel in module sym
--> clippy_lints/src/relative_path_in_macro_definition.rs:57:61
|
57 | && (ident == sym::core || ident == sym::kernel)
| ^^^^^^ not found in sym

The issue arises because rustc_span::symbol::sym does not define a kernel symbol, unlike core. Upon reviewing clippy_utils::sym, I found it to be a macro that wraps Symbol::intern, used for creating symbols from strings, rather than providing predefined symbols like core.
To address this, I propose using Symbol::intern("kernel") to create the kernel symbol dynamically, allowing direct comparison with ident (of type rustc_span::Ident). The updated code snippet is:
if ident == sym::core || ident == Symbol::intern("kernel") {
// ... rest of the lint logic ...
}

This approach avoids ident.as_str() as you recommended, using direct symbol comparisons for efficiency. I've tested this change locally, and it resolves the compilation error while maintaining the lint's functionality to check for relative paths to core and kernel in macro definitions.
Could you please confirm if using Symbol::intern("kernel") is acceptable for this lint, or if there's a preferred method within Clippy's codebase (e.g., using clippy_utils::sym! macro or another approach)? Thank you for your guidance!

[llogiq]

Yes, you can simply add "kernel" into that generate! macro call, then you can use clippy_utils::sym::kernel in your code.

[KunWuChan]

Yes, you can simply add "kernel" into that generate! macro call, then you can use clippy_utils::sym::kernel in your code.

It seems that the definition of "sym::core" is in
https://github.com/rust-lang/rust/blob/master/compiler/rustc_span/src/symbol.rs.
But according to the discussion below, we no longer need to add the extra check. Anyway, thanks for the reply.

[llogiq]

Suggested change

	if let TokenTree::Token(prev_token, _) = prev {
	prev_token.kind == TokenKind::PathSep
	} else {
	false
	}
	matches!(prev, TokenTree::Token(Token { kind: TokenKind::PathSep, .. }, _))

[KunWuChan]

thx, I'll change it in v2.

[llogiq]

Here we should reuse the first element of the path instead of writing both options.

[KunWuChan]

thx, I'll change it in v2.

[rustbot]

☔ The latest upstream changes (possibly 7bac114) made this pull request unmergeable. Please resolve the merge conflicts.

[smoelius]

Sorry to intrude, but can I ask why core and kernel are the focus? Wouldn't it make sense to warn about any relative path in a macro definition?

[KunWuChan]

Sorry to intrude, but can I ask why core and kernel are the focus? Wouldn't it make sense to warn about any relative path in a macro definition?

The source is here:
Rust-for-Linux/linux#1150

[smoelius]

The source is here: Rust-for-Linux/linux#1150

Thank you, @KunWuChan. It would be nice if eventually the lint could be extended as @ojeda wrote in Rust-for-Linux/linux#1150 (comment):

Sounds like a good idea -- in general it would be nice to do that for all crates (or at least non-leaf ones) that we are aware, i.e. I don't think we will want modules named the same way as crates -- it can get confusing fast.

I think many besides kernel developers would benefit from a lint like that.

[llogiq]

I agree that there is nothing in the issue about the lint that should restrict which paths must be relative. And kernel is not even part of the standard library, if we wanted to restrict the lint to that, we'd need core, std and alloc instead.

So let's simply remove that additional check and make the lint more general.

[ojeda]

Yeah, we expected the lint to be general, rather than just core and kernel (i.e. those two crates were mentioned in the kernel-side issue since some cleanups were expected in macros that referred to those, but it applies to other paths, of course).

Thanks for this!

[KunWuChan]

The source is here: Rust-for-Linux/linux#1150

Thank you, @KunWuChan. It would be nice if eventually the lint could be extended as @ojeda wrote in Rust-for-Linux/linux#1150 (comment):

Sounds like a good idea -- in general it would be nice to do that for all crates (or at least non-leaf ones) that we are aware, i.e. I don't think we will want modules named the same way as crates -- it can get confusing fast.

I think many besides kernel developers would benefit from a lint like that.

Sure, I'll try to do it as the suggestions.

I agree that there is nothing in the issue about the lint that should restrict which paths must be relative. And kernel is not even part of the standard library, if we wanted to restrict the lint to that, we'd need core, std and alloc instead.

So let's simply remove that additional check and make the lint more general.

thx, I'll remove the additional check and make the lint more general.

[KunWuChan]

Yeah, we expected the lint to be general, rather than just core and kernel (i.e. those two crates were mentioned in the kernel-side issue since some cleanups were expected in macros that referred to those, but it applies to other paths, of course).

Thanks for this!

Okay, I'll remove the additional check and add some testcases.