Add new lint `paths_from_format` #8833

merelymyself · 2022-05-15T07:29:58Z

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

changelog: Add new lint [`path_from_format`]

rust-highfive · 2022-05-15T07:30:00Z

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

Please see the contribution instructions for more information.

bors · 2022-05-25T01:58:32Z

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

bors · 2022-05-26T05:38:33Z

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

xFrednet · 2022-05-27T16:11:34Z

Hey @merelymyself, welcome to the project!

r? @dswij (I'd appreciate it if you could leave a normal comment so that I can also assign you to this PR) 🙃

bors · 2022-05-31T05:31:53Z

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

bors · 2022-05-31T18:10:08Z

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

giraffate · 2022-06-01T00:55:36Z

Could you do a rebase, not merge commit? We follow no merge-commit policy.
https://github.com/rust-lang/rust-clippy/blob/master/doc/basics.md#pr

merelymyself · 2022-06-01T04:16:13Z

@giraffate , does this work?

dswij

Thanks for the PR, and welcome!

The basic idea for the lint looks good to me 👍 .
I left some suggestions and questions.

tests/ui/path_from_format.rs

clippy_lints/src/path_from_format.rs

merelymyself · 2022-06-03T07:01:27Z

Hi, @dswij. Made some changes, could you take another look? Thanks!

bors · 2022-06-03T10:55:03Z

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

tests/ui/path_from_format.rs

clippy_lints/src/path_from_format.rs

tests/ui/path_from_format.rs

clippy_lints/src/path_from_format.rs

merelymyself · 2022-06-09T16:29:44Z

Thanks for the patience and advice, @dswij. Could you take another look?

If there's a situation where a path like 'foo{}/bar' shows up, instead of offering a suggestion, a note is offered instead now.

dswij

Thanks for all the changes!

I think we can just not emit the lint for foo{}/bar. I'd imagine someone uses it like this, which is legit

let prefix = "some-prefixes";
let _ Path::new(format!("./{prefix}.csv"));

Note that the suggestion is also wrong right now when the variable in join() has a path separator:

let x = "/bar";
let y = Path::new("/foo").join(x); // y = "bar"

clippy_lints/src/path_from_format.rs

dswij · 2022-06-11T15:58:49Z

clippy_lints/src/path_from_format.rs

+                    PATH_FROM_FORMAT,
+                    expr.span,
+                    "`format!(..)` used to form `PathBuf`",
+                    "consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability",


Thinking about this, for long paths, .join() might actually be annoying. Maybe we can only suggest when the path is pretty short. Need some opinion on this, WDYT? @merelymyself @xFrednet

I don't really mind either way, but I feel that having consistency is slightly more important. Plus, I don't really see long paths that often in Rust projects (or is that just confirmation bias?)

clippy_lints/src/path_from_format.rs

tests/ui/path_from_format.rs

clippy_lints/src/path_from_format.rs

bors · 2022-06-15T06:38:45Z

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

bors · 2022-10-16T14:21:20Z

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

merelymyself · 2022-10-16T14:40:38Z

Hi, @dswij ! Sorry to disturb, but do you mind taking a look soon?

bors · 2022-10-20T15:19:54Z

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

xFrednet · 2023-01-07T11:12:29Z

Hey @dswij, this is a ping from triage, could you review this PR? If you don't have the time, feel free to pass it to me.

@merelymyself Would you mind rebasing the branch on master? IDK, what changes have been made to src/docs.rs in the meantime, but I hope that the rebase will go smoothly. You're welcome to reach out, if you need assistance :)

dswij · 2023-01-07T16:58:09Z

@xFrednet Thanks for taking time to look at this. I'll pass this to you if you don't mind :)

r? @xFrednet

@merelymyself Sorry I missed the pings, I've been distracted with other stuff and somehow didn't notice the ping :\

xFrednet

The basic structure and idea is already good. Though, while reading though the documentation, I found examples, that suggested a different way. I'll add a comment to the original issue and ask the author :)

@rustbot author

xFrednet · 2023-01-09T13:34:04Z

clippy_lints/src/paths_from_format.rs

+    /// `.join()` introduces additional allocations that are not present when `PathBuf::push` is
+    /// used instead.


Looking at this, I think it would be better to suggest PathBuf directly. Even if that might require additional modifications by the user. PathBuf is intended for modifications as this one AFAIK :)

Though, in that case we should have the applicability be less than machine applicable

Sorry, to clarify, you mean you want to do some or all of the following?

Replace .join() with .push()

Replace the Path::new with PathBuf::from

Instead of suggesting a replacement, just flag it out to the user

Sorry, if my comment wasn't clear. I mean a combination of the first two. So suggest PathBuf::from() and then add segments using push(). The thing is, that this probably change the type of the buffer and require further adjustments by the user. Therefore, we should display the suggestion, but not make it machine applicable.

Does this make sense, or should I try to describe it better? :)

I'm looking at the docs for PathBuf now, and I just realised that there might be a recommended solution for situations like this 😵‍💫.

let var = "path"; pathbuf = ["/", var, "filename.txt"].iter().collect::<PathBuf>();

Functionally equivalent to

let var = "path"; pathbuf = PathBuf::from(format!("/{var}/filename.txt"));

Will you be ok with this? Given it should still be machine applicable but I don't think it will have the same problems as before.

You have to be careful when using the array as all the values have to be the same type.

clippy_lints/src/paths_from_format.rs

bors · 2023-01-19T15:23:47Z

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

xFrednet

Hey, thank you for your patience, the review sadly took a bit longer. You're welcome to ask if you have any questions :)

xFrednet · 2023-01-28T21:49:46Z

clippy_lints/src/paths_from_format.rs

+                    PATHS_FROM_FORMAT,
+                    expr.span,
+                    "`format!(..)` used to form `PathBuf`",
+                    "consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability",


I would remove the second part, as some users might not consider this more readable. The OS-agnostic part is also the main point, IMO.

xFrednet · 2023-01-28T21:52:50Z

clippy_lints/src/paths_from_format.rs

+        if_chain! {
+            if let ExprKind::Call(_, [arg, ..]) = expr.kind;
+            if let ty = cx.typeck_results().expr_ty(expr);
+            if is_type_diagnostic_item(cx, ty, sym::PathBuf);


Should we maybe also check for calls to Path::new?

xFrednet · 2023-01-28T21:56:33Z

clippy_lints/src/paths_from_format.rs

+impl<'tcx> LateLintPass<'tcx> for PathsFromFormat {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if_chain! {
+            if let ExprKind::Call(_, [arg, ..]) = expr.kind;


Right now, this lint only checks for function calls, where the output is a PathBuf and the first argument is a format macro. The lint logic should also check the first value of ExprKind::Call to ensure that it's a path to a function called new or other related function names.

The logic should also contain a check if the code originated from a macro. I believe the check should be as simple as !expr.span.from_expansion()

xFrednet · 2023-01-28T21:59:20Z

clippy_lints/src/paths_from_format.rs

+                    .iter()
+                    .map(rustc_span::Symbol::as_str)
+                    .collect();
+                let mut applicability = Applicability::MachineApplicable;


Since the applicability is explicitly set with the lint emission, you can remove this value and all usages of it.

For Sugg::hir_with_applicability you can just use Sugg::hir

xFrednet · 2023-01-28T22:00:22Z

clippy_lints/src/paths_from_format.rs

+                    }
+                    else {


Formatting nit, as well as the other else cases in the if_chain!

Suggested change

}

else {

} else {

xFrednet · 2023-01-28T22:01:38Z

clippy_lints/src/paths_from_format.rs

+                            let _ = write!(sugg, ".join(&{arg})");
+                        }
+                        else {
+                            sugg = format!("{sugg}.join(&{arg})");


Is there a reason, why you use two different methods to append text to an existing string?

xFrednet · 2023-01-30T20:37:19Z

clippy_lints/src/paths_from_format.rs

+
+// In essence, this checks if the format!() used is made for concatenation of the filenames / folder
+// names itself. This returns true when it is something like
+// `PathBuf::from(format!("/x/folder{}/textfile.txt", folder_number))`


Based on the comment in the related issue, I would actually think that such a paht can still be problematic. I'd suggest either, checking if the path starts with //?/ which makes it a verbatim path under windows or (IMO the better approach) add a config value, to allow this. I have the feeling the lint can be very noisy without, but ignoring these cases can also be wrong.

bors · 2023-02-03T12:15:00Z

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

xFrednet · 2023-02-28T15:10:43Z

Hey @merelymyself this is a ping from triage. Are you still working on this PR? If you have any questions, you're always welcome to reach out :)

merelymyself · 2023-03-02T06:16:26Z

Hi, so sorry. Thanks for all the help so far, but I don't think I'll have much time in the near future to work on this with any level of consistency - again, apologies for any inconvenience caused. I'll close the PR.

xFrednet · 2023-03-02T12:12:00Z

It was no inconvenience at all, I hope you still had some fun!

I'll mark this PR as S-inactive-closed it's still a good start for anyone that wants to work on the issue.

rust-highfive assigned camsteffen May 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 15, 2022

merelymyself changed the title ~~Add new lint `[path_from_format]`~~ Add new lint [path_from_format] May 15, 2022

merelymyself changed the title ~~Add new lint [path_from_format]~~ Add new lint path_from_format May 15, 2022

merelymyself force-pushed the master branch from e905f73 to d1b9f0b Compare May 25, 2022 07:34

xFrednet assigned xFrednet and unassigned camsteffen May 27, 2022

merelymyself force-pushed the master branch from be7f95f to 7fbd14f Compare June 1, 2022 04:15

xFrednet assigned dswij Jun 1, 2022

dswij reviewed Jun 1, 2022

View reviewed changes

tests/ui/path_from_format.rs Outdated Show resolved Hide resolved

clippy_lints/src/path_from_format.rs Outdated Show resolved Hide resolved

clippy_lints/src/path_from_format.rs Outdated Show resolved Hide resolved

merelymyself marked this pull request as draft June 1, 2022 15:40

merelymyself marked this pull request as ready for review June 3, 2022 07:00

merelymyself force-pushed the master branch from 0e8f0a6 to ba8899a Compare June 4, 2022 05:34

dswij reviewed Jun 4, 2022

View reviewed changes

tests/ui/path_from_format.rs Outdated Show resolved Hide resolved

clippy_lints/src/path_from_format.rs Outdated Show resolved Hide resolved

tests/ui/path_from_format.rs Outdated Show resolved Hide resolved

clippy_lints/src/path_from_format.rs Outdated Show resolved Hide resolved

infmagic2047 reviewed Jun 4, 2022

View reviewed changes

clippy_lints/src/path_from_format.rs Outdated Show resolved Hide resolved

xFrednet removed their assignment Jun 7, 2022

dswij reviewed Jun 11, 2022

View reviewed changes

merelymyself force-pushed the master branch from b2456b0 to 61f054f Compare July 11, 2022 10:06

merelymyself force-pushed the master branch from e2320a5 to b0c6da1 Compare October 16, 2022 14:29

rustbot assigned xFrednet and unassigned dswij Jan 7, 2023

xFrednet reviewed Jan 9, 2023

View reviewed changes

rustbot 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 Jan 9, 2023

merelymyself force-pushed the master branch from e5fadf8 to b031924 Compare January 19, 2023 02:50

merelymyself added 3 commits January 20, 2023 21:32

add lint paths_from_format

4596d61

formatting, [args,..] and registering the lint

6c2f56f

docs for valid use

5a6ca6b

merelymyself force-pushed the master branch from b031924 to 5a6ca6b Compare January 20, 2023 13:35

merelymyself added 4 commits January 21, 2023 10:57

fix minor issue regarding args/arg

47e4a45

cargo dev update_lints

7980350

cargo fmt

1a35d49

dogfood

29384d7

xFrednet reviewed Jan 30, 2023

View reviewed changes

merelymyself closed this Mar 2, 2023

xFrednet added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 2, 2023

xFrednet mentioned this pull request Mar 2, 2023

Lint suggestion: Path construction using format! #8812

Open

		/// `.join()` introduces additional allocations that are not present when `PathBuf::push` is
		/// used instead.

Add new lint paths_from_format #8833

Add new lint paths_from_format #8833

Conversation

merelymyself commented May 15, 2022 • edited Loading

rust-highfive commented May 15, 2022

bors commented May 25, 2022

bors commented May 26, 2022

xFrednet commented May 27, 2022

bors commented May 31, 2022

bors commented May 31, 2022

giraffate commented Jun 1, 2022

merelymyself commented Jun 1, 2022

dswij left a comment

Choose a reason for hiding this comment

merelymyself commented Jun 3, 2022

bors commented Jun 3, 2022

merelymyself commented Jun 9, 2022

dswij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jun 15, 2022

bors commented Oct 16, 2022

merelymyself commented Oct 16, 2022

bors commented Oct 20, 2022

xFrednet commented Jan 7, 2023

dswij commented Jan 7, 2023 • edited Loading

xFrednet 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

merelymyself Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 19, 2023

xFrednet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 3, 2023

xFrednet commented Feb 28, 2023

merelymyself commented Mar 2, 2023

xFrednet commented Mar 2, 2023

Add new lint `paths_from_format` #8833

Add new lint `paths_from_format` #8833

merelymyself commented May 15, 2022 •

edited

Loading

dswij commented Jan 7, 2023 •

edited

Loading

merelymyself Jan 21, 2023 •

edited

Loading