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

Refactor absolute_paths #13041

Merged
merged 1 commit into from
Aug 10, 2024
Merged

Refactor absolute_paths #13041

merged 1 commit into from
Aug 10, 2024

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jul 4, 2024

Checks are rearranged to do the more expensive checks later. Since the most likely path length will be one (locals and imported/local items) this will exclude such paths on the first check.

Tests were rewritten as they were hard to follow (annotations would have helped), spammy (lots of tests for the same thing) and insufficient.

One thing thing that came up and should be decided on now is what to do about the difference between path::to::Trait::item (4 segments) and path::to::Type::item (3 segments). The current behaviour treats these as different lengths which is terrible. I personally think these should both be three segments since the item can't actually be imported. Only the type or the trait could be. This makes crate_name::Trait::item the shortest absolute path which is shorter than the lint allows by default.

changelog: None

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2024

r? @y21

rustbot has assigned @y21.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 4, 2024
clippy_lints/src/absolute_paths.rs Show resolved Hide resolved
clippy_lints/src/absolute_paths.rs Outdated Show resolved Hide resolved
tests/ui-toml/absolute_paths/absolute_paths.rs Outdated Show resolved Hide resolved
@Jarcho
Copy link
Contributor Author

Jarcho commented Jul 6, 2024

Went ahead and changed path::to::Enum::variant and path::to::Trait::item to both not count the final element (both would be counted as three, not four).

Also make sure to ping me to squash this before merging. Those last two commit messages are useless.

Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Took me a fair bit to go through the test changes but looks good (aside from 2 minor things). What you said with not counting the last segment for type/trait associated items also makes sense to me.

clippy_lints/src/absolute_paths.rs Outdated Show resolved Hide resolved
clippy_utils/src/check_proc_macro.rs Show resolved Hide resolved
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

There's one missing test annotation that I missed last time (normally I wouldn't comment on this since I don't think we really require them yet, but since you already went out of your way to annotate it all). All other changes look good to me.

r=me either way

//~[no_short]| absolute_paths
let _ = std::option::Option::None::<i32>;
//~[default]^ absolute_paths
//~[no_short]| absolute_paths
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//~[no_short]| absolute_paths
//~[no_short]| absolute_paths
//~[allow_crates]| absolute_paths

@bors
Copy link
Contributor

bors commented Jul 17, 2024

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

@y21 y21 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 6, 2024
* Check the path length first
* Use `is_from_proc_macro`
* Use symbols instead of strings when checking crate names
@Jarcho
Copy link
Contributor Author

Jarcho commented Aug 10, 2024

@bors r=y21

@bors
Copy link
Contributor

bors commented Aug 10, 2024

📌 Commit f9509d3 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 10, 2024

⌛ Testing commit f9509d3 with merge 780c61f...

@bors
Copy link
Contributor

bors commented Aug 10, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 780c61f to master...

@bors bors merged commit 780c61f into rust-lang:master Aug 10, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants