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

RFC: ForbiddenValues trait to enable more optimizations #2888

Closed
wants to merge 13 commits into from
Closed

RFC: ForbiddenValues trait to enable more optimizations #2888

wants to merge 13 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 24, 2020

@ghost ghost changed the title RFC: ForbiddenValue trait to enable more optimizations RFC: ForbiddenValue trait to enable more optimizations Mar 24, 2020
@shepmaster
Copy link
Member

I'm no compiler implementation expert, but this RFC feels pretty light in the details department. It seems to focus mostly on a surface syntax of how this would be used, but doesn't give any time to how the implementation using that information would happen.

@RustyYato
Copy link

RustyYato commented Mar 24, 2020

This looks like it only permits 1 forbidden value, which is rather limiting. Note: If you're type has any padding bytes (which most structs do), you can get 255 forbidden values by simply replacing the padding byte with

#[repr(u8)]
enum ZeroPadding {
    Padding = 0
}

For enums are much harder to layout optimize, usually we can merge the discriminants together, this is what allows Option<Result<(), ()>> to be a single byte. So I don't see the value in adding a special (super-limited) trait. If you can make this more general than what we already have, then it may be worth consideration. But as it stands, we really don't need this added complexity.

@ghost
Copy link
Author

ghost commented Mar 24, 2020

@shepmaster I know nothing about rustc, but here's my best guess about the implementation:

  1. In rustc, there must be code that decides whether or not to perform an optimization using forbidden value(s) given some type(s) - I propose we change that code to also check whether or not the type(s) in question implement ForbiddenValue.
  2. In rustc, there must be code that decides which forbidden value(s) to use when it performs the optimization - I propose we change that code to use ForbiddenValue::FORBIDDEN_VALUE_BYTES as the forbidden value where applicable.

@KrishnaSannasi The value of this RFC is for those cases where it's not convenient to use an enum as a part of your type. For example:

pub struct S {
    x: u32,
}

If legal values for the field x are all u32 values except u32::MAX, then you wouldn't want to make an enum with 2^32-1 variants and use it for x in order to get the optimizations for S.

@RustyYato
Copy link

So, similar to the currently unstable + internal rustc_layout_scalar_valid_range_start and rustc_layout_scalar_valid_range_end attributes?

@ghost
Copy link
Author

ghost commented Mar 24, 2020

@KrishnaSannasi I wasn't aware of those attributes, so thank you for bringing them to my attention. The error message when #![feature(rustc_attrs)] isn't available says that those attributes will never be stable. But they seem to be one way to tap into the forbidden-value-based optimizations which is exactly what this RFC is asking for.

@clarfonthey
Copy link
Contributor

Personally, I'd rather those internals be exposed as some sort of bounded integer types than something like this.

@Lokathor
Copy link
Contributor

That is much less easily usable to create type system guarantees.

@shepmaster
Copy link
Member

I don't think "bounded integer types" and "arbitrary niche specification" are exclusive features. Bounded integer types are a concrete usage, potentially with a different surface syntax, but could be implemented using arbitrary niches.

In my head, I view it akin to how async/await sits atop generators.

@clarfonthey
Copy link
Contributor

clarfonthey commented Mar 26, 2020

That's a pretty good analogy, honestly. The main reason why I mention bounded integer types is because it requires actual language support to allow coercing integer literals into bounded types, and allowing casting properly. Niche specialization can technically be done today in a very hacky way with enums, although I personally wouldn't mind requiring that someone make an enum to allow niches like disjoint ranges (e.g. a byte whose values can only be 0-10 or 20-100).

Having to explicitly define 0-254 enum variants to get these niches today is an enormous hack, and ranges at least ease the pain with minimal changes from a user perspective.

@Evrey
Copy link

Evrey commented Mar 27, 2020

It would be extremely powerful if we could utilise const impl for this feature to turn the check into a const fn is_forbidden(&self) -> bool. For NonZeroX that'd just do self.0 == 0.

Alternatively, and with less reliance on const fn progress, the FORBIDDEN_VALUE_BYTES constant could instead be a &[[u8; size_of::<Self>()]]. If that slice is empty, the trait impl has no effect. If it has just one entry, it equals the current proposal. If it has more than one entries, all of them are forbidden. If all possible values are included, then all possible states of Self are forbidden, oops. An equal entry appearing more than once has the same effect as an entry appearing just once.

Edit: As a potential upside of doing const fn is_forbidden: All branches depending on self can be desugared by inserting this code snippet immediately before the branching code…

if {const B: bool = self.is_forbidden(); B} {
    unsafe { core::hint::unreachable_unchecked() };
}

Then rustc needs no extra magic other than injecting this snippet here and there.

Edit2: A second benefit could be that const fn is_forbidden can express complex interdependencies of multiple fields, such as self.start > self.end for a range type.

Edit3: Potential downside is that figuring out how many potential forbidden variants there are for enum layout optimisations is potentially too complicated with const fn is_forbidden.

@pickfire
Copy link
Contributor

pickfire commented Apr 2, 2020

How about a forbidden bit? I have seen it in http://troubles.md/abusing-rustc/

@dtolnay dtolnay added T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. labels Apr 4, 2020
@elichai
Copy link

elichai commented Apr 9, 2020

Is this useful as long as repr(Rust) has unstable representation?
I don't want to trade one optimization for another.

@Evrey
Copy link

Evrey commented Apr 10, 2020

@elichai Good question. As in my boolean function proposition, it would be good to just use Self instead of a byte array. So const FORBIDDEN_VALUE: Self. However, with a constant, one could abuse that by calling methods in invalid state. With a boolean function, there is no instance of an invalid Self lying around.

Changed the `ForbiddenValue` trait (renamed to `ForbiddenValues`) so that it is able to define multiple forbidden values.
@ghost
Copy link
Author

ghost commented May 11, 2020

This looks like it only permits 1 forbidden value, which is rather limiting.

@RustyYato I changed this RFC to allow multiple forbidden values. Also, added an example of a FastFloat type that shows a case where a bounded integer type wouldn't be of any use.

@ghost ghost changed the title RFC: ForbiddenValue trait to enable more optimizations RFC: ForbiddenValues trait to enable more optimizations May 11, 2020
unsafe impl ForbiddenValues<[[u8; 4]; 3], [u8; 4]> for FastFloat {
const FORBIDDEN_VALUES: [[u8; 4]; 3] = unsafe {
[
mem::transmute(f32::NAN),
Copy link

@Evrey Evrey May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that f32 has about 16.8 million different NaN values.

Copy link
Author

@ghost ghost May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. You just write a procedural macro to produce those 16.8 million lines of code and buy a supercomputer to compile it 😜

I admit that your suggestion for a trait with a const fn is_forbidden(&self) -> bool method would be strictly better than this RFC - it would make it (relatively) easy to specify large ranges of forbidden values. But I do wonder about its compiler implementation - without knowing much at all, I would imagine that the compiler would have to call value.is_forbidden() for each possible bit pattern of value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would the compiler have to call value.is_forbidden() for each possible bit pattern?

If the compiler "needs" 5 values for a variety of optimizations, it would just have to find 5 forbidden values and use those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would always have a worst case of going through every value, though. Which isn't scalable past a few bits.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To eliminate dead code, all rustc needs to do is insert and constant-fold the mentioned unreachable_unchecked snippet. This is something it can do today.

Finding holes for compactly encoding enum variants, however, is more tricky.

mem::transmute(f32::NAN),
mem::transmute(f32::INFINITY),
mem::transmute(f32::NEG_INFINITY),
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we have a range of forbidden values? For example if we were to have u8 that is able to store 55 numbers at most, then all the rest of the numbers as forbidden values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have to add all those 201 (2^8-55) forbidden values to the FORBIDDEN_VALUES array as [u8; 1] values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems extremely suboptimal. Why not just accept ranges? Individual values can be done as ranges too.

Copy link
Member

@jhpratt jhpratt May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a struct that is packed and uses 30 bits. For obvious reasons it's stored as a 32 bit integer.

If we used const fn(T) -> bool or ranges, this would be trivial to write. With the proposal, I'd have to list out over three billion values. Obviously that's neither feasible nor fast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that a list is infeasible, but my point is that a predicate is also infeasible. Imagine that the forbidden values are in the higher bits, and that the compiler simply does a sweep of the values in order to find the forbidden ones. It'll still have to go through almost anything.

And if your argument is that the compiler should be able to look at the function and determine what it's doing, 1. the halting problem and 2. ranges are easier and way less jank :p

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slice of ranges? I think that would solve pretty much every situation, as a value can (of course) be represented as a range.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clarfon Do you mean ranges as in something like the following?

unsafe trait ForbiddenValues<A, B>
where
    A: FixedSizeArray<(B, B)>,
    B: FixedSizeArray<u8>,
{
    const FORBIDDEN_VALUES: A;
}

Where B is now interpreted as a fixed-size unsigned integer type with an arbitrary width, and the tuple (B, B) represents the inclusive start and end of the range of forbidden values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using a list of both ranges and bit masks to define forbidden values is probably the best solution, since it handles nearly all cases and doesn't have the halting problem and similar issues you'd run into with defining it using a const fn.

For the bit mask, it would match all values v that pass (v & a) == b where a and b are values of some sort (byte arrays? integers?).

As an example of why ranges alone are insufficient:
What if you have a type like:

enum E<T> {
    A(T),
    B(T),
    C(T),
    D(T),
}

type E2<'a'> = E<&'a u32>;

where you want Rust to pack the enum tag for E2 into the 2 low bits of the pointer since the pointer is guaranteed to be some value with the two low bits are zeros due to references guaranteed alignment and u32 having an alignment of 4? Rust currently doesn't pack the enum that way, but could. This kind of bit packing is used extensively throughout LLVM's codebase (implemented using custom C++ code), so is not that uncommon.

If you were to enumerate the forbidden values of a custom T type using ranges, you'd get something like:

const FORBIDDEN_VALUES: [...] = [
    0x0..=0x3, // include 0 for null
    0x5..=0x7, // repeats every 4 values
    0x9..=0xB,
    0xD..=0xF,
    0x11..=0x13,
    0x15..=0x17,
    0x19..=0x1B,
    0x1D..=0x1F,
    ... // another *billion* lines for 32-bit pointers
];

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like the following to allow forbidding values both by ranges and by a bit mask?

unsafe trait ForbiddenValues<R, B>
where
    R: FixedSizeArray<RangeInclusive<B>>,
    B: FixedSizeArray<u8>,
{
    const FORBIDDEN_RANGES: R;
    const FORBIDDEN_BITS: B;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For bitmasks, just a single integer is insufficient, you'd need something like:

struct BitMask<T> {
    mask: T,
    value: T,
}

impl<T> BitMask<T> {
    fn matches<'a>(&'a self, value: T) -> bool
    where
        T: BitAnd<&'a T, Output = T> + PartialEq,
    {
        (value & &self.mask) == self.value
    }
}

unsafe trait ForbiddenValues<B>
where
    B: MemoryBits,
{
    const FORBIDDEN_RANGES: &'static [RangeInclusive<B>];
    const FORBIDDEN_BIT_MASKS: &'static [BitMask<B>];
}

This allows you to represent things such as an integer where top 4 and bottom 12 bits can't be 0x3 and 0xCAB by including a bit mask BitMask { mask: 0xF000_0FFFu32, value: 0x3000_0CABu32 } assuming u32 implements MemoryBits.

@pickfire
Copy link
Contributor

I wonder if rust currently pack struct of bools into just a single u8 or usize. It would be interesting to see multiple bool reuse the same memory but different bit.

@ghost
Copy link
Author

ghost commented May 13, 2020

Can I use the “null pointer optimization” for my own non-pointer types?

use std::{mem, num::NonZeroU32};

struct Foo {
    n: NonZeroU32,
}

impl Foo {
    pub fn new(v: u32) -> Self {
        Self { n: Self::convert(v) }
    }
    
    pub fn get(&self) -> u32 {
        u32::from(self.n).wrapping_sub(1)
    }

    pub fn set(&mut self, v: u32) {
        self.n = Self::convert(v)
    }
    
    fn convert(v: u32) -> NonZeroU32 {
        NonZeroU32::new(v.wrapping_add(1)).expect("Out of range")
    }
}

fn main() {
    let mut f = Foo::new(0);
    dbg!(f.get());

    f.set(1);
    dbg!(f.get());

    // f.set(u32::MAX); // panic
    // dbg!(f.get());

    assert_eq!(mem::size_of::<Foo>(), mem::size_of::<Option<Foo>>());
}

It only now, much later, occurred to me that your solution here probably kind of is what @Ixrec meant by implementing exotic layout guarantees with a bunch of bitshift operations (because I had no idea what he meant by that). With the exception that your example uses wrapping_add|sub instead of shifts because my example just so happened to have no extra bit to spare (which was not my intention... like I said, I had no idea).

So, if my options are to use Option<Bar> with extra shift/math operations when accessing Bar or use a trivial struct Opt<T> { t: T } wrapper on Bar with no extra shift/math operations, then I would choose the latter (and suffer the worse ergonomics compared to using Option).

@Ixrec
Copy link
Contributor

Ixrec commented May 13, 2020

Sorry for the confusion. @burdges and @shepmaster's comments together are pretty much exactly what I had in mind with that comment. I thought this was well-known since that's basically how all bitflags/bitfield crates work, and those are by far the most prominent use case for the sort of thing this RFC is about.

@pickfire
Copy link
Contributor

pickfire commented May 13, 2020

And in both of those examples, I can't recall anyone claiming they put such types into an enum and then suffered from a missing niche optimization.

@lxrec so this means one should not use the bool in standard library but instead use some more optimized bool for memory in some other crates? I think this would make more crates such as https://github.com/maciejhirsz/beef appear to be a more optimized version of the stuff available in standard library. Maybe it would be cool to see multiple copy of the standard library as a crate built for different reasons, such as async-std (built for async but it's already here) or built for speed or built for memory (didn't check but embedded rust probably have one).

@Ixrec
Copy link
Contributor

Ixrec commented May 13, 2020

There isn't really such a thing as a "more optimized bool". What you'd be optimizing is a larger data structure which semantically is made up of bools, e.g. because you want each "logical bool" to take up only one bit of actual memory. In other words, bitfields.

In general there will always be lots of crates offering variations on stuff in std, because there's a million ways to implement these things with various tradeoffs that are occasionally relevant to someone. However, async-std doesn't really have anything to do with this RFC. and IIUC beef is only tangentially relevant to this RFC since it's not so much exploiting niches in user-defined types as it is a) exploiting pointer layout guarantees and b) getting away with smaller types by exploiting the fact that you almost never need String/Vec lengths/capacities that go all the way up to usize::MAX. You could argue that a) should happen by default in the language, but raw pointers are already a primitive type, so that could be done with a far more limited proposal than this.

@burdges
Copy link

burdges commented May 13, 2020

There are parallels between bitfield access and Cell so maybe one should not do fancy *Guard types for bitfield types.

As an aside, we'd save more memory and better improve cache locality with NRVO ala #2884 because crates can easily burn considerable stack space now

@kennytm
Copy link
Member

kennytm commented May 13, 2020

(#2884 did not propose NRVO, just plain RVO. Also you don't need #2884 for the optimizer to apply NRVO.)

@ghost
Copy link
Author

ghost commented May 13, 2020

You should check if any proc macro crates for bitfields and/or packed enum create some Guard type ala MutexGuard that you explicitly borrow from the bitfield and/or packed enum. I'd expect some Guard type would remain the only way to pack this type into 1 byte because you must handled mutated data when taking references:

enum Foo {
    A(U8LessThan64>
    B(U8LessThan64>
    C(U8LessThan128>
}

I admit I don't know what a Guard is or how references are relevant here. But you mention that the size of Foo in your example could somehow be made 1 byte, and I can and want to say something about that.

First of all, as we can see from the code snippet below, the compiler doesn't optimize the size of Bar (a type similar to Foo in your example) to be 1 byte even when it could do so. This RFC doesn't suggest that the compiler should optimize the size of Bar nor do I think it should.

In order to optimize the size of Bar, the compiler would need to set some of the forbidden bits of U8LessThan2 and U8LessThan4in a Bar variable to indicate which enum variant (A, B, or C) the variable contains. And then, in order to retrieve the value in the field of the variant, the compiler would need to mask those forbidden bits out.

This is clearly a tradeoff between size (of Bar) and performance (pessimizing performance by adding an extra bitmasking operation) and not a pure optimization (by which I mean something that makes things strictly better, and not worse in any way). I'm not (and neither is this RFC) interested in making these kinds of tradeoffs. This RFC is interested in getting the compiler to do more of those "pure" optimizations that it is already making with types like Option<U8LessThan2>.

#[repr(u8)]
enum U8LessThan2 {
    _0, _1,
}

#[repr(u8)]
enum U8LessThan4 {
    _0, _1, _2, _3,
}

enum Bar {
    A(U8LessThan2),
    B(U8LessThan2),
    C(U8LessThan4),
}

const _: () = assert!(mem::size_of::<Bar>() == 2);

@clarfonthey
Copy link
Contributor

So, I can't actually find any good examples -- would someone be able to provide a link to some example of what the weird unsafe bitshifting looks like, as a way of creating forbidden values?

@pickfire
Copy link
Contributor

@ghost
Copy link
Author

ghost commented May 14, 2020

@clarfon Is https://github.com/maciejhirsz/beef/blob/856988b38cafa8c8f8f44f6f0923c717ce3f518b/src/lean.rs#L24-L29 considered weird enough?

AFAIK, that example uses a u64 value to store two u32 values in order to reduce the number fields in a struct from 3 to 2 (which boosts performance due to a specific aspect of calling convention). Then it uses bit masking and shifting to access those two u32 values embedded in the u64 value. But I don't see how any of that is relevant to this RFC or forbidden values.

@pickfire
Copy link
Contributor

Is forbidden values applied to existing NonZero* types?

@ghost
Copy link
Author

ghost commented May 14, 2020

There have been multiple comments here that mention bitfields in various contexts, and I haven't understood why people are talking about bitfields. What do bitfields have to do with this RFC? I've been thinking that it's me who doesn't understand something here because that happens a lot, but now I'm beginning to think that maybe some people have misunderstood what this RFC is trying to accomplish. And that would be clearly my fault.

So, I'll now try to explain better what this RFC is all about:

  1. The short version:

Give the compiler the ability to use the forbidden value(s) of any applicable (opted-in) user-defined type in order to represent data-less enum variants.

  1. The long version:

There are some types that should/must never hold certain (forbidden) value(s). Here are some examples of types with forbidden values that the compiler knows about and uses to represent data-less enum variants today:

  • Primitive types:
    bool, char, &T, &mut T

  • Standard library types:

std::{
    boxed::Box,
    num::{NonZeroI8, NonZeroU8, NonZeroI16, /* ... */},
    ptr::NonNull,
}
  • All unitary (i.e. data-less, c-like) enum types whose number of variants is less than the number of different values their internal representation can have. For example:
enum UnitaryEnum {
    A, B, C, D,
}

(I believe that list is not exhaustive)

Enum types can have both data-less variants and variants with data attached to them. Here's an example:

enum EnumType<T> {
    VariantWithData(T),
    VariantWithoutData1,
    VariantWithoutData2,
}

If you instantiate an EnumType<Foo> with type Foo that has at least two forbidden values that the compiler knows about, then the compiler doesn't create a separate tag (an integer) field inside EnumType<Foo> for indicating which variant it currently contains, but rather it stores only a Foo inside EnumType<Foo> and uses some two different forbidden values of Foo to indicate that EnumType<Foo> contains either VariantWithoutData1 or VariantWithoutData2, and it uses all the non-forbidden values of Foo to indicate that it contains a VariantWithData(foo_value). This optimization not only makes the size of EnumType<Foo> smaller than it would be with a separate tag, but it may result in better performance also due to the fact that the value of Foo that is being read is already in a register after the cpu has verified that EnumType<Foo> contains the VariantWithData variant.

Currently there are only two ways a user-defined type Foo can be such that the compiler knows it has forbidden value(s) (and what those values are) and then uses those values to perform the aforementioned optimization whenever possible: either Foo is a unitary enum type or it is a type that contains a field of some type the compiler already knows has forbidden value(s). The goal of this RFC is to be able to tell the compiler about forbidden values of user-defined types it currently doesn't and cannot know about.

@programmerjake
Copy link
Member

programmerjake commented May 14, 2020

(edit: added additional explanation and example usage)

There have been multiple comments here that mention bitfields in various contexts, and I haven't understood why people are talking about bitfields. What do bitfields have to do with this RFC? I've been thinking that it's me who doesn't understand something here because that happens a lot, but now I'm beginning to think that maybe some people have misunderstood what this RFC is trying to accomplish. And that would be clearly my fault.

The reason several people have mentioned bitfields and bitmasks is because some kinds of forbidden values are not one or a few ranges of values, instead, (like &u32) they have tons of forbidden values that are way easier to express using a bitmask:
For example, &u32 has forbidden values for address 0 (the null pointer) and for every address that is not a multiple of 4 (because u32 must be aligned to a multiple of 4 bytes). Expressing those forbidden values is easily done using:

  • The value 0
  • The bit mask expression v & 0x3 == 0x1
  • The bit mask expression v & 0x3 == 0x2
  • The bit mask expression v & 0x3 == 0x3

We would like to be able to express those kinds of forbidden values for our own custom types.

Now that I think about it, both kinds of forbidden value specifications could be combined into a single kind:
Any value v is a forbidden value when (v & mask) >= low && (v & mask) <= high for at least one element in a list of mask, low, high triples. Note that if mask is 0xFFF...FF then that is the same as just being in the range low..=high.

// replace as necessary to avoid depending on const generics if that causes problems:
struct ForbiddenValue<const N: usize> {
    mask: [u8; N],
    low: [u8; N],
    high: [u8; N],
}

impl<const N: usize> ForbiddenValue<N> {
    const fn is_forbidden(&self, value: &[u8; N]) -> bool {
        let mut cmp_low = Ordering::Equal;
        let mut cmp_high = Ordering::Equal;
        // would need to switch for into a while loop since const for doesn't currently work
        for i in 0..N {
            // compare in little endian order -- could switch to big endian by reversing iteration order
            // or match native endian
            let masked_byte = self.mask[i] & value[i];
            match masked_byte.cmp(&self.low[i]) {
                Ordering::Equal => {}
                v => cmp_low = v,
            }
            match masked_byte.cmp(&self.high[i]) {
                Ordering::Equal => {}
                v => cmp_high = v,
            }
        }
        cmp_low != Ordering::Less && cmp_high != Ordering::Greater
    }
}

trait ForbiddenValues<const N: usize> {
    const FORBIDDEN_VALUES: &'static [ForbiddenValue<N>];
}

Example usage:

#[repr(transparent)]
struct MyFakeReferenceType(u32);
// assumes little endian; 4 is size of u32
impl ForbiddenValues<4> for MyFakeReferenceType {
    const FORBIDDEN_VALUES: &'static [ForbiddenValue<4>] = &[
        ForbiddenValue { mask: [0xFF; 4], low: [0; 4], high: [0; 4] }, // exclude value 0
        // exclude values where (value & 3) >= 1 && (value & 3) <= 3
        ForbiddenValue { mask: [3, 0, 0, 0], low: [1, 0, 0, 0], high: [3, 0, 0, 0] },
    ];
}

@ghost
Copy link
Author

ghost commented May 14, 2020

I changed the design (in the RFC) a bit by removing the generic parameters because they would allow the trait to be implemented multiple times for a specific type.

Then it occurred to me, that it is already possible with the current design to define a large number of forbidden values easily by using a const block like in the following: (notice that this wouldn't be possible if ForbiddenValues::FORBIDDEN was a slice)

unsafe trait ForbiddenValues {
    type Forbidden: FixedSizeArray<[u8; mem::size_of::<Self>()]>;

    const FORBIDDEN: Self::Forbidden;
}

unsafe impl ForbiddenValues for f32 {
    type Forbidden = [[u8; 4]; 50];

    const FORBIDDEN: [[u8; 4]; 50] = {
        let mut forbidden = [[0u8; 4]; 50];
        let mut item = 1u32;
        let mut i = 0;

        while i < 50 {
            forbidden[i] = unsafe { mem::transmute(item) };
            item += 128;
            i += 1;
        }
        forbidden
    };
}

And I think it's unlikely that the compiler would need a huge number of forbidden values to perform its optimizations, so a reasonable number like 50 would probably suffice.

Another thing to note is that I believe that these (potentially large) FORBIDDEN arrays are completely compiled away from the final binary, as long as user doesn't access them.

@programmerjake
Copy link
Member

And I think it's unlikely that the compiler would need a huge number of forbidden values to perform its optimizations, so a reasonable number like 50 would probably suffice.

There are some optimizations that could be done in the future that would need large numbers of forbidden values, such as packing values into bitfields, so, to prevent getting stuck with an API that is really unsuited for those use cases, I think we should plan ahead by using an API that can succinctly express all forbidden values, especially including those that require bitmasks to describe succinctly (such as the type MyFakeReferenceType described in #2888 (comment))

@ghost
Copy link
Author

ghost commented May 15, 2020

There are some optimizations that could be done in the future that would need large numbers of forbidden values, such as packing values into bitfields

@programmerjake Could you elaborate a bit on what you mean by that?

And I'm going to anticipate your answer now, and comment a bit on it beforehand. Tell me if I'm wrong, but this is what I assume you meant by that:

There will be some bitfield-type that gives the user the impression (by doing bitwise operations behind the scenes) that one can store types of arbitrary bit widths inside it. In order to store a value of type T in, let's say, certain 6 bits of that bitfield-type, the compiler must verify that all the values of T fit in 6 bits (and which bits they are stored in T). You call that an optimization, but packing values into smaller types by doing bitwise operations is not an optimization, it is a tradeoff. Like I said before, I still maintain that bitfields have nothing to do with this RFC.

Another point I want to make, or rather suggest, is that we should assume that there will be a bounded integer type in the standard library at some point, and that the compiler will implicitly know about all the forbidden values of those types just like it does today with unitary enums. Such bounded integer types will remove a lot of the use cases for the ForbiddenValues trait in this RFC, including the use case for verifying that a (bounded integer) value fits in some "field" of some bitfield-type. And besides, third-party crates seem to be able to do bitfields just fine even today.

@ghost
Copy link
Author

ghost commented May 15, 2020

I think we should plan ahead by using an API that can succinctly express all forbidden values

@programmerjake I'd like to point out that the design you described cannot express all forbidden values. For example:

// Contract:
// For each value `f` of type `Foo`, `f.start <= f.end`
struct Foo {
    start: u32,
    end: u32,
}

The iterator based design and the predicate based design are the two designs that can express all forbidden values (not succinctly though, and neither one of them can, practically speaking, express forbidden bits). The predicate based design may not be feasible from compiler implementation's point of view. And the iterator based design has the caveat that it may not be able to use std::iter::Iterator due to the impl const requirement, so there would need to be some new alternative iterator trait, like:

pub trait AlternativeIterator {
    type Item;

    const LEN: usize;

    fn next(&mut self) -> Option<Self::Item>;
}

The current design is simple & not robust, and the iterator based design is complicated & robust. I think they represent the two extremes of the design space. In order to navigate that space, it would be good to get the compiler to tell us statistics about the maximum and the average number of forbidden values it needs to perform the optimizations in crates.io.

@Evrey
Copy link

Evrey commented May 16, 2020

Just to add perspective: I'm not just interested in more efficient enum packing. I'm interested in statically proving things about my data. So if I can't exhaustively and conveniently express a large number of invalid states, this RFC is — for me — useless.

@ghost
Copy link
Author

ghost commented May 16, 2020

I'm interested in statically proving things about my data.

@Evrey Can you elaborate on what you mean by that? I'm pretty sure "statically" means "at compile-time". But other than that, I just don't understand what you mean.

So if I can't exhaustively and conveniently express a large number of invalid states, ...

I take "exhaustively" to mean that you need to be able to express all invalid states. You also use the word "conveniently" - do you consider the external iterator based design convenient enough?

@jhpratt
Copy link
Member

jhpratt commented May 16, 2020

@Evrey Note that the trait is unsafe. The compiler will make assumptions based on what you tell it: it won't have to prove anything. Having to statically prove certain values aren't possible is extremely difficult if not impossible, let alone extraordinarily time consuming.

@burdges
Copy link

burdges commented May 16, 2020

We'll need compiler plugins for formal verification tools like refinement types because rustc performance would suffer if random crates started using those tools everywhere.

I'm kinda curious just how much performance hit we already take from generic-array, which winds up being an unecessary dependency of 500+ crates.

I think https://internals.rust-lang.org/t/pre-rfc-prefer-all-zeros-niche-for-niche-variant-optimization/12076 has a "wiser" conversation about the topics around this RFC.

@Evrey
Copy link

Evrey commented May 16, 2020

@tommit One example I repeatedly brought up is expressing start <= end as an invariant for ranges. But there's more, like len <= cap for a vector, etc.

As for the iterator: It still passes around byte buffers, which have the big endianess issue. And iterating over 16.8 million NaN values is… uhh… let's just say compile times will be horrible. Let's also imagine writing an f32 wrapper that only allows finite values in 0.0 ..= 1.0. Pretty useful for a lot of things. And now imagine the amount of invalid compared to valid state. And when iterating over all that, I'll need quite the complicated state machine, first iterating over out-of-range values, the infinities, and finally all the NaNs.

@jhpratt Well, yes, like with NonZeroU32 and friends initialisation will be dynamically checked. But when extracting the value, the compiler is informed about the given invariants and optimises accordingly.

@burdges Very true. But on the other hand, whoever uses refinement types (and I see this RFC as an opportunity for pretty much »poor man's refinement types«) or even full formal verification of the code is already okay with long compile times.

@jhpratt
Copy link
Member

jhpratt commented May 16, 2020

Yeah, it makes an assumption about the data because you said it could, basically. I think we're sort of saying the same thing.

@ghost ghost closed this May 16, 2020
@ghost ghost deleted the forbidden-value-trait branch May 16, 2020 18:36
```rust
*(&raw const t as *const [u8; size_of::<T>()]) != f
```
2. `T` must not be a type that has forbidden values the compiler already knows about (e.g. `char`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. char

How about other types, are there any other types that the compiler already knows about? Is it possible to lists them out?

Copy link
Contributor

@Ixrec Ixrec May 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how relevant this is now that the issue's closed, but https://doc.rust-lang.org/std/#primitives is a list of primitive types in the language. Which of these counts as "having forbidden values" depends on what you mean by that. For example, "uninitialized" is a forbidden / insta-UB-causing value for all of them, and str has to be valid UTF-8 within safe code but unsafe code can violate that without triggering UB, and I don't know if you'd want to include library types like NonZero*, and so on and so forth. But if we try to just ignore all of this crippling scope vagueness, I think bool is the only other primitive type that is "like char" insofar as it clearly has to be implemented as one of the integer types with certain values excluded (in bool's case, every value except 0 or 1).

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.