Added a new lint path_comparison_to_empty

[aljazluci]

---

changelog: [path_comparison_to_empty] Added a new lint path_comparison_to_empty, which checks for comparing a path to Path::new("") or PathBuf::new() and suggests to use is_empty

fixes: #17615

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

[Gri-ffin]

The lint could also benefit from checking PathBuf::from("").

View changes since this review

[CommanderStorm]

Code LGTM, but I suspect that there are a few cases where I don't know if they are handled correctly, so could you add a testcase for them? 🤔

View changes since this review

[CommanderStorm]

gotta love a nice XOR! 👍🏻

[CommanderStorm]

usually, we try a bit to more split than the very long fn main.
Could you split this up into a few functions for the different cases in an mod main?
This way everything is organised.

Also, please add a test to ensure that macro expansions are correctly handled like in the rest of the codebase. I am not sure if the last part needs an return from the fn, I think we do.
I also suspect that there might be testcase missing when the path is coming from an const.

[DanielEScherzer]

community review: thanks for writing this lint I suggested

View changes since this review

[DanielEScherzer]

this is also a performance improvement, suggest the performant category instead

[CommanderStorm]

That is supprisingly true, rustc seens to not be able to see past this

https://godbolt.org/z/a16MEP47G

[asder8215]

Yea, the reason why using Path::is_empty is faster than doing path equality in this case is because you don't have to go through doing component based equality using Path::components (which is a bit more involved than just checking the length of the path). I'm currently working on optimizing it for Unix/non-prefixed based platforms here, but regardless Path::is_empty is better to use over path == Path::new("").

Which reminds me that I should swap out the path == Path::new("") for path.is_empty() in the create_dir_all update PR I was working on

[rustbot]

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

[asder8215]

I agree with @Gri-ffin that the lint could benefit from checking PathBuf::from(""). I will also note that it also has a benefit from checking with an empty string, "", directly as well because the PartialEq trait implementation for Path on str actually end up doing components based equality as well. It's the reason why this assertion:

assert_eq!(Path::new("//"), "/");

succeeds. You can see that in this rust playground link. Internally, they all end up triggering impl PartialEq for Path (or the impl PartialEq for PathBuf, which also does components based equality).

Technically, all the empty string variations (e.g. str, String, OsStr, OsString, Cow) comparing with a Path/PathBuf could all be checked and suggested to use Path::is_empty instead.