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: suggest ptr::read instead of mem::replace(..., uninitialized()) #5695

Merged
merged 1 commit into from
Jun 23, 2020
Merged

New lint: suggest ptr::read instead of mem::replace(..., uninitialized()) #5695

merged 1 commit into from
Jun 23, 2020

Conversation

esamudera
Copy link
Contributor

@esamudera esamudera commented Jun 8, 2020 •
edited by phansch
Loading

resolves: #5575

changelog: improvements to MEM_REPLACE_WITH_UNINIT:

  • add a new test case in tests/ui/repl_uninit.rs to cover the case of replacing with mem::MaybeUninit::uninit().assume_init().
  • modify the existing MEM_REPLACE_WITH_UNINIT when replacing with mem::uninitialized to suggest using ptr::read instead.
  • lint with MEM_REPLACE_WITH_UNINIT when replacing with mem::MaybeUninit::uninit().assume_init()

@rust-highfive
Copy link

r? @phansch

(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 Jun 8, 2020
@esamudera
Copy link
Contributor Author

Hello, this is the draft implementation of a lint to suggest using ptr::read when a user is replacing with uninitialized object.

I think it's currently a bit messy, so feel free to give me feedback so I can work on them. Thanks!

phansch
phansch reviewed Jun 9, 2020
View reviewed changes
Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me, just two nits 👍

Apart from that I don't have much experience with unsafe rust. It would be good to have a second pair of eyes for the UI tests, so maybe r? @oli-obk

clippy_lints/src/mem_replace.rs Outdated
fn check_replace_with_uninit(cx: &LateContext<'_, '_>, src: &Expr<'_>, expr_span: Span) {
if let ExprKind::Call(ref repl_func, ref repl_args) = src.kind {
fn check_replace_with_uninit(cx: &LateContext<'_, '_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
if let ExprKind::MethodCall(method_path, _, method_args) = src.kind {
Copy link
Member

Choose a reason for hiding this comment

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

This if let can be moved down into the if_chain to remove one level of indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have moved the MethodCall check inside the if_chain

clippy_lints/src/mem_replace.rs Outdated
if_chain! {
// check if replacement is `maybe_uninit::uninit().assume_init()`:
if method_path.ident.name == sym!(assume_init);
if ! method_args.is_empty();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ! method_args.is_empty();
if !method_args.is_empty();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This formatting typo has been fixed

@phansch phansch assigned phansch and oli-obk and unassigned phansch Jun 9, 2020
@phansch phansch added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 9, 2020
@esamudera
Copy link
Contributor Author

Hi @oli-obk, just want to follow up on this PR in case my previous comment got buried. I will quote my previous comment:

So I checked the Rust source code, and found that #[rustc_diagnostic_item = "assume_init"] is already implemented: https://github.com/rust-lang/rust/blob/master/src/libcore/mem/maybe_uninit.rs#L495

I then implemented the diagnostic_item check and wow you're right, the code is greatly simplified and much more readable now.

With this, I guess this implementation is pretty much done right? If yes, I can rebase everything into one and remove the draft status from this PR 👍

And this is the commit: a62ae7c

Thanks!

@esamudera esamudera force-pushed the lint_mem_uninitialized branch from a62ae7c to 8c1ee06 Compare June 16, 2020 18:29
@esamudera esamudera marked this pull request as ready for review June 16, 2020 18:30
@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 22, 2020
@flip1995
Copy link
Member

r? @phansch for final review

@rust-highfive rust-highfive assigned phansch and unassigned oli-obk Jun 22, 2020
@phansch
Copy link
Member

phansch commented Jun 23, 2020

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Jun 23, 2020

📌 Commit 8c1ee06 has been approved by phansch

@bors
Copy link
Contributor

bors commented Jun 23, 2020

⌛ Testing commit 8c1ee06 with merge c8484a1...

bors added a commit that referenced this pull request Jun 23, 2020
@bors
Auto merge of #5695 - esamudera:lint_mem_uninitialized, r=phansch
c8484a1 
New lint: suggest `ptr::read` instead of `mem::replace(..., uninitialized())`

resolves: #5575

changelog:
- add a new test case in `tests/ui/repl_uninit.rs` to cover the case of replacing with `mem::MaybeUninit::uninit().assume_init()`.
- modify the existing `MEM_REPLACE_WITH_UNINIT` when replacing with `mem::uninitialized` to suggest using `ptr::read` instead.
- lint with `MEM_REPLACE_WITH_UNINIT` when replacing with `mem::MaybeUninit::uninit().assume_init()`
@bors
Copy link
Contributor

bors commented Jun 23, 2020

💔 Test failed - checks-action_test

@phansch
Copy link
Member

phansch commented Jun 23, 2020

@bors r=phansch,oli-obk retry

@bors
Copy link
Contributor

bors commented Jun 23, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Jun 23, 2020

📌 Commit 8c1ee06 has been approved by phansch,oli-obk

@bors
Copy link
Contributor

bors commented Jun 23, 2020

⌛ Testing commit 8c1ee06 with merge c56c7e2...

@bors
Copy link
Contributor

bors commented Jun 23, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch,oli-obk
Pushing c56c7e2 to master...

@bors bors merged commit c56c7e2 into rust-lang:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansch phansch phansch left review comments

@oli-obk oli-obk oli-obk approved these changes

Assignees

@phansch phansch

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.

New lint: suggest ptr::read instead of mem::replace(..., uninitialized())
6 participants
@esamudera @rust-highfive @flip1995 @phansch @bors @oli-obk