-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Lint PathBufPushOverwrite #3954
Conversation
if let ExprKind::Lit(ref lit) = get_index_arg.node; | ||
if let LitKind::Str(ref r, _) = lit.node; | ||
then { | ||
if Path::new(&r.as_str()).has_root() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would is_absolute
be better than has_root
here? The docs for push
seem to suggest the path will be replaced if the other path is absolute.
Also, could this if
be part of the if_chain!
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! Seems like is_absolute
is better. As It's only equivalent to is_root
on Unix. I'll make the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying from below: #3954 (comment)
I'm not sure about the is_absolute vs has_root thing. ::push("\windows") on windows will result in c:\windows, while it doesn't matter what the previous path was (except the prefix). But is_absolute does not cover this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
Basic implementation LGTM. Some minor things need to be changed.
Also AppVeyor is not happy with the unix-like paths. probably because of the is_absolute
vs has_root
thing.
then { | ||
span_help_and_lint(cx, PATH_BUF_PUSH_OVERWRITE, expr.span, | ||
"Calling `push` with '/' (file system root) can overwrite the previous path definition", | ||
"Remove the '/' (file system root) from the start of the path instead"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use span_lint_and_sugg
. You can build the suggestion with https://doc.rust-lang.org/std/primitive.str.html#method.trim_left_matches.
Also formatting:
span_lint_and_sugg(
cx,
LINT,
span,
// ...
);
78c53f5
to
53e34fa
Compare
This LGTM now. I just would like to see, that you change the If you need help with this just let me know! :) |
Sure! I fixed the other issues first, I'll do the Another thing that I left for today, is that one of the builds on travisci failing because of this doctest:
https://travis-ci.com/rust-lang/rust-clippy/jobs/193169548 Is the best alternative here to add a |
Try to write the assert like this, instead of comparing strings (like in the push documentation): assert_eq!(x, PathBuf::from("/bar")); |
I 'almost' got it, added the suggestion and rustfix. I tried using Component::RootDir from the Path in order to replace the the Root correctly for both win/linux use cases. It did work for linux, but it's failing now on windows. I'll get a windows vm to debug this better, or find a better way to do it. Thanks for the reviews so far @flip1995 ! |
Stupid (?) idea: You could setup a test repo with travis for windows. Then you push your clippy binaries there and debug it in Travis. Not sure if that or the windows VM is more work. |
@flip1995 that was a very good idea indeed! Much better than getting a VM just for that. Thanks so much! :D In the end, I think the best impl to remove 'root' paths is the following: Digging more on how this is being tested on Rust And seeing how windows, unix and others, look for their MAIN_SEPARATOR. And also because calling |
☔ The latest upstream changes (presumably #3968) made this pull request unmergeable. Please resolve the merge conflicts. |
645683d
to
6d4e682
Compare
6d4e682
to
7e9cb5b
Compare
Awesome 🚀 no worries, I was going to rebase it anyway! |
Thanks! @bors r+ |
📌 Commit 7e9cb5b has been approved by |
Add Lint PathBufPushOverwrite Closes #3923 This is a very simple Lint that checks if push is being called with a Root Path. Because that can make it overwrite the previous path. I used std::path::Path to check if it's root, in order to keep it working across windows/linux environments instead of checking for '/'. Is that alright? On the `if_chain!` block, Is there a way to make it short while getting the value of the first argument? I got the example from other lints. Note that this is first Lint, I hope I got everything covered 🚀
☀️ Test successful - checks-travis, status-appveyor |
Closes #3923
This is a very simple Lint that checks if push is being called with a Root Path. Because that can make it overwrite the previous path.
I used std::path::Path to check if it's root, in order to keep it working across windows/linux environments instead of checking for '/'. Is that alright?
On the
if_chain!
block, Is there a way to make it short while getting the value of the first argument? I got the example from other lints.Note that this is first Lint, I hope I got everything covered 🚀