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

[lengyijun]

changelog: [pathbuf_init_then_push]: new lint: Checks for calls to push immediately after creating a new PathBuf

Just a mirror of VEC_INIT_THEN_PUSH

[rustbot]

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

[Manishearth]

r? @Centri3

[lengyijun]

Someone may care about the performance: .join() introduce an implicit clone
But personally, I don't care

[bors]

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

[xFrednet]

Hey @lengyijun, would you mind rebasing this on master?

@ARandomDev99 This might be a good PR to review. There are some conflicts, but the implementation should be unaffected by the rebase.

If you comment something on this PR, I can also add you as an assignee

r? xFrednet

[ARandomDev99]

I concur with Centri3. Instead of multiple calls to PathBuf::join, it is better to suggest using PathBuf::from while dealing with only string literals. Additionally, I believe this lint should be in the pedantic category instead of complexity.

[xFrednet]

Additionally, I believe this lint should be in the pedantic category instead of complexity.

Could you explain why you feel this should be pedantic?

clippy::vec_init_then_push is warn-by-default and if this is free from FPs I think it's reasonable to have it as warn-by-default as well. We can bikeshed this part and leave it to the FCP before we merge this :)

[ARandomDev99]

Could you explain why you feel this should be pedantic?

@lengyijun said:

Someone may care about the performance: .join() introduce an implicit clone

This doesn't seem like a change that everyone would prefer. Having it allowed by default seems more appropriate to me. Unlike clippy::vec_init_then_push which improves performance, this lint only suggests replacing calls to push with join. This could stay in the complexity category if it replaced the calls to push with a single call to PathBuf::from.

We can bikeshed this part and leave it to the FCP before we merge this

I'm still relatively new around here. What does FCP mean? 😅

[xFrednet]

This doesn't seem like a change that everyone would prefer.

Ahh, yep you're right. Then pedantic would be the safe choice 👍

I'm still relatively new around here. What does FCP mean? 😅

No problem, FCP stands for final comment period it's a new concept for Clippy. When a new lint is ready to be merged, we create a topic on zulip to get feedback. If no concerns are raised within ~1 week we merge the PR, otherwise we discuss the issues :)

[xFrednet]

The implementation looks good to me overall. I would like to see more tests, also negative examples of code that shouldn't trigger the lint. I left some example comments.

There is also a random binary file called c. Can you remove that one?

If you rebase, we could also take a look at the lintcheck output in our CI :D

---

Sorry, that it took this long, it again slipped my mind 🙈

[xFrednet]

Let me know when this PR is ready to be reviewed again. I'm assuming the last commits are not ready yet due to their name :)

[bors]

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

[xFrednet]

I left some more comments, they should hopefully all be easy to fix. They are hopefully not too nitpicky ^^'

Let me know, if you have any question :D

[xFrednet]

LGTM, I've started the FCP: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20.60pathbuf_init_then_push.60.20rust-clippy.2311700/near/449299039

I have one last tiny comment. Could you maybe also squash the smaller commits? Having one review commit or so is totally fine :D

[bors]

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

[lengyijun]

https://github.com/rust-lang/rust-clippy/blob/279494eced419dca11c4b8e589fc6cec9dc82f46/lintcheck/src/input.rs#L182C16-L187C19

Emmm

[xFrednet]

I assume the Emmm is referring to the comment?

You can use span_contains_comment to check if there is a comment between the creation and the push

[xFrednet]

Okay, I'd say the FCP has concluded. It sounds like the lint is okay, but we had others suggest the restriction group instead, since it might be more readable but has a performance overhead. Could you change the lint group? Then it should be ready for merge. With the addition of span_contains_comment as mentioned above :D

[xFrednet]

This version looks good to me, and the two detected cases in our CI are also very good: https://github.com/rust-lang/rust-clippy/actions/runs/10004116830?pr=11700

Thank you for the last changes. I left a tiny nit, which should be simple to fix. Then you can r=me

---

Roses are red,
This PR is good,
I delegate this,
To bors the bot

[bors]

✌️ @lengyijun, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

[lengyijun]

@bors r=@xFrednet

[bors]

📌 Commit cb77f12 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit cb77f12 with merge 8fe5c75...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 8fe5c75 to master...