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.

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

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

Closed
RalfJung opened this issue May 6, 2020 · 3 comments · Fixed by #5695
Closed

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

RalfJung opened this issue May 6, 2020 · 3 comments · Fixed by #5695
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-correctness Lint: Belongs in the correctness lint group

Comments

@RalfJung
Copy link
Member

RalfJung commented May 6, 2020

I have now at least twice seen code like this:

mem::replace(x, mem::uninitialized());

Most recently here, but I saw this before somewhere (not sure where).

Could Clippy suggest to replace this code by the following:

ptr::read(x)

The latter is more clear, avoids the mess that is uninitialized memory (both of the cases where I saw this were actually UB because of this), and can never be wrong when the former is right. Indeed, LLVM will likely (but not always) compile the former to the latter, because "write of uninitialized data to memory" can be just optimized away to nothing.

@flip1995 flip1995 added A-lint Area: New lints L-correctness Lint: Belongs in the correctness lint group good-first-issue These issues are a good way to get started with Clippy labels May 7, 2020
@esamudera
Copy link
Contributor

Hello, I'm interested in working on this issue, if this is still available.

All I need to do should be just:

  • Add mem::replace(x, mem::uninitialized()); test case to tests/ui/mem_replace.rs
  • Implement lint in clippy_lints/src/mem_replace.rs:
    • detect mem::replace fn call with mem::unititialized() param
    • suggest replacing it with ptr::read(x)

Right? If yes, I can start working on this 👍

@RalfJung
Copy link
Member Author

RalfJung commented Jun 3, 2020

Yes that sounds good. :-)
For bonus points, also detect MaybeUninit::uninit().assume_init() as an equivalent to mem::uninitialized().

@esamudera
Copy link
Contributor

Okay, adding that to the test case 👍

bors added a commit that referenced this issue Jun 23, 2020
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 bors closed this as completed in c56c7e2 Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-correctness Lint: Belongs in the correctness lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants