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

unsafeck: Don't treat AscribeUserType as use #80826

Closed
wants to merge 1 commit into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Jan 8, 2021

Part of #80059.


Previously, if the MIR had an AscribeUserType statement that ascribed a
type to the pointee of a raw pointer, it would be treated as a dereference
of the raw pointer for purposes of unsafe-checking. For example, the
following code would be treated as containing a raw-pointer dereference:

fn foo(ptr: *const bool) {
    let _: bool = *ptr;
}

Producing this error:

error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
 --> issue-80059.rs:2:12
  |
2 |     let _: bool = *ptr;
  |            ^^^^ dereference of raw pointer

Note that the error points to the type ascription as having a
dereference! That's because the generated AscribeUserType MIR statement
is treated as containing a dereference of _1:

AscribeUserType((*_1), +, UserTypeProjection { base: UserType(1), projs: [] });

Now the unsafe-checker ignores uses inside AscribeUserType statements,
which means this code now compiles successfully.


Note that this code:

fn foo(ptr: *const bool) {
    let _: bool = *ptr;
}

does not produce an error (it compiles fine) because of the magical
behavior of the _ (wildcard) pattern (see #80059).


r? @RalfJung

Previously, if the MIR had an AscribeUserType statement that ascribed a
type to the pointee of a raw pointer, it would be treated as a dereference
of the raw pointer for purposes of unsafe-checking. For example, the
following code would be treated as containing a raw-pointer dereference:

    fn foo(ptr: *const bool) {
        let _: bool = *ptr;
    }

Producing this error:

    error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
     --> issue-80059.rs:2:12
      |
    2 |     let _: bool = *ptr;
      |            ^^^^ dereference of raw pointer

Note that the error points to the type ascription as having a
dereference! That's because the generated AscribeUserType MIR statement
is treated as containing a dereference of `_1`:

    AscribeUserType((*_1), +, UserTypeProjection { base: UserType(1), projs: [] });

Now the unsafe-checker ignores uses inside `AscribeUserType` statements,
which means this code now compiles successfully.

-----

Note that this code:

    fn foo(ptr: *const bool) {
        let _: bool = *ptr;
    }

does *not* produce an error (it compiles fine) because of the magical
behavior of the `_` (wildcard) pattern (see rust-lang#80059).
@camelid camelid added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Jan 8, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2021
@camelid camelid added 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. labels Jan 8, 2021
@camelid
Copy link
Member Author

camelid commented Jan 8, 2021

I'm not sure if the old behavior was intended or not – it doesn't seem right, but _ is magical :)

So do we need T-lang approval?

@camelid
Copy link
Member Author

camelid commented Jan 8, 2021

Note that this code:

fn foo(ptr: *const bool) { 
	let _: bool = { *ptr };
}

still (rightly – #80059 (comment)) produces an error:

error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
 --> <anon>:2:18
  |
2 |     let _: bool = { *ptr };
  |                     ^^^^ dereference of raw pointer
  |
  = note: raw pointers may be NULL, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior

so the behavior wrt unsafe-checking of let _: ... = ... should be now the same (as far as I can tell) to let _ = ....

@RalfJung
Copy link
Member

RalfJung commented Jan 9, 2021

FWIW, I am not sure I agree with the intent of this PR, because...

so the behavior wrt unsafe-checking of let _: ... = ... should be now the same (as far as I can tell) to let _ = ....

... now the two behaviors are the same, but arguably, they are both questionable. Is it truly better to be consistent here? I am not convinced it is worth changing this until we have t-lang consensus for what the intended behavior is.

Once that consensus exists, surely the case with and without type ascription should behave the same. But until then, it is unclear which of them is wrong.

@camelid
Copy link
Member Author

camelid commented Jan 9, 2021

I agree with your reasoning, but I still think we should make this change. A type ascription (to my knowledge) does not count as a "use" of a value, regardless of whether let _ = *ptr is unsafe. If T-lang makes the decision that let _ = *ptr should be unsafe, then this change should still be made because a different instruction will be emitted that triggers the error. On the other hand, if the decision is made that let _ = *ptr should continue to be safe, this change should also be made because otherwise let _: bool = *ptr will continue to emit an error.

@RalfJung
Copy link
Member

RalfJung commented Jan 9, 2021

That's fair, I'd draw a different conclusion though. ;)

I'm not an expert on the unsafety checker anyway, so maybe someone else can review this...
r? @pnkfelix

@rust-highfive rust-highfive assigned pnkfelix and unassigned RalfJung Jan 9, 2021
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2021
@pnkfelix
Copy link
Member

i think T-lang discussed this and ended up agreeing with Ralf, but I want to go review the footage before I close it based on a memory, especially late at night...

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2021
@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2021

i think T-lang discussed this and ended up agreeing with Ralf, but I want to go review the footage before I close it based on a memory, especially late at night...

https://youtu.be/4dRVeTn5-jQ?t=1398 is the lang team meeting I was thinking of.

The essential outcome is that we agreed on these things:

  1. the unsafety-checking is meant to be syntactic, not semantic. Its an accident that we accepted the code from Unsafe checking skips pointer dereferences in unused places #80059.
  2. we are working on making a syntactic unsafety-check (that I think will operate on the HAIR).
  3. we don't want to put in more code that reinforces the previous mistakes. I think this PR falls into that category, of doubling-down on the artifacts of MIR. But leaving things alone ends up being a closer approximation for where we want to end up eventually.

So I'm going to close this PR. Sorry for taking so long to get around to that double-checking of the lang team meeting.

@pnkfelix pnkfelix closed this Mar 5, 2021
@camelid
Copy link
Member Author

camelid commented Mar 5, 2021

Once unsafeck operates on the THIR (cc rust-lang/compiler-team#402), I'm assuming the behavior will be consistent between let _ = ... and let _: bool = ... since it will be syntax-directed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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

Successfully merging this pull request may close these issues.

5 participants