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

Prepare for removal of safe_packed_borrows lint #55

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

taiki-e
Copy link
Owner

@taiki-e taiki-e commented Feb 27, 2021

pin-project-lite is using safe_packed_borrows lint for checking repr(packed) types. (See taiki-e/pin-project#34 for more)

As described in #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 (rust-lang/rust#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
Copy link
Owner Author

taiki-e commented Feb 27, 2021

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?

src/lib.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link

RalfJung commented Feb 27, 2021

@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
Copy link

RalfJung commented Feb 27, 2021

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
Copy link
Owner Author

taiki-e commented Feb 27, 2021

@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
Copy link
Owner Author

taiki-e commented Feb 27, 2021

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 (rust-lang/rust#82587). Thanks for your help!

@RalfJung
Copy link

(Hopefully, it will be easier to understand.)

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

@taiki-e taiki-e added the S-blocked Status: Blocked on something else label Feb 27, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2021
Enable report_in_external_macro in unaligned_references

Fixes an issue where `unaligned_references` is not triggered in external macros, unlike `safe_packed_borrows`.
Also, given that this lint is planned to eventually change to hard error (rust-lang#82525), it would make sense for this lint to be triggered for external macros as well.

See taiki-e/pin-project-lite#55 (comment) and taiki-e/pin-project-lite#55 (comment) for more.

r? `@RalfJung`
@taiki-e
Copy link
Owner Author

taiki-e commented Mar 2, 2021

bors r+

rust-lang/rust#82587 has been merged and unaligned_references now works as I expected 🎉

@RalfJung
Copy link

RalfJung commented Mar 2, 2021

Awesome, and good catch. :)

@bors
Copy link
Contributor

bors bot commented Mar 2, 2021

Build succeeded:

@bors bors bot merged commit 5d4167e into master Mar 2, 2021
@bors bors bot deleted the unaligned_references branch March 2, 2021 11:09
@taiki-e taiki-e removed the S-blocked Status: Blocked on something else label Mar 2, 2021
bors bot added a commit that referenced this pull request Mar 2, 2021
56: 0.1: Prepare for removal of safe_packed_borrows lint r=taiki-e a=taiki-e

This is the backport of #55

Co-authored-by: Taiki Endo <te316e89@gmail.com>
bors bot added a commit to taiki-e/pin-project that referenced this pull request Mar 27, 2021
321: Prepare for removal of safe_packed_borrows lint r=taiki-e a=taiki-e

Same as taiki-e/pin-project-lite#55, but, pin-project's safety no longer depends on this, so there is no need to rush release. 

Co-authored-by: Taiki Endo <te316e89@gmail.com>
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