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_imports lint help on module named tests_* too assertive and potentially confusing #121502

Closed
jieyouxu opened this issue Feb 23, 2024 · 4 comments · Fixed by #121580
Closed
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-confusing Diagnostics: Confusing error or lint that should be reworked. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jieyouxu
Copy link
Member

jieyouxu commented Feb 23, 2024

Code

mod tests_revision_unpaired_stdout_stderr {
    use std::ptr;
}

fn main() {}

Current output

warning: unused import: `std::ptr`
 --> src/main.rs:2:9
  |
2 |     use std::ptr;
  |         ^^^^^^^^
  |
help: consider adding a `#[cfg(test)]` to the containing module
 --> src/main.rs:1:1
  |
1 | mod tests_revision_unpaired_stdout_stderr {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: `#[warn(unused_imports)]` on by default

Desired output

warning: unused import: `std::ptr`
 --> src/main.rs:2:9
  |
2 |     use std::ptr;
  |         ^^^^^^^^
  |
help: if this is a test module, consider adding a `#[cfg(test)]` to the containing module
 --> src/main.rs:1:1
  |
1 | mod tests_revision_unpaired_stdout_stderr {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: `#[warn(unused_imports)]` on by default

Rationale and extra context

This help message seems weird. I'd expect the help to be more like:

  • Consider adding #[allow(unused_imports)] if you want to silent the lint on the unused import
  • Consider removing the unused import

Suggesting to add #[cfg(test)] to the module is the least expected option, and it seems too assertive in its current wording.

Other cases

No response

Rust Version

rustc 1.78.0-nightly (bb594538f 2024-02-20)
binary: rustc
commit-hash: bb594538fc6e84213a6b8d5e165442570aa48923
commit-date: 2024-02-20
host: x86_64-apple-darwin
release: 1.78.0-nightly
LLVM version: 18.1.0

Anything else?

Perhaps the detection for triggering #[cfg(test)] on the module could be smarter and only consider a module as a potential test module if it contains at least one #[test] annotated function?

@jieyouxu jieyouxu added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. 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. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Feb 23, 2024
@jieyouxu jieyouxu changed the title unused_imports lint help on module name tests_* too assertive and potentially confusing unused_imports lint help on module named tests_* too assertive and potentially confusing Feb 23, 2024
@jieyouxu jieyouxu removed the D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. label Feb 23, 2024
@Redidacove
Copy link

How can i proceed to solve this any suggestions on how can it be done changes to which file will make it happen since i am new to this repo

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 23, 2024

Hi @Redidacove, thank you for your interest. For now, I think making the help message less assertive would be sufficient. Here are some tips to get you started:

  1. You will need to find where this help message is emitted.

You can identify where this help message is coming from by grepping the repo, i.e.

$ rg --no-ignore --fixed-strings "consider adding a `#[cfg(test)]` to the containing module" compiler 
compiler/rustc_lint/src/context/diagnostics.rs
111:                    "consider adding a `#[cfg(test)]` to the containing module",
  1. In this case, it might be as simple as changing this string to make it sound less assertive.
  2. You can reformat your changes via ./x fmt.
  3. Run the UI test suite to make sure at least one test fails -- this means at least one test is actually testing this diagnostic.
  4. If you're happy with the stderr output from your change, re-bless the UI test suite and now make sure the UI test suite passes.
  5. Run ./x test tidy to make sure tidy checks passes locally.
$ ./x test tests/ui --stage 1 --bless
  1. Submit a PR and wait for review. Your reviewer will discuss with you to see if any further changes are desirable.

https://rustc-dev-guide.rust-lang.org/ is the more in-depth reference. If you are stuck, feel free to ask here, or make a new thread in #t-compiler/help.

@Suyashtnt
Copy link
Contributor

@rustbot claim

@Suyashtnt
Copy link
Contributor

(Unless @Redidacove already submitted a change and I didn't find it, I should be able to create a PR soon)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 1, 2024
…aelwoerister

make unused_imports less assertive in test modules

closes rust-lang#121502

This is a fairly small change and I used the fix suggested in the example expected error message.
Not sure if I should've rather used the alternatives but this one seems the most descriptive.

Some alternatives:
- if this is meant to be a test module, add `#[cfg(test)]` to the containing module
- try adding #[cfg(test)] to this test module
- consider adding #[allow(unused_imports)] if you want to silent the lint on the unused import
- consider removing the unused import
@bors bors closed this as completed in 748c615 Mar 1, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 1, 2024
Rollup merge of rust-lang#121580 - Suyashtnt:issue-121502-fix, r=michaelwoerister

make unused_imports less assertive in test modules

closes rust-lang#121502

This is a fairly small change and I used the fix suggested in the example expected error message.
Not sure if I should've rather used the alternatives but this one seems the most descriptive.

Some alternatives:
- if this is meant to be a test module, add `#[cfg(test)]` to the containing module
- try adding #[cfg(test)] to this test module
- consider adding #[allow(unused_imports)] if you want to silent the lint on the unused import
- consider removing the unused import
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. D-confusing Diagnostics: Confusing error or lint that should be reworked. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants