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: mem_replace_with_uninit #4511

Merged
merged 1 commit into from
Sep 20, 2019
Merged

New lint: mem_replace_with_uninit #4511

merged 1 commit into from
Sep 20, 2019

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Sep 6, 2019

changelog: add mem_replace_uninit lint

This fixes #4485

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 6, 2019
@llogiq
Copy link
Contributor Author

llogiq commented Sep 6, 2019

Seems to be blocked on a rustup.

@mati865
Copy link
Contributor

mati865 commented Sep 6, 2019

Should work when you retrigger CI, or do rebase.

@llogiq llogiq force-pushed the replace_uninitialized branch 4 times, most recently from 1e6adf7 to 3c1ea68 Compare September 6, 2019 19:22
@Shnatsel
Copy link
Member

Shnatsel commented Sep 6, 2019

There is some potential for false positives here: mem::zeroed() is only dangerous for types where not all possible bit patterns are valid. It's actually fine for numeric types like u8/i8/f32, char, or arrays of them. Not sure about structs. Everything else is dangerous even with mem::zeroed.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 6, 2019

You're right, that should probably be another lint, because mem::zeroed::<usize>() is not incorrect, just a silly way to say 0.

@Shnatsel
Copy link
Member

Shnatsel commented Sep 6, 2019 •
edited
Loading

mem::zeroed() is the only way to say 0 in a generic context, so people actually use that. However, it is UB when used in a generic context.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 7, 2019

If the types are all zero-compatible, they should implement Default and use that.

@llogiq llogiq force-pushed the replace_uninitialized branch 2 times, most recently from 1e68ee4 to c6c095a Compare September 7, 2019 06:03
flip1995
flip1995 requested changes Sep 10, 2019
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.

Can you also run ./util/dev fmt. The only reason CI passes, is because nightly rustfmt is missing since a few days.

tests/ui/mem_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
@llogiq
Copy link
Contributor Author

llogiq commented Sep 13, 2019

@flip1995 can you have another look? The problem was that I tried to match uninitialized() as Path, when it was a Call all along.

flip1995
flip1995 requested changes Sep 16, 2019
View reviewed changes
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
tests/ui/repl_uninit.rs Show resolved Hide resolved
@llogiq llogiq force-pushed the replace_uninitialized branch 2 times, most recently from 070e3a7 to b4dee97 Compare September 16, 2019 21:32
flip1995
flip1995 approved these changes Sep 17, 2019
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.

Thanks, waiting for the rustup.

@flip1995
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 19, 2019

📌 Commit ceeffcb has been approved by flip1995

@llogiq
Copy link
Contributor Author

llogiq commented Sep 19, 2019

rebased, will r=@flip1995 once travis is happy.

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Sep 19, 2019
@flip1995
Rollup merge of rust-lang#4511 - rust-lang:replace_uninitialized, r=f…
586f438 
…lip1995

New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes rust-lang#4485
@flip1995 flip1995 mentioned this pull request Sep 19, 2019
Closed
@bors
Copy link
Contributor

bors commented Sep 19, 2019

⌛ Testing commit ceeffcb with merge 55a4081...

bors added a commit that referenced this pull request Sep 19, 2019
@bors
Auto merge of #4511 - rust-lang:replace_uninitialized, r=flip1995
55a4081 
New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes #4485
@bors bors mentioned this pull request Sep 19, 2019
Merged
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Sep 19, 2019
@flip1995
Rollup merge of rust-lang#4511 - rust-lang:replace_uninitialized, r=f…
70d5acc 
…lip1995

New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes rust-lang#4485
@flip1995 flip1995 mentioned this pull request Sep 19, 2019
Closed
@flip1995
Copy link
Member

@bors retry (for queue prio)

bors added a commit that referenced this pull request Sep 19, 2019
@bors
Auto merge of #4553 - flip1995:rollup-0chhiy3, r=flip1995
f84e26d 
Rollup of 4 pull requests

Successful merges:

 - #4511 (New lint: mem_replace_with_uninit)
 - #4535 (New lint: Require `# Safety` section in pub unsafe fn docs)
 - #4539 (Changes cast-lossless to a pedantic lint)
 - #4544 (#4542 remove machine applicable suggestion)

Failed merges:

r? @ghost

changelog: none
@bors
Copy link
Contributor

bors commented Sep 19, 2019

⌛ Testing commit ceeffcb with merge 2173797...

bors added a commit that referenced this pull request Sep 19, 2019
@bors
Auto merge of #4511 - rust-lang:replace_uninitialized, r=flip1995
2173797 
New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes #4485
@bors
Copy link
Contributor

bors commented Sep 19, 2019

💔 Test failed - checks-travis

@flip1995
Copy link
Member 

Not all lints defined properly. Please run `util/dev update_lints` to make sure all lints are defined properly.

Your other new lint (the # safety one) got merged first 🤷‍♂

@llogiq
Copy link
Contributor Author

llogiq commented Sep 20, 2019

@bors retry

@flip1995
Copy link
Member

Needs to be reapproved

@bors r+

@bors
Copy link
Contributor

bors commented Sep 20, 2019

📌 Commit 8d884c8 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Sep 20, 2019

⌛ Testing commit 8d884c8 with merge ca9e4ad...

bors added a commit that referenced this pull request Sep 20, 2019
@bors
Auto merge of #4511 - rust-lang:replace_uninitialized, r=flip1995
ca9e4ad 
New lint: mem_replace_with_uninit

changelog: add `mem_replace_uninit` lint

This fixes #4485
@bors
Copy link
Contributor

bors commented Sep 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing ca9e4ad to master...

@bors bors merged commit 8d884c8 into master Sep 20, 2019
This was referenced Sep 20, 2019
Closed
Merged
Closed
Merged
@flip1995 flip1995 deleted the replace_uninitialized branch October 28, 2019 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

@Shnatsel Shnatsel Shnatsel approved these changes

Assignees
No one assigned
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: mem::replace(mem::uninitialized) is dangerous
5 participants
@llogiq @mati865 @Shnatsel @flip1995 @bors