New lint `clippy::join_absolute_paths` #11453

xFrednet · 2023-09-02T21:43:08Z

Hey @ofeeg, this PR is a copy of all the changes you did in 47171d3 from PR #11208. I wasn't sure how to fix the git history. Hope you're okay with me figuring this out in this separate PR

changelog: New lint [join_absolute_paths]
#11453

Closes: #10655

r? @Centri3 Since you also gave feedback on the other PR. I hope that I copied everything correctly, but a third pair of eyes would be appreciated :D

clippy_lints/src/methods/mod.rs

Centri3 · 2023-09-03T10:03:08Z

clippy_lints/src/methods/join_absolute_paths.rs

+                diag
+                      .note("joining a path starting with separator will replace the path instead")
+                      .help("if this is unintentional, try removing the starting separator")
+                      .help("if this is intentional, try creating a new Path instead");


Suggested change

diag

.note("joining a path starting with separator will replace the path instead")

.help("if this is unintentional, try removing the starting separator")

.help("if this is intentional, try creating a new Path instead");

diag.note("joining a path starting with separator will replace the path instead")

.help("if this is unintentional, try removing the starting separator")

.help("if this is intentional, try creating a new Path instead");

Still a bit weird in terms of formatting. I also think we should also add suggestions here instead of just .help(...).

tests/ui/join_absolute_paths.rs

Centri3 · 2023-09-03T10:13:14Z

clippy_lints/src/methods/join_absolute_paths.rs

+    if is_type_diagnostic_item(cx, ty, Path)
+        && let ExprKind::Lit(spanned) = &join_arg.kind
+        && let LitKind::Str(symbol, _) = spanned.node
+        && (symbol.as_str().starts_with('/') || symbol.as_str().starts_with('\\'))


Does \\ do this on unix as well? Or is it normalized to / immediately? If it doesn't, we could say this behavior would only be present on windows to remove any potential confusion.

On unix it just accepts it as a file name

let path = Path::new("C:\\Users"); let path2 = path.join("\\Ducks"); println!("{path2:#?}"); -> "C:\\Users/\\Ducks"

This behavior actually makes sense to me. I'll add a note to the lint description. I wouldn't add it to the lint emission, as it's already quite verbose.

Can we use [has_root](https://github.com/rust-lang/rust-clippy/pull/11453#method.has_root) to check ?

@lengyijun I assume you mean Path::has_root()?

I don't think that it would be good to be platform specific here. I can imagine that several projects just run Clippy on linux without specifying --all-targets. This assumption comes from the fact that I often forget to specify that flag in the CI. There might also be users, who only run Clippy locally. However, this can lead to false positives, even if I don't believe that anybody wants to start their folder name with \\ on linux. So, I think we should keep the current behavior, but move the lint to suspicious, to have the lint warn by default and not deny. Does that sound good to everyone?

clippy_lints/src/methods/join_absolute_paths.rs

Centri3 · 2023-09-03T10:15:29Z

Everything looks fine to me, minus the nits. CI seems to agree too.

ofeeg · 2023-09-04T17:58:56Z

Looks like it all got figured out. Sorry for the trouble.

bors · 2023-09-15T15:58:22Z

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

bors · 2023-10-26T21:18:08Z

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

xFrednet · 2023-11-10T22:10:19Z

I've rebased and moved the lint to suspicious, the rest should be the same as before

I say as the CI fails... And everything should be fixed now

xFrednet · 2023-11-13T12:16:51Z

@Centri3 Seems to be busy lately, @blyxyas would you mind taking over the review?

r? @blyxyas

blyxyas

Meow

clippy_lints/src/methods/mod.rs

clippy_lints/src/methods/join_absolute_paths.rs

blyxyas

LGTM, thanks! ❤️
Could you squash these and add a Co-Authored-By: ofeeg <[email protected]> to the commit message, in honor of those who are no longer listed as authors.

xFrednet · 2023-11-20T12:14:24Z

Meow @blyxyas

  ^~^  ,
 ('Y') )
 /   \/ 
(\|||/)

blyxyas · 2023-11-20T12:22:07Z

clippy_lints/src/methods/mod.rs

+    /// let new = Path::new("/sh");
+    /// assert_eq!(new, PathBuf::from("/sh"));
+    /// ```
+    #[clippy::version = "1.75.0"]


Suggested change

#[clippy::version = "1.75.0"]

#[clippy::version = "1.76.0"]

* `join_absolute_paths` Address PR review * Move `clippy::join_absolute_paths` to `clippy::suspicious` * `join_absolute_paths`: Address PR review Co-Authored-By: ofeeg <[email protected]>

blyxyas · 2023-11-20T12:28:54Z

@bors r+

bors · 2023-11-20T12:28:58Z

📌 Commit 34d9e88 has been approved by blyxyas

It is now in the queue for this repository.

bors · 2023-11-20T12:30:06Z

⌛ Testing commit 34d9e88 with merge 11a2eb0...

bors · 2023-11-20T12:38:03Z

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

rustbot assigned Centri3 Sep 2, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 2, 2023

xFrednet mentioned this pull request Sep 2, 2023

Added new lint "path_join_correction" #11208

Closed

Centri3 reviewed Sep 3, 2023

View reviewed changes

xFrednet force-pushed the 11208-path-join-correct branch from e129b5d to 1a024eb Compare September 25, 2023 18:10

lengyijun mentioned this pull request Oct 27, 2023

[pathbuf_init_then_push]: Checks for calls to push immediately a… #11700

Merged

xFrednet force-pushed the 11208-path-join-correct branch from 1a024eb to 7d69ac6 Compare November 10, 2023 22:09

xFrednet force-pushed the 11208-path-join-correct branch from 7d69ac6 to b9433a6 Compare November 10, 2023 22:11

rustbot assigned blyxyas and unassigned Centri3 Nov 13, 2023

blyxyas requested changes Nov 15, 2023

View reviewed changes

xFrednet force-pushed the 11208-path-join-correct branch from f79f6ae to 952934b Compare November 19, 2023 13:21

blyxyas approved these changes Nov 20, 2023

View reviewed changes

xFrednet force-pushed the 11208-path-join-correct branch from 952934b to 148d200 Compare November 20, 2023 12:12

blyxyas reviewed Nov 20, 2023

View reviewed changes

New lint clippy::join_absolute_paths

34d9e88

* `join_absolute_paths` Address PR review * Move `clippy::join_absolute_paths` to `clippy::suspicious` * `join_absolute_paths`: Address PR review Co-Authored-By: ofeeg <[email protected]>

xFrednet force-pushed the 11208-path-join-correct branch from 148d200 to 34d9e88 Compare November 20, 2023 12:28

bors merged commit 11a2eb0 into rust-lang:master Nov 20, 2023
8 checks passed

xFrednet deleted the 11208-path-join-correct branch November 20, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint `clippy::join_absolute_paths` #11453

New lint `clippy::join_absolute_paths` #11453

xFrednet commented Sep 2, 2023 •

edited

Loading

Centri3 Sep 3, 2023

Centri3 Sep 3, 2023

xFrednet Sep 3, 2023

lengyijun Oct 27, 2023

xFrednet Nov 10, 2023

Centri3 commented Sep 3, 2023

ofeeg commented Sep 4, 2023

bors commented Sep 15, 2023

bors commented Oct 26, 2023

xFrednet commented Nov 10, 2023 •

edited

Loading

xFrednet commented Nov 13, 2023

blyxyas left a comment

blyxyas left a comment

xFrednet commented Nov 20, 2023

blyxyas Nov 20, 2023

blyxyas commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

New lint clippy::join_absolute_paths #11453

New lint clippy::join_absolute_paths #11453

Conversation

xFrednet commented Sep 2, 2023 • edited Loading

Centri3 Sep 3, 2023

Choose a reason for hiding this comment

Centri3 Sep 3, 2023

Choose a reason for hiding this comment

xFrednet Sep 3, 2023

Choose a reason for hiding this comment

lengyijun Oct 27, 2023

Choose a reason for hiding this comment

xFrednet Nov 10, 2023

Choose a reason for hiding this comment

Centri3 commented Sep 3, 2023

ofeeg commented Sep 4, 2023

bors commented Sep 15, 2023

bors commented Oct 26, 2023

xFrednet commented Nov 10, 2023 • edited Loading

xFrednet commented Nov 13, 2023

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas left a comment

Choose a reason for hiding this comment

xFrednet commented Nov 20, 2023

blyxyas Nov 20, 2023

Choose a reason for hiding this comment

blyxyas commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

New lint `clippy::join_absolute_paths` #11453

New lint `clippy::join_absolute_paths` #11453

xFrednet commented Sep 2, 2023 •

edited

Loading

xFrednet commented Nov 10, 2023 •

edited

Loading