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

Fix items_after_test_module for non root modules, add applicable suggestion #11611

Merged

Conversation

Alexendoo
Copy link
Member

@Alexendoo Alexendoo commented Oct 4, 2023

Fixes #11050
Fixes #11153

changelog: [items_after_test_module]: Now suggests a machine-applicable suggestion.
changelog: [items:after_test_module]: Also lints for non root modules

@rustbot
Copy link
Collaborator

rustbot commented Oct 4, 2023

r? @blyxyas

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 4, 2023
@blyxyas
Copy link
Member

blyxyas commented Oct 5, 2023

I was testing the master version of this lint with the following setup:

// main.rs ==================================
#![deny(clippy::items_after_test_module)]

mod other_mod;
use crate::other_mod::add;

fn main() {
    println!("{}", add(3, 54));
}

// other_mod.rs ==============================
pub fn add(a: usize, b: usize) -> usize {
    a + b
}

#[cfg(test)]
mod tests {
    #[test]
    fn my_test() {}
}

const MY_ITEM: usize = 0;

But it doesn't emit. Turns out that the lint doesn't emit on any module that isn't on the main.rs / lib.rs. This is a bug.

@Alexendoo Alexendoo changed the title Move items_after_test_module logic into check_crate Fix items_after_test_module for non root modules, add applicable suggestion Oct 5, 2023
@Alexendoo Alexendoo force-pushed the items-after-test-module-check-crate branch from 0fbf52f to 1eee413 Compare October 5, 2023 14:31
@Alexendoo
Copy link
Member Author

Reworked the lint to cover that, also added a hidden suggestion that can be applied by --fix and tweaked the spans while I was at it

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Just some mini-nits and a question and this should be ready (。・ω・。)

clippy_lints/src/items_after_test_module.rs Outdated Show resolved Hide resolved
clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Kinda weird how what should be submodule.rs's standard error is actually stored in in_submodule.stderr

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because it's the stderr output from running the in_submodule.rs test

@Alexendoo Alexendoo force-pushed the items-after-test-module-check-crate branch from 1eee413 to dcc4001 Compare October 6, 2023 12:46
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks! ❤️ ( =ノωヽ=)

@blyxyas
Copy link
Member

blyxyas commented Oct 6, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 6, 2023

📌 Commit dcc4001 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 6, 2023

⌛ Testing commit dcc4001 with merge 279127c...

@bors
Copy link
Collaborator

bors commented Oct 6, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 279127c to master...

@bors bors merged commit 279127c into rust-lang:master Oct 6, 2023
5 checks passed
@Alexendoo Alexendoo deleted the items-after-test-module-check-crate branch October 6, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
4 participants