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 an option for checking for valid bits is in the getter rather than constructor #228

Closed
CasperN opened this issue Oct 13, 2020 · 5 comments · Fixed by #282
Closed

Add an option for checking for valid bits is in the getter rather than constructor #228

CasperN opened this issue Oct 13, 2020 · 5 comments · Fixed by #282

Comments

@CasperN
Copy link

CasperN commented Oct 13, 2020

Hi, I want to use bitflags for generated code that interacts with data in other languages, some of which are bitflags (google/flatbuffers#6098). I don't want to drop data so I'd like to use from_bits_unchecked. However, it is marked as unsafe. I'd like to submit a PR to add an option to get around this. I have two ideas:

  1. Move the valid-bits-check to the getter: There'll be only one constructor: from_bits and the bits() getter becomes bits() -> Result<...>, unsafe bits_unchecked(), and bits_truncated()

  2. Document why from_bits_unchecked is unsafe #200 documents that this crate uses unsafe to mean a usage issue rather than a memory issue, so maybe I could make an option to remove the unsafe altogether, and update the generated documentation appropriately?

What are your thoughts on these ideas? Does bitflags depend on the "bits are truncated to defined flags" invariant?

@sollyucko
Copy link

sollyucko commented Oct 14, 2020

Related: #188, #200, #207, #208, #211

@CasperN
Copy link
Author

CasperN commented Oct 17, 2020

@niklasf @KodrAus worked on #200

@CasperN
Copy link
Author

CasperN commented Nov 13, 2020

Bump!

I'll do the PR if needed

@KodrAus
Copy link
Member

KodrAus commented Jan 8, 2021

Hi @CasperN! 👋

We can't consider any changes to the bits() method, because the library is already stable.

The way we modeled this originally was that the flags represents a closed enum, where the set specified in the definition is the complete set of possible flags the enum could combine. Since not all valid bit patterns of the underlying integer type are valid bit patterns of the flags it's not safe for us to simply accept any integer and treat it as valid flags. So we made the unchecked method unsafe.

If we wanted to clean up the semantics we could consider allowing an #[open] attribute or something on the bitflags! definition itself, so you'd declare an open bitflags that could happily accept any integer, assuming it's not the source-of-truth for what flags exist:

bitflags! {
    #[open]
    struct Flags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
    }
}

Then let these #[open] bitflags have #[repr(transparent)] and so can be soundly transmuted to and from the given integer type, making from_bits_unchecked safe.

What do you think?

@KodrAus
Copy link
Member

KodrAus commented Jan 8, 2021

We could also use #[repr(transparent)] for this. I don’t think that’s too much of a semantic hijacking, so long as it also does apply the attribute.

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 a pull request may close this issue.

3 participants