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

Fix misaligned transmute lint. #2418

Conversation

devonhollowood
Copy link
Contributor

This clears up some ambiguous wording within the lint's descriptions,
and makes it so that the lint only applies to pointers. This should fix
a number of false positives (when the lint was applied to non-pointer
types) as well as false negatives (when the lint was applied to two
pointers of the same alignment, but whose pointed-to types had differing
alignments). See discussion at #2400.

This clears up some ambiguous wording within the lint's descriptions,
and makes it so that the lint only applies to pointers. This should fix
a number of false positives (when the lint was applied to non-pointer
types) as well as false negatives (when the lint was applied to two
pointers of the same alignment, but whose pointed-to types had differing
alignments).
@oli-obk
Copy link
Contributor

oli-obk commented Feb 23, 2018

I don't think we should go ahead with this. We should probably remove the lint entirely and create a lint for transmuting references to references, because one should be using raw ptr casts in that case.

@devonhollowood
Copy link
Contributor Author

This lint also checks raw ptr -> raw ptr casting for alignment issues--are you saying that we should remove that part too, or just the part of the lint that checks ref -> ref casting?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2018

No I meant we should forbid transmuting references to references or pointers to pointers and suggest casts instead. And then we can detect casts that go from low to high alignment

@devonhollowood
Copy link
Contributor Author

Ah, understood. That seems like a pretty reasonable approach to me as well.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 1, 2018

@devonhollowood do you want to tackle that change?

@devonhollowood
Copy link
Contributor Author

@oli-obk Yes, although I probably won't get a chance for another couple weeks, so if someone else wants to jump in and make the change they should feel free to do so.

@devonhollowood
Copy link
Contributor Author

@oli-obk I got some free time so I started working on this tonight, and I was wondering if you could clarify something you said earlier:

I meant we should forbid transmuting references to references or pointers to pointers and suggest casts instead.

As far as I know you can't cast to a ref with as. So in the case that you want to (say) turn an &f32 into a &u32 I think you need to use transmute. Should we still forbid this? If so, what should we recommend instead?


Also, what is the policy on deprecating old lints? Since it sounds like you want to deprecate misaligned-transmute. Should I just make the lint a no-op and mark it as deprecated in the docs (with an explanation)? Or should I remove it entirely (which I think is a breaking change for code with #[allow(misaliged_transmute)] etc)?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 4, 2018

So in the case that you want to (say) turn an &f32 into a &u32 I think you need to use transmute. Should we still forbid this? If so, what should we recommend instead?

You can do &*(&some_u32 as *const u32 as *const f32) which conveys the horror of it much better than transmute(&some_u32)

Also, what is the policy on deprecating old lints? Since it sounds like you want to deprecate misaligned-transmute. Should I just make the lint a no-op and mark it as deprecated in the docs (with an explanation)?

We have a collection of deprecated lints in https://github.com/rust-lang-nursery/rust-clippy/blob/master/clippy_lints/src/deprecated_lints.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants