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

Add lint on reference-to-pointer transmutes #124865

Open
scottmcm opened this issue May 7, 2024 · 7 comments
Open

Add lint on reference-to-pointer transmutes #124865

scottmcm opened this issue May 7, 2024 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented May 7, 2024

Inspired by #124861, specifically glium/glium@ebdc18e

There's no reason to use an unsafe transmute to get a pointer from a reference, and as that code example shows it can easily hide bugs. We should (at least) warn about it.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 7, 2024
@workingjubilee workingjubilee added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-diagnostics Area: Messages for errors, warnings, and lints T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 8, 2024
@kpreid
Copy link
Contributor

kpreid commented May 8, 2024

  • For &T to *mut T, recommend from_ref+cast_mut, but not auto-applicable because we should comment that it's probably wrong,

I recommend not offering such a suggestion in this case. People are often too quick to accept suggestions that successfully remove warnings, and it would be bad to transform simple wrong code into complex wrong code.

@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 8, 2024
@digama0
Copy link
Contributor

digama0 commented May 8, 2024

Why are none of the options "use a coercion"? In code review I would generally prefer this over from_ref; there may be some complications around reborrowing but it's not clear to me that the same issues don't also hold for from_ref. There are also type inference issues, so perhaps from_ref should only be suggested when the types would not be inferred correctly without it?

@scottmcm
Copy link
Member Author

scottmcm commented May 8, 2024

@digama0 I was assuming that if they wrote a transmute specifically then it's not a place where a coercion work, as presumably they would have tried that first. Even if it would, I think I'd rather suggest a function here that doesn't need changing the flow of things (or adding a ({ let temp: *const T = expr; temp }) or whatever). They can always change it after they read the docs for from_ref or from_mut if they want, but in general I think we should be recommending very specific "do one thing" functions where possible. Certainly it shouldn't suggest as *const _, at the very least.

Maybe if we had stable type ascription I'd lean that way, but since we don't, I like recommending the methods here for the same reason that Ralf added them in #104977. After all, even coercions can be footguns, as seen with the &mut T-to-*const T coercion.

@workingjubilee
Copy link
Contributor

Yes, we should almost certainly lint on the last case precisely because the coercion is also wrong.

@digama0
Copy link
Contributor

digama0 commented May 8, 2024

@scottmcm The original bugfix which started this is exactly a case where the right solution was to use a coercion, but mind you a coercion from &mut local instead of &local. If the linter is limited to not ask the question of asking where &local came from and whether it ought to be made mutable (instead proposing things like from_ref and a cast) I think it is considerably less useful, since that's just another way to silence the lint without fixing the actual UB.

Maybe if we had stable type ascription I'd lean that way, but since we don't, I like recommending the methods here for the same reason that Ralf added them in #104977. After all, even coercions can be footguns, as seen with the &mut T-to-*const T coercion.

I'm not suggesting type ascriptions or anything like that, I mean when using the coercion alone with no additional annotation does the right thing according to our analysis of the situation in the given context (no changed types, no &mut T to *const T, only cases exactly equivalent to the from_ref we were otherwise going to suggest anyway). Ralf's stated motivation in #104977 was to avoid as-casts, and I'm not talking about using any casts. I mean literally passing x: &T to a function expecting *const T or the mut version of that. As far as I know this usage of coercion is considered as fine and good and there are not really any hazards in it which are not already shared by any other mechanism for converting a &T to a *const T.

@Urgau
Copy link
Contributor

Urgau commented May 8, 2024

I tried, sometime ago, to add a lint that would do that, but ran into a issue,

because recommending from_mut is not always right/straight forward, and can lead to unsoundness in code that didn't have it before:

fn main() {
    let _: *mut i32 = &mut 1 as *mut _; // this line is fine
    let _: *mut i32 = std::ptr::from_mut(&mut 1); // but this one isn't, the
                                                  // pointer is dangling
}

the solution to that would be to have temporary like this:

fn main() {
    let tmp0 = &mut 1;
    let _: *mut i32 = std::ptr::from_mut(tmp0);
}

but this begs the question of usability.

@workingjubilee
Copy link
Contributor

That's not a transmute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants