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: unsound_collection_transmute #4592

Merged
merged 1 commit into from
Oct 8, 2019
Merged

New lint: unsound_collection_transmute #4592

merged 1 commit into from
Oct 8, 2019

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Sep 27, 2019

changelog: Add unsound_collection_transmute lint

This fixes #4515

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 27, 2019
JohnTitor
JohnTitor reviewed Sep 28, 2019
View reviewed changes
tests/ui/transmute_collection.rs Outdated Show resolved Hide resolved
clippy_lints/src/transmute.rs Outdated Show resolved Hide resolved
clippy_lints/src/transmute.rs Outdated Show resolved Hide resolved
@llogiq llogiq force-pushed the transmute-collection branch from bbc8562 to 2f430ef Compare September 28, 2019 19:09
@llogiq
Copy link
Contributor Author

llogiq commented Sep 28, 2019

@JohnTitor Thanks, I included your suggestions.

@llogiq llogiq force-pushed the transmute-collection branch from 2f430ef to e59d8a0 Compare September 28, 2019 19:46
@llogiq
Copy link
Contributor Author

llogiq commented Sep 28, 2019

Oops, it appears I needed another rebase.

@llogiq llogiq force-pushed the transmute-collection branch 2 times, most recently from 362d51c to 017fb60 Compare September 28, 2019 21:38
@llogiq
Copy link
Contributor Author

llogiq commented Sep 30, 2019

r? @flip1995

flip1995
flip1995 reviewed Sep 30, 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.

Impl LGTM. I think the fmttest should have triggered here, but maybe it didn't since some formatting errors are in the if_chain! macro. 🤔

clippy_lints/src/transmute.rs Outdated Show resolved Hide resolved
clippy_lints/src/transmute.rs
]);

// used to check for UNSOUND_COLLECTION_TRANSMUTE
static COLLECTIONS: &[&[&str]] = &[
Copy link
Member

Choose a reason for hiding this comment

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

Should we also include LinkedList here?

Copy link
Contributor Author

@llogiq llogiq Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. Transmuting might be admissible IF the alignment&size of the target type is lower or equal to that of the source type, the list isn't modified and either leaked or transmuted back to the original type afterwards. But this hasn't yet been specified by the unsafe WG, so I wouldn't bet on it. In any event, I'm erring on the side of least false positives here.

@llogiq
Copy link
Contributor Author

llogiq commented Sep 30, 2019

r? @JohnTitor

@llogiq llogiq force-pushed the transmute-collection branch from 017fb60 to dea836a Compare October 2, 2019 15:52
@llogiq llogiq force-pushed the transmute-collection branch from dea836a to 27fa2b7 Compare October 2, 2019 19:18
@llogiq
Copy link
Contributor Author

llogiq commented Oct 2, 2019

rebased.

@flip1995
Copy link
Member

flip1995 commented Oct 3, 2019

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2019

📌 Commit 27fa2b7 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Oct 3, 2019

⌛ Testing commit 27fa2b7 with merge 90e2155...

bors added a commit that referenced this pull request Oct 3, 2019
@bors
Auto merge of #4592 - rust-lang:transmute-collection, r=flip1995
90e2155 
New lint: `unsound_collection_transmute`

changelog: Add `unsound_collection_transmute` lint

This fixes #4515
@bors
Copy link
Contributor

bors commented Oct 3, 2019

💔 Test failed - status-appveyor

@llogiq
Copy link
Contributor Author

llogiq commented Oct 3, 2019

It appears appveyor failed because it was missing rustfmt?

@flip1995
Copy link
Member

flip1995 commented Oct 3, 2019

Not missing. But on windows rustfmt interpreted +nightly in rustfmt +nightly --check as a file name (also in cargo +nightly fmt --all --check)

@llogiq llogiq mentioned this pull request Oct 3, 2019
Merged
bors added a commit that referenced this pull request Oct 8, 2019
@bors
Auto merge of #4592 - rust-lang:transmute-collection, r=flip1995
e2393b0 
New lint: `unsound_collection_transmute`

changelog: Add `unsound_collection_transmute` lint

This fixes #4515
@bors
Copy link
Contributor

bors commented Oct 8, 2019

⌛ Testing commit 27fa2b7 with merge e2393b0...

@phansch phansch mentioned this pull request Oct 8, 2019
Merged
@bors
Copy link
Contributor

bors commented Oct 8, 2019

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

@bors bors merged commit 27fa2b7 into master Oct 8, 2019
@bors bors mentioned this pull request Oct 8, 2019
Closed
This was referenced Oct 8, 2019
Merged
Closed
@llogiq llogiq mentioned this pull request Oct 20, 2019
Closed
@flip1995 flip1995 deleted the transmute-collection branch October 28, 2019 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnTitor JohnTitor JohnTitor left review comments

@flip1995 flip1995 flip1995 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: transmute of Vec<T> into Vec<U> with different size or alignment is UB
4 participants
@llogiq @flip1995 @bors @JohnTitor