Added new lint "path_join_correction" #10663

ofeeg · 2023-04-17T21:03:00Z

Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
changelog: none. Otherwise, please write a short comment
explaining your change.

It's also helpful for us that the lint name is put within backticks (` `),
and then encapsulated by square brackets ([]), for example:

changelog: [`lint_name`]: your change

If your PR fixes an issue, you can add fixes #issue_number into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

Followed lint naming conventions
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: Added lint resolving issue #10655

rustbot · 2023-04-17T21:03:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

xFrednet

Welcome to Clippy 👋. This taught me something new about the behavior of Path::join(). I think this is an excellent lint, which should be warn-by-default. You already have a good start and added the lint at the right location. I've left comments, how to improve the implementation.

If you require any further assistance, you're very welcome to reach out :)

Also, small side note: The commits are not linked to your user account, probably because they use a different email. For as that is totally fine, and we can merge it with that, but I wanted to make you aware in case that is unintentional :)

xFrednet · 2023-04-22T17:40:42Z

clippy_lints/src/methods/mod.rs

@@ -3655,6 +3678,7 @@ impl Methods {
                    if let Some(("collect", _, _, span, _)) = method_call(recv) {
                        unnecessary_join::check(cx, expr, recv, join_arg, span);
                    }
+		    else {path_join_correction::check(cx, expr,  join_arg, span);}


Clippy uses spaces for indention, sadly rustfmt, doesn't work with if-let chains which are used in this function. Could you ensure that everything uses spaces here? :)

Suggested change

else {path_join_correction::check(cx, expr, join_arg, span);}

else {path_join_correction::check(cx, expr, join_arg, span);}

xFrednet · 2023-04-22T17:44:13Z

clippy_lints/src/methods/mod.rs

+    /// ### What it does
+    /// Checks for initial '/' in an argument to .join on a path.
+    /// ### Why is this bad?
+    /// .join() calls on paths already attach the '/'.
+    /// ### Example
+    /// ```rust
+    /// // example code where clippy issues a warning
+    /// let path = std::path::Path::new("/bin");
+    /// path.join("/sh");
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// // path.join("sh");
+    /// ```


Some smaller formatting adjustments :)

/// Checks for initial `'/'` in an argument to `.join()` on a `Path`. /// /// ### Why is this bad? /// `.join()` comments starting with a separator, can replace the entire path. /// If this is intentional, prefer creating a new `Path` instead. /// /// See [`Path::join()`](https://doc.rust-lang.org/std/path/struct.Path.html#method.join) /// /// ### Example /// ```rust /// let path = std::path::Path::new("/bin"); /// let res = path.join("/sh"); /// assert_eq!(res, PathBuf::from("/sh")); /// ``` /// /// Use instead: /// ```rust /// let path = std::path::Path::new("/bin"); /// /// // If this was unintentional, remove the leading separator /// let extend = path.join("sh"); /// assert_eq!(extend, PathBuf::from("/bin/sh")); /// /// // If this was intentional, create a new path instead /// let new = Path::new("/sh") /// assert_eq!(new PathBuf::from("/sh")); /// ```

Edit: I've just checked the documentation for Path::join and it turns out that .join("/xyz") actually behaves differently than I would first expect, as it actually replaces the path. I'd say that we should actually make this lint warn by default, in the suspicious category. The disadvantage is, that we have to construct two suggestions, one for the case that they intended this behavior and one if they didn't. I'll explain more below.

xFrednet · 2023-04-22T17:45:50Z

clippy_lints/src/methods/path_join_correction.rs

+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, join_arg: &'tcx Expr<'tcx>, span: Span) {
+    let applicability = Applicability::MachineApplicable;
+    if_chain!(


This is sadly another case, where rustfmt doesn't work correctly. Could you try fixing the indention here? :)

xFrednet · 2023-04-22T17:46:47Z

clippy_lints/src/methods/path_join_correction.rs

+    if_chain!(
+      if let ExprKind::Lit(spanned) = &join_arg.kind;
+      if let LitKind::Str(symbol, _) = spanned.node;
+      if symbol.as_str().starts_with('/');


Here we could check for both separators '/' and '\\'

xFrednet · 2023-04-22T17:49:18Z

clippy_lints/src/methods/mod.rs

+    /// // path.join("sh");
+    /// ```
+    #[clippy::version = "1.70.0"]
+    pub PATH_JOIN_CORRECTION,


I would suggest a different name, as #![warn(path_join_correction)] doesn't really explain what the lint does. Maybe extending_path_with_separator would be better, even if it's longer :). That name is also generic enough to include PathBuf.push(). We don't need to include that in this iteration, but that would be a nice addition for a follow-up.

I personally think the original lint name (join_absolute_path) is the perfect fit for this lint. It lints when you use the join method with an absolute path. It's quite literal and illustrative.

xFrednet · 2023-04-22T17:52:18Z

clippy_lints/src/methods/path_join_correction.rs

+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, join_arg: &'tcx Expr<'tcx>, span: Span) {
+    let applicability = Applicability::MachineApplicable;
+    if_chain!(
+      if let ExprKind::Lit(spanned) = &join_arg.kind;


Here, it should be checked, if the join() call is actually done on a Path. We have some documentation how this can be done: Checking for a specific type

xFrednet · 2023-04-22T17:54:36Z

clippy_lints/src/methods/path_join_correction.rs

+      span.with_hi(expr.span.hi()),
+      r#"argument in join called on path contains a starting '/'"#,
+      "try removing first '/'",
+      "join(\"your/path/here\")".to_owned(),


It's perfect that you tried to include a suggestion. These greatly improve lint messages and usability. The string given in the suggestion should ideally be something the user can copy and directly past into their code, which is not yet the case for this one.

In Clippy/rustc suggestions are usually created by requesting code snippets and concatenating/modifying them in a compatible way.

For this lint, the user can have two different intentions, which makes it a bit harder. I'd suggest using span_lint_and_then to create two suggestions. Something like this (Everything inside <> is still a todo):

span_lint_and_then( cx, <LINT>, <span>, "`Path::join()` starts with a path separator", |diag| { diag.note("expanding the path with a leading separator, will replace the path"); // The applicability has to be unspecified, as the intention of the user is not clear diag.span_suggestion_verbose( <str-lit-span>, "if the path should be extended, try removing the separator", <str-lit-without-separator>, Applicability::Unspecified, ); diag.span_suggestion_verbose( <expr-span>, "if the path should be replaced, try creating a new path instead", format!("Path::new(<str-lit-snippet>)"), Applicability::Unspecified, ); } );

To get specific snippets, you can take a look at snippet_with_applicability

xFrednet · 2023-04-22T21:52:52Z

tests/ui/path_join_correction.rs

+    // should be linted
+    let path = std::path::Path::new("/bin");
+    path.join("/sh");
+    println!("{}", path.display());


Could you add some more tests, with PathBuf, windows path separators \\ and raw strings like r#"/xyz"# to see if the suggestion is correct? 🙃

Hi! Sorry I have not looked at my computer for a couple of days and the app I uses on my phone to interact with github did not notify me of any of these replies. I've read them all now and will start on implementing each of your requests one at a time.

I was thinking at the time of submitting this about implementing some of these suggestions but I was admittedly pretty tired that day.

EDIT: Also, thank you for letting me know about the email address clashing. I use magit in emacs and forgot to set a valid email address. 🤣

xFrednet · 2023-04-24T20:42:06Z

Hi! Sorry I have not looked at my computer for a couple of days and the app I uses on my phone to interact with github did not notify me of any of these replies. I've read them all now and will start on implementing each of your requests one at a time.

No problem at all, my studies are currently quite time-consuming. Having some time between reviews helps me as well. Just let me know when it's ready for the next review or when you need some assistance!

EDIT: Also, thank you for letting me know about the email address clashing. I use magit in emacs and forgot to set a valid email address.

No problem! I also mention it, in case, the email/name is supposed to be private. I once commited something under my real name, before I had it linked to my GH and luckily caught it before it landed on master :)

bors · 2023-05-12T13:51:42Z

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

xFrednet · 2023-07-05T21:21:33Z

Oh, I totally missed the PR update. Sorry, I'll give you a review soon.

@blyxyas Do you maybe want to give this review a go if you have the time? :)

blyxyas

Very good for a first contribution! Thanks for joining the project! ❤️

blyxyas · 2023-07-05T21:35:56Z

clippy_lints/src/methods/mod.rs

+    /// Checks for initial `'/'` in an argument to `.join()` on a `Path`.
+    ///
+    /// ### Why is this bad?
+    /// `.join()` comments starting with a separator, can replace the entire path.


Suggested change

/// `.join()` comments starting with a separator, can replace the entire path.

/// `.join()` comments starting with a separator (`/` or `\\`) can replace the entire path.

blyxyas · 2023-07-05T21:36:04Z

clippy_lints/src/methods/mod.rs

@@ -3194,6 +3195,41 @@ declare_clippy_lint! {
    "calling `drain` in order to `clear` a container"
 }

+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for initial `'/'` in an argument to `.join()` on a `Path`.


Suggested change

/// Checks for initial `'/'` in an argument to `.join()` on a `Path`.

/// Checks for initial `/` or `\\` in an argument to `.join()` on a `Path`.

blyxyas · 2023-07-05T21:41:34Z

clippy_lints/src/methods/mod.rs

+    /// // path.join("sh");
+    /// ```
+    #[clippy::version = "1.70.0"]
+    pub PATH_JOIN_CORRECTION,


I personally think the original lint name (join_absolute_path) is the perfect fit for this lint. It lints when you use the join method with an absolute path. It's quite literal and illustrative.

blyxyas · 2023-07-05T22:03:30Z

clippy_lints/src/methods/path_join_correction.rs

+    let ty = cx.typeck_results().expr_ty(expr);
+    if_chain!(
+     if is_type_diagnostic_item(cx, ty, Path);
+     let applicability = Applicability::MachineApplicable;


No need to bind the applicability to a variable, don't worry :)
(Personal taste here, it's still fine)

blyxyas · 2023-07-05T22:12:39Z

tests/ui/path_join_correction.rs

Could you also commit the .stderr version and .fixed of this files? You can get those by using cargo dev bless after a cargo test <blah blah>.

blyxyas

Headers now use //@

blyxyas · 2023-07-15T06:04:56Z

tests/ui/join_absolute_path.rs

@@ -0,0 +1,26 @@
+// run-rustfix


Suggested change

// run-rustfix

//@run-rustfix

blyxyas · 2023-07-15T06:08:05Z

It seems like blessing isn't working... Could you describe your workflow?

ofeeg · 2023-07-15T13:06:46Z

It seems like blessing isn't working... Could you describe your workflow?

Not sure. Eventually I noticed that .fixed and .stderr files weren't being created. I use emacs and with this I just grep'd the old name and replaced all instances of the old name with a new name using sed. I wonder if I should just refork clippy and copy all the work I've done to the new fork.

blyxyas · 2023-07-15T13:43:16Z

Just a cargo test and then a cargo dev bless should fix it 🤔

ofeeg · 2023-07-15T13:53:08Z

Just a cargo test and then a cargo dev bless should fix it thinking

I did that and those files just don't get created. Also, this still fails. Unsure what possibly happened, but it's been a while since I last updated this and for all I can remember most of what I did was just keep doing cargo commands until the thing worked. I probably bugged it out at some point.

blyxyas · 2023-07-15T14:10:52Z

I'll make a PR against your fork in a few hours

blyxyas · 2023-07-15T14:26:05Z

Just before my PR, could you create a new branch named "path_join_correct" and change this PR to point to that new branch (copy-pasting these changes)? Making a PR from the master branch will have some problems.

ofeeg · 2023-07-15T14:53:49Z

Just before my PR, could you create a new branch named "path_join_correct" and change this PR to point to that new branch (copy-pasting these changes)? Making a PR from the master branch will have some problems.

Do I have to make a new PR to change the source branch? Github doesn't seem to just let me change ofeeg:master to ofeeg:path_join_correct.

EDIT: I made the branch and pasted the supposed changes.

blyxyas · 2023-07-15T21:20:29Z

No, you dont need make a new PR. I'm on mobile currently, but I'm pretty sure it's possible as I've done it for a few PRs

ofeeg · 2023-07-15T21:36:57Z

No, you dont need make a new PR. I'm on mobile currently, but I'm pretty sure it's possible as I've done it for a few PRs

All the sources I'm reading are telling me I have to make a new PR and close this one in order to merge from a different branch. If you know how to do it let me know; otherwise I will make a new PR and reference this one. I'll remember to not try to merge from my forks master in the future to prevent this.

xFrednet · 2023-07-16T07:34:53Z

As far as I know, there is no way to change the base branch of a PR. You can only force push another branch to the source branch of a PR, that would override the changes on the branch. But this PR is linked to ofeeg:master. I'm sorry. You can create a new PR or just continue development on your master branch, both options are totally file for me. :) @blyxyas Do you have a preference in this regard?

blyxyas · 2023-07-16T11:55:12Z

I have a preference for creating a new branch. Having clean git history is kinda very important when working in Rust (my first Pr had 110 commits, 75 comments of me fighting with Git, deleting the whole fork and remaking everything 😢)

From personal experience, it isn't something fun :(

Yeah, I'm not currently with a pc at range, but although I'm pretty sure it's possible, creating a new PR is also fine.

Make sure to use r? xFrednet in the PR description :)

Mason Ray Hanna III added 2 commits April 17, 2023 16:46

Added new lint for issue rust-lang#10655

9a7bf41

This commit resolves issue rust-lang#10655

5de0ee9

rustbot assigned xFrednet Apr 17, 2023

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

Fixed errors and lack of documentation in mod.rs.

327cc32

xFrednet reviewed Apr 22, 2023

View reviewed changes

Mason Ray Hanna III and others added 5 commits April 24, 2023 21:22

Added type check and test case for string slice.

4361151

Added supposed check for windows paths.

25ee587

Build failure fix.

c5b5560

Build fix attempt rust-lang#2.

412dfa6

Fixed formatting.

e1718e0

xFrednet 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 May 2, 2023

ofeeg added 2 commits May 15, 2023 11:42

Merge remote-tracking branch 'upstream/master'

1f26d8c

Rebase and merge attempt number 2.

08a8a81

ofeeg force-pushed the master branch from e0b1c56 to 08a8a81 Compare May 15, 2023 16:22

ofeeg added 3 commits May 15, 2023 12:39

Included sources to std::path in lint string.

606d801

Rebase and merge attempt number 2.

048bcc2

Fixed arguments in lint string.

c36de32

xFrednet mentioned this pull request May 18, 2023

Detect path join with hardcoded absolute path #10655

Closed

blyxyas reviewed Jul 5, 2023

View reviewed changes

ofeeg added 3 commits July 14, 2023 18:24

Renamed lint to join_absolute_path as per request.

18bd97f

Ran cargo dev update_lints

b694718

Ran cargo dev bless after a full cargo test

b91f9e0

blyxyas requested changes Jul 15, 2023

View reviewed changes

tests/ui/join_absolute_path.rs

@@ -0,0 +1,26 @@

// run-rustfix

Copy link

Member

blyxyas Jul 15, 2023

Choose a reason for hiding this comment

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

Suggested change

// run-rustfix

//@run-rustfix

ofeeg closed this Jul 21, 2023

ofeeg mentioned this pull request Jul 21, 2023

Added new lint "path_join_correction" #11208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new lint "path_join_correction" #10663

Added new lint "path_join_correction" #10663

ofeeg commented Apr 17, 2023 •

edited

Loading

rustbot commented Apr 17, 2023

xFrednet left a comment •

edited

Loading

xFrednet Apr 22, 2023

xFrednet Apr 22, 2023

xFrednet Apr 22, 2023

xFrednet Apr 22, 2023

xFrednet Apr 22, 2023

blyxyas Jul 5, 2023

xFrednet Apr 22, 2023

xFrednet Apr 22, 2023

xFrednet Apr 22, 2023

ofeeg Apr 24, 2023 •

edited

Loading

xFrednet commented Apr 24, 2023

bors commented May 12, 2023

xFrednet commented Jul 5, 2023 •

edited

Loading

blyxyas left a comment

blyxyas Jul 5, 2023

blyxyas Jul 5, 2023

blyxyas Jul 5, 2023

blyxyas Jul 5, 2023

blyxyas Jul 5, 2023

blyxyas left a comment

blyxyas Jul 15, 2023

blyxyas commented Jul 15, 2023

ofeeg commented Jul 15, 2023 •

edited

Loading

blyxyas commented Jul 15, 2023

ofeeg commented Jul 15, 2023

blyxyas commented Jul 15, 2023

blyxyas commented Jul 15, 2023 •

edited

Loading

ofeeg commented Jul 15, 2023 •

edited

Loading

blyxyas commented Jul 15, 2023

ofeeg commented Jul 15, 2023

xFrednet commented Jul 16, 2023

blyxyas commented Jul 16, 2023 •

edited

Loading

	else {path_join_correction::check(cx, expr, join_arg, span);}
	else {path_join_correction::check(cx, expr, join_arg, span);}

	/// `.join()` comments starting with a separator, can replace the entire path.
	/// `.join()` comments starting with a separator (`/` or `\\`) can replace the entire path.

	/// Checks for initial `'/'` in an argument to `.join()` on a `Path`.
	/// Checks for initial `/` or `\\` in an argument to `.join()` on a `Path`.

Added new lint "path_join_correction" #10663

Added new lint "path_join_correction" #10663

Conversation

ofeeg commented Apr 17, 2023 • edited Loading

rustbot commented Apr 17, 2023

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofeeg Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

xFrednet commented Apr 24, 2023

bors commented May 12, 2023

xFrednet commented Jul 5, 2023 • edited Loading

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas commented Jul 15, 2023

ofeeg commented Jul 15, 2023 • edited Loading

blyxyas commented Jul 15, 2023

ofeeg commented Jul 15, 2023

blyxyas commented Jul 15, 2023

blyxyas commented Jul 15, 2023 • edited Loading

ofeeg commented Jul 15, 2023 • edited Loading

blyxyas commented Jul 15, 2023

ofeeg commented Jul 15, 2023

xFrednet commented Jul 16, 2023

blyxyas commented Jul 16, 2023 • edited Loading

ofeeg commented Apr 17, 2023 •

edited

Loading

xFrednet left a comment •

edited

Loading

ofeeg Apr 24, 2023 •

edited

Loading

xFrednet commented Jul 5, 2023 •

edited

Loading

ofeeg commented Jul 15, 2023 •

edited

Loading

blyxyas commented Jul 15, 2023 •

edited

Loading

ofeeg commented Jul 15, 2023 •

edited

Loading

blyxyas commented Jul 16, 2023 •

edited

Loading