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

unused_qualifications lints on macro-generated paths which happen to have spans from the input #122519

Closed
kpreid opened this issue Mar 14, 2024 · 10 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. L-unused_qualifications Lint: unused_qualifications P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@kpreid
Copy link
Contributor

kpreid commented Mar 14, 2024

Code

#![warn(unused_qualifications)]

#[derive(Copy, Clone, bytemuck::Pod, bytemuck::Zeroable)]
#[repr(C)]
pub struct Foo {}

Current output

warning: unnecessary qualification
 --> src/lib.rs:5:12
  |
5 | pub struct Foo {}
  |            ^^^
  |
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![warn(unused_qualifications)]
  |         ^^^^^^^^^^^^^^^^^^^^^
help: remove the unnecessary path segments
  |
5 | pub struct Foo {}
  |

Desired output

No warning.

Rationale and extra context

Rationale:

  1. The warning is non-actionable. The suggestion is useless, showing literally zero changed characters.
  2. Proc-macros idiomatically use absolute paths even when not necessary, to maximize their compatibility. In principle, macros could internally #[allow(unused_qualifications)] to solve this problem, but that would be additional busywork for macro authors.

Therefore, I propose that unused_qualifications should not lint on a path unless the tokens that it proposes deleting all have spans from the original source text (not any other spans that a macro might produce).

There is currently a PR out for not linting on absolute paths, which would also fix this problem, but I don't understand the rationale for doing that outside of macros, so I'm filing this issue for consideration of the specific case of macros.

Other cases

This occurs only on nightly, not on stable 1.76.0 or beta.

For some reason, among all the proc macros in use in the project I'm testing on nightly, only bytemuck is affected.

Rust Version

rustc 1.78.0-nightly (9c3ad802d 2024-03-07)
binary: rustc
commit-hash: 9c3ad802d9b9633d60d3a74668eb1be819212d34
commit-date: 2024-03-07
host: x86_64-apple-darwin
release: 1.78.0-nightly
LLVM version: 18.1.0

Anything else?

@rustbot label +regression-from-stable-to-nightly

@kpreid kpreid added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 14, 2024
@rustbot rustbot added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 14, 2024
@apiraino
Copy link
Contributor

Note: the bytemuck crate should be installed with the derive feature to reproduce:

bytemuck = { version = "1.15.0", features = ["derive"] }

Bisection points to #121528 cc: @Alexendoo (@petrochenkov as reviewer) for a comment on this. Thanks!

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 15, 2024
@Alexendoo
Copy link
Member

Alexendoo commented Mar 15, 2024

The proc macro sets the span of the path + idents to the span of Foo which is not from an expansion so the lint isn't suppressed

I don't know if there's an issue for it anywhere but it's the same root issue that necessitates the is_from_proc_macro hack in Clippy - that rustc does not record this information

@petrochenkov
Copy link
Contributor

This is an issue in specific proc macro, not in rustc.
If some macro output has an input span, then it's resolved at input location, has all edition hygiene properties from the input, etc, that's the whole purpose of spans.

In clippy they hide such issues with hacks like is_from_proc_macro, but in rustc we should not do that.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 15, 2024

Absolute paths in general shouldn't be reported by unused_qualifications, though.
But #122435 already addresses that.

@jieyouxu
Copy link
Member

This is an issue in specific proc macro, not in rustc. If some macro output has an input span, then it's resolved at input location, has all edition hygiene properties from the input, etc, that's the whole purpose of spans.

I've had other lints run into this issue too, I think the proc macros need to use something like Span::mixed_site().located_at(input_span) to have correct hygiene for the user spans. In this case, I think this issue can be closed because the proc macro needs to construct the correct span and not for rustc to add hacks to detect if a span is actually from a proc macro?

@kpreid
Copy link
Contributor Author

kpreid commented Mar 15, 2024

Bisection points to #121528

Yes, but it's not a bug in that.

Absolute paths in general shouldn't be reported by unused_qualifications, though.

Why not? I saw that issue, but I don't understand the claim. The absolute path is an unused qualification. Is it just “If you are writing an absolute path, you probably really meant that”, or is there some deeper reason?

The proc macro sets the span of the path + idents to the span of Foo which is not from an expansion so the lint isn't suppressed

Ah, I didn't read the macro code closely enough — I checked the creation of the path itself, but I didn't see the quote_spanned! that would overwrite that. So my suggestion won't usefully discriminate in this case. Oh well.

@jieyouxu jieyouxu added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Mar 16, 2024
@kpreid
Copy link
Contributor Author

kpreid commented Apr 28, 2024

This misbehavior is about to become stable in 1.78. I believe it would be fixed (in the specific case, at least) by #122435 but that was not merged in time and has not been backported.

@apiraino
Copy link
Contributor

@kpreid FIY #122435 is now stable-accepted so I think it will be in the next stable release

@apiraino
Copy link
Contributor

I think we can close this as solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. L-unused_qualifications Lint: unused_qualifications P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants