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

Jump to bottom

Add items_after_test_module lint #10578

Merged
merged 2 commits into from
Apr 23, 2023
Merged

Add items_after_test_module lint #10578

merged 2 commits into from
Apr 23, 2023

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Mar 31, 2023

Resolves task 3 of #10506, alongside 1 resolved at #10543 in an effort to help standarize a little bit more testing modules.

changelog:[items_after_test_module]: Added the lint.

@rustbot
Copy link
Collaborator

rustbot commented Mar 31, 2023

r? @dswij

(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 Mar 31, 2023
@bors
Copy link
Contributor

bors commented Apr 1, 2023

☔ The latest upstream changes (presumably #10534) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas blyxyas force-pushed the items_after_test_module branch from f9a006c to e7a6957 Compare April 1, 2023 22:35
@bors
Copy link
Contributor

bors commented Apr 4, 2023

☔ The latest upstream changes (presumably #10543) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas blyxyas force-pushed the items_after_test_module branch from e7a6957 to 38de6e6 Compare April 4, 2023 12:06
@bors
Copy link
Contributor

bors commented Apr 7, 2023

☔ The latest upstream changes (presumably #10601) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas
Copy link
Member Author

blyxyas commented Apr 7, 2023 •
edited
Loading

Im thinking about removing suggestions.
The current commit fixes that specific test case, but I tried with putting an attribute over should_not_lint, but it completely breaks the program.

I could fix that case, but then if I put another item with an attribute over that one, it will break again.

@blyxyas blyxyas force-pushed the items_after_test_module branch from 47c52dc to ad42ac9 Compare April 7, 2023 09:53
@bors
Copy link
Contributor

bors commented Apr 7, 2023

☔ The latest upstream changes (presumably #10497) made this pull request unmergeable. Please resolve the merge conflicts.

@blyxyas blyxyas force-pushed the items_after_test_module branch 2 times, most recently from 614e03e to 4e91b2e Compare April 7, 2023 18:08
dswij
dswij reviewed Apr 8, 2023
View reviewed changes
tests/ui/items_after_test_module.stderr Outdated Show resolved Hide resolved
tests/ui/items_after_test_module.rs Show resolved Hide resolved
clippy_lints/src/items_after_test_module.rs
declare_lint_pass!(ItemsAfterTestModule => [ITEMS_AFTER_TEST_MODULE]);

impl LateLintPass<'_> for ItemsAfterTestModule {
fn check_mod(&mut self, cx: &LateContext<'_>, _: &Mod<'_>, _: HirId) {
Copy link
Member

Choose a reason for hiding this comment

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

Some style nit/suggestion dump, feel free to ignore 😅

Maybe we can take out the if_chain check, then do a skip_while iterator on items iter.

@dswij
Copy link
Member

dswij commented Apr 8, 2023

Im thinking about removing suggestions. The current commit fixes that specific test case, but I tried with putting an attribute over should_not_lint, but it completely breaks the program.

Yeah, that sounds a bit convoluted. I'd recommend leaving the suggestion for now, and maybe come back to it in the future

@blyxyas blyxyas force-pushed the items_after_test_module branch from 4c0f747 to 0c6da4c Compare April 14, 2023 17:37
dswij
dswij reviewed Apr 20, 2023
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

@blyxyas Sorry for the rather slow review.

Just a question, and should be good to merge afterwards :)

clippy_lints/src/items_after_test_module.rs

if_chain! {
if was_test_mod_visited;
if i == (items.len() - 3 /* Weird magic number (HIR-translation behaviour) */);
Copy link
Member

Choose a reason for hiding this comment

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

Will this ever cause a panic? i.e. will items.len() < 3?

Copy link
Member

Choose a reason for hiding this comment

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

We can use https://doc.rust-lang.org/std/primitive.usize.html#method.checked_sub if we can't guarantee that this won't cause an overflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tested it with this code (The minimum amount of items that meats the requirements for the lint):

#[cfg(test)]
mod test {}

const U: usize = 0;

In an independent crate using this command: cargo dev lint ../<test dir>/ -- -- -W clippy::items_after_test_module --test
Even though the lint, well, lints. It doesn't overflow.

@dswij
Copy link
Member

dswij commented Apr 21, 2023

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2023

📌 Commit 0c6da4c has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 21, 2023

⌛ Testing commit 0c6da4c with merge 138f692...

bors added a commit that referenced this pull request Apr 21, 2023
@bors
Auto merge of #10578 - blyxyas:items_after_test_module, r=dswij
138f692 
Add `items_after_test_module` lint

Resolves task *3* of #10506, alongside *1* resolved at #10543 in an effort to help standarize a little bit more testing modules.

---

changelog:[`items_after_test_module`]: Added the lint.
@bors
Copy link
Contributor

bors commented Apr 21, 2023

💔 Test failed - checks-action_test

@dswij
Copy link
Member

dswij commented Apr 21, 2023

@blyxyas Whoops, CI failed there. Can you help to take a look?

@blyxyas
Copy link
Member Author

blyxyas commented Apr 21, 2023

I don't know how to fix this, it work on my machine and the normal Github CI test 'base'. Maybe retrying?

@blyxyas
Copy link
Member Author

blyxyas commented Apr 22, 2023

@dswij Could you retry the bors tests?

@dswij
Copy link
Member

dswij commented Apr 22, 2023

@bors retry

@bors
Copy link
Contributor

bors commented Apr 22, 2023

⌛ Testing commit 0c6da4c with merge 0884d16...

bors added a commit that referenced this pull request Apr 22, 2023
@bors
Auto merge of #10578 - blyxyas:items_after_test_module, r=dswij
0884d16 
Add `items_after_test_module` lint

Resolves task *3* of #10506, alongside *1* resolved at #10543 in an effort to help standarize a little bit more testing modules.

---

changelog:[`items_after_test_module`]: Added the lint.
@bors
Copy link
Contributor

bors commented Apr 22, 2023

💔 Test failed - checks-action_test

@blyxyas
Copy link
Member Author

blyxyas commented Apr 22, 2023

??? It works on my machine, with a modern 64-bit Ubuntu-based OS, like the CI. I guess I'm going to ask on Zulip, because I can't replicate this CI

@blyxyas blyxyas force-pushed the items_after_test_module branch from 0c6da4c to 75363a6 Compare April 22, 2023 18:57
@blyxyas blyxyas force-pushed the items_after_test_module branch from 76ed335 to 1ac8dc5 Compare April 22, 2023 19:13
@blyxyas
Copy link
Member Author

blyxyas commented Apr 22, 2023

It's fixed now, the bug was caused because the test didn't change to the //@ syntax for headers.

@dswij
Copy link
Member

dswij commented Apr 23, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Apr 23, 2023

📌 Commit 1ac8dc5 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 23, 2023

⌛ Testing commit 1ac8dc5 with merge a3ed905...

@bors
Copy link
Contributor

bors commented Apr 23, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing a3ed905 to master...

@bors bors merged commit a3ed905 into rust-lang:master Apr 23, 2023
@bors bors mentioned this pull request Apr 23, 2023
Closed
@blyxyas blyxyas mentioned this pull request Apr 23, 2023
Closed
3 tasks
This was referenced Apr 23, 2023
Merged
Closed
Merged
Closed
Merged
Merged
@dtolnay dtolnay mentioned this pull request Apr 25, 2023
Closed
@blyxyas blyxyas deleted the items_after_test_module branch October 5, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij approved these changes

Assignees

@dswij dswij

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blyxyas @rustbot @bors @dswij