New lint no_effect_replace#8754
Conversation
|
r? @Manishearth (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Currently travelling, redirecting reviews r? @llogiq |
llogiq
left a comment
llogiq
left a comment
There was a problem hiding this comment.
This is a good addition to clippy. We can still simplify the lint and make it more general by using SpanlessEq. Otherwise we should have a type or method def check.
llogiq
left a comment
llogiq
left a comment
There was a problem hiding this comment.
I thought you'd add my example to the test file, because it ensures we don't have such false positives.
For an example of the type check involved, look at clippy_lints/sec/methods/bytes_nth.rs, lines 12..19.
|
That's better. I still think you should use |
|
And I'd like to have that test against custom |
|
@llogiq thank you for suggestion again, all is good now (I hope) :) |
llogiq
left a comment
llogiq
left a comment
There was a problem hiding this comment.
Now it's just a matter of simplification, otherwise I'm OK with merging it. Thanks for bearing with me.
|
That's interesting. It appears that Thank you for staying with us to the end. @bors r+ |
|
📌 Commit 96d18b6 has been approved by |
New lint `no_effect_replace` Closes #1595 Signed-off-by: Federico Guerinoni <guerinoni.federico@gmail.com> 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. Also, it's helpful for us that the lint name is put into brackets `[]` and backticks `` ` ` ``, e.g. ``[`lint_name`]``. 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. - \[x] Followed [lint naming conventions][lint_naming] - \[x] Added passing UI tests (including committed `.stderr` file) - \[x] `cargo test` passes locally - \[x] Executed `cargo dev update_lints` - \[x] Added lint documentation - \[x] Run `cargo dev fmt` [lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints 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: Add `no_effect_replace` lint.
|
💔 Test failed - checks-action_test |
|
@llogiq fixed commit msg 🤞 |
|
@bors retry |
|
@bors are you ok? |
|
☔ The latest upstream changes (presumably #8575) made this pull request unmergeable. Please resolve the merge conflicts. |
flip1995
left a comment
flip1995
left a comment
There was a problem hiding this comment.
Two more comments on this PR before merging.
|
☔ The latest upstream changes (presumably #8769) made this pull request unmergeable. Please resolve the merge conflicts. |
llogiq
left a comment
llogiq
left a comment
There was a problem hiding this comment.
One final thing about the error message, then I'll r+.
Closes #1595 changelog: Add no_effect_replace lint.
|
Thank you! @bors r+ |
|
📌 Commit ea62347 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
6 participants
Closes #1595
Signed-off-by: Federico Guerinoni guerinoni.federico@gmail.com
changelog: Add [
no_effect_replace] lint.