Skip to content
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.

Sign up for GitHub

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

Jump to bottom

New lint no_effect_replace #8754

Merged
merged 1 commit into from
May 24, 2022
Merged

New lint no_effect_replace #8754

merged 1 commit into from
May 24, 2022

Conversation

guerinoni
Copy link
Contributor

@guerinoni guerinoni commented Apr 27, 2022 •
edited by flip1995
Loading

Closes #1595

Signed-off-by: Federico Guerinoni [email protected]

changelog: Add [no_effect_replace] lint.

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 27, 2022
@guerinoni guerinoni mentioned this pull request Apr 27, 2022
Closed
@Manishearth
Copy link
Member

Currently travelling, redirecting reviews

r? @llogiq

@rust-highfive rust-highfive assigned llogiq and unassigned Manishearth Apr 27, 2022
llogiq
llogiq requested changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clippy_lints/src/no_effect_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/no_effect_replace.rs Outdated Show resolved Hide resolved
llogiq
llogiq requested changes Apr 30, 2022
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clippy_lints/src/no_effect_replace.rs Outdated Show resolved Hide resolved
@guerinoni guerinoni requested a review from llogiq April 30, 2022 14:55
@llogiq
Copy link
Contributor

llogiq commented Apr 30, 2022

That's better. I still think you should use SpanlessEq to subsume all your cases. Then you could use a let chain to simplify much of your code.

@llogiq
Copy link
Contributor

llogiq commented Apr 30, 2022

And I'd like to have that test against custom replace method false positives.

@guerinoni
Copy link
Contributor Author

@llogiq thank you for suggestion again, all is good now (I hope) :)

llogiq
llogiq reviewed May 2, 2022
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's just a matter of simplification, otherwise I'm OK with merging it. Thanks for bearing with me.

clippy_lints/src/no_effect_replace.rs Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor

llogiq commented May 3, 2022

That's interesting. It appears that SpanlessEq doesn't take consts into account.

Thank you for staying with us to the end.

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2022

📌 Commit 96d18b6 has been approved by llogiq

@bors
Copy link
Contributor

bors commented May 3, 2022

⌛ Testing commit 96d18b6 with merge ea1c96a...

bors added a commit that referenced this pull request May 3, 2022
@bors
Auto merge of #8754 - guerinoni:no_effect_replace, r=llogiq
ea1c96a 
New lint `no_effect_replace`

Closes #1595

Signed-off-by: Federico Guerinoni <[email protected]>

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.
@bors
Copy link
Contributor

bors commented May 3, 2022

💔 Test failed - checks-action_test

@guerinoni guerinoni requested a review from llogiq May 3, 2022 19:17
@guerinoni
Copy link
Contributor Author

@llogiq fixed commit msg 🤞

@llogiq
Copy link
Contributor

llogiq commented May 4, 2022

@bors retry

@guerinoni
Copy link
Contributor Author

@bors are you ok?

@bors
Copy link
Contributor

bors commented May 4, 2022

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

flip1995
flip1995 requested changes May 5, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more comments on this PR before merging.

clippy_lints/src/no_effect_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/no_effect_replace.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented May 10, 2022

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

@guerinoni guerinoni requested a review from flip1995 May 22, 2022 20:46
llogiq
llogiq reviewed May 24, 2022
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final thing about the error message, then I'll r+.

clippy_lints/src/methods/no_effect_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/no_effect_replace.rs Outdated Show resolved Hide resolved
@guerinoni
New lint no_effect_replace
ea62347 
Closes #1595

changelog: Add no_effect_replace lint.
@guerinoni guerinoni requested a review from llogiq May 24, 2022 09:24
@llogiq
Copy link
Contributor

llogiq commented May 24, 2022

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit ea62347 has been approved by llogiq

@bors
Copy link
Contributor

bors commented May 24, 2022

⌛ Testing commit ea62347 with merge fbb9e56...

@bors
Copy link
Contributor

bors commented May 24, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing fbb9e56 to master...

@bors bors merged commit fbb9e56 into rust-lang:master May 24, 2022
@guerinoni guerinoni deleted the no_effect_replace branch May 24, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 Awaiting requested review from flip1995

@llogiq llogiq Awaiting requested review from llogiq

Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spot where str::replace is used with the same argument
6 participants
@guerinoni @rust-highfive @Manishearth @llogiq @bors @flip1995