taiki-e / pin-project-lite

A lightweight version of pin-project written with declarative macros.
https://docs.rs/pin-project-lite
Apache License 2.0
216 stars 15 forks source link

Prepare for removal of safe_packed_borrows lint #55

Closed taiki-e closed 3 years ago

taiki-e commented 3 years ago

pin-project-lite is using safe_packed_borrows lint for checking repr(packed) types. (See https://github.com/taiki-e/pin-project/pull/34 for more)

As described in https://github.com/taiki-e/pin-project-lite/issues/26, lint-based tricks aren't perfect, but they're much better than nothing.

safe_packed_borrows is planned to be removed in favor of unaligned_references (https://github.com/rust-lang/rust/pull/82525), so I would like to switch to unaligned_references. (However, actually, enable both lint as unaligned_references does not exist in older compilers.)

taiki-e commented 3 years ago

unaligned_references works fine when used without external macros, but doesn't seem to work when used inside external macros. (also it works on internal macros: playground)

In the following code, unaligned_references emits the error correctly:

#[repr(packed)]
pub struct X {
    pub inner: u16,
}

#[forbid(unaligned_references)]
fn _f(this: &X) {
    let _ = &this.inner;
    //~^ERROR reference to packed field is unaligned
}

However, if you use a macro that generates similar code, like this:

macro_rules! pin_project {
    (
        $(#[$attrs:meta])*
        pub struct $ident:ident {
            $(
                $(#[$pin:ident])?
                $field_vis:vis $field:ident: $field_ty:ty
            ),+ $(,)?
        }
    ) => {
        $(#[$attrs])*
        pub struct $ident {
            $(
                $field_vis $field: $field_ty
            ),+
        }

        const _: () = {
            // #[forbid(safe_packed_borrows)]
            #[forbid(unaligned_references)]
            fn __assert_not_repr_packed(this: &$ident) {
                $(
                    let _ = &this.$field;
                )+
            }
        };
    };
}

No error occurs on packed types (you just get a warning about safe_packed_borrows):

pin_project_lite::pin_project! {
    //~^WARN borrow of packed field is unsafe and requires unsafe function or block (error E0133)
    // The above warning is by safe_packed_borrows and unaligned_references doesn't seem to be working.
    #[repr(packed)]
    pub struct Y {
        pub inner: u16
    }
}

I guess this is related to the difference in the span between the forbid(unaligned_references) attribute and the field where lint is triggered, but I'm not sure why safe_packed_borrows and unaligned_references behave differently.

cc @RalfJung @Aaron1011: Do you know why this doesn't work? Or is there a way to work around this?

RalfJung commented 3 years ago

@taiki-e so what is the expected behavior here? I am confused by your example. Erroring on let _ = &this.inner; seems exactly right?

I have no idea how the scoping of forbid works and how that information is propagated to the actual lints.

RalfJung commented 3 years ago

There is a flag report_in_external_macro that one can set for a lint, and it does not seem to be set for unaligned_references. But it is not set for safe_packed_borrows either so I am not sure how this would make a difference. That said, safe_packed_borrows is a lint emitted by unsafety checking (not by our usual lint passes), so maybe that makes a difference.

taiki-e commented 3 years ago

@RalfJung

I am confused by your example.

Sorry about that. I updated the example. (Hopefully, it will be easier to understand.)

Erroring on let _ = &this.inner; seems exactly right?

Yes. The problem is that if the external macro (pin_project_lite::pin_project!, in this case) generates the same code, no error occurs.

taiki-e commented 3 years ago

There is a flag report_in_external_macro that one can set for a lint, and it does not seem to be set for unaligned_references.

Using report_in_external_macro worked fine (https://github.com/rust-lang/rust/pull/82587). Thanks for your help!

RalfJung commented 3 years ago

(Hopefully, it will be easier to understand.)

Yes it makes sense now, and I am glad that that mysterious flag helps. :)

taiki-e commented 3 years ago

bors r+

https://github.com/rust-lang/rust/pull/82587 has been merged and unaligned_references now works as I expected 🎉

RalfJung commented 3 years ago

Awesome, and good catch. :)

bors[bot] commented 3 years ago

Build succeeded: