Facet implementation for type with invariants

[tyilo]

Is it supposed to be safe to implement Facet with #[derive(Facet)] for a type that has internal invariants?

If so, then facet_json::from_str needs to be unsafe:

use facet::Facet;

type T = u64;

#[derive(Facet)]
pub struct MyVec {
    pub inner: Vec<T>,
}

#[derive(Facet)]
pub struct AtLeastTwoVec {
    inner: Vec<T>,
}

impl AtLeastTwoVec {
    pub fn new(v: Vec<T>) -> Option<Self> {
        if v.len() < 2 {
            None
        } else {
            Some(Self { inner: v })
        }
    }

    pub fn second(&self) -> &T {
        // SAFETY: `inner` has at least length 2
        unsafe { self.inner.get_unchecked(1) }
    }
}

fn main() {
    assert!(MyVec::type_eq::<AtLeastTwoVec>());

    let v = MyVec { inner: vec![1] };
    let peek = facet_peek::Peek::new(&v);
    let json = facet_json::to_json_string(peek, false);

    let v: AtLeastTwoVec = facet_json::from_str(&json).unwrap();
    dbg!(v.second()); // Undefined behavior
}

[fasterthanlime]

Is it supposed to be safe to implement Facet

No, Facet is an unsafe trait.

[fasterthanlime]

But.. yeah. I see what you mean.

I don't think unsafe derives are a thing yet, really?

[Veykril]

But.. yeah. I see what you mean.

I don't think unsafe derives are a thing yet, really?

Not yet rust-lang/rfcs#3715

[tyilo]

I think this should be mentioned in the documentation for both the Facet trait and the Facet derive macro.

Maybe the derive trait can be renamed to UnsafeFacet to make this apparent?

[tyilo]

What is the plan for implementing Facet for NonZero<...>?

[fasterthanlime]

If so, then facet_json::from_str needs to be unsafe:

Maybe that's just the solution. It's true that building any type of the deployments facet is inherently unsafe unless you know what you're doing? I don't know.

Maybe the solution here is to expose some method to verify invariants? It wouldn't solve the naming issue or the documentation issue, but we could make from_str return an error if it actually deserializes into something that is not supposed to be representable.

I quite like this idea. In fact, it could be specified as an attribute in the derived macro, something like:

#[derive(Facet)]
#[facet(invariant = Self::at_least_two)]
pub struct AtLeastTwoVec {
    inner: Vec<T>,
}

Or something. It could even return an error value instead of just a bool so we have a nice user experience.

[JSorngard]

serde allows one to deserialize into an intermediate type and then implement a check for the invariant in a TryFrom implementation: https://serde.rs/container-attrs.html#try_from. Might that structure be interesting here?

Example:

#[derive(Deserialize)] 
#[serde(try_from = MaybeFive)] 
struct NotFive {
    int: i32, 
} 

#[derive(Deserialize)] 
struct MaybeFive(i32); 

impl TryFrom<MaybeFive> for NotFive {
    type Err = FiveErr;
    fn try_from(val: MaybeFive) -> Result<Self, Self::Err> {
    if val.0 != 5 {
        Ok(Self { int: val.0 })
    } else {
        Err(FiveErr)
    } 
} 

struct FiveErr;

impl Display for FiveErr {...}

[tyilo]

Should Facet be able to be implemented for structs with invariants that can't be checked at runtime?

Such as:

struct MyByteVec {
  // Safety invariant: `ptr` points to `len` bytes of memory
  ptr: *mut u8,
  len: usize,
}

I think MyByteVec should be able to implement Facet, however it can't implement any safe deserialization.

[fasterthanlime]

Should Facet be able to be implemented for structs with invariants that can't be checked at runtime?

Yes, but not through the derive, if that makes sense?

I really don't like the UX implications of renaming the derive macro to anything other than Facet, but... Someone from the compiler team was just asking me what we would need to make more progress on facets, so maybe we can get those unsafe derives shipped sooner rather than later??

[fasterthanlime]

I really don't like the UX implications of renaming the derive macro to anything other than Facet, but...

I'm starting to change my mind.

[madsmtm]

I'm not sure I agree with the first example being the fault of the Facet derive; rather, it's the unsafe { self.inner.get_unchecked(1) } that's wrong, because you implemented Facet, and thus the type no longer upholds its invariants.
It'd be the same situation if you'd implemented Default, and we don't mark that trait as unsafe (remember: any unsafe extends to the entire module, just like Unpin is safe but can cause unsoundness elsewhere if implemented wrongly).

Are there other examples of Facet being unsound? If not, then I don't think it needs to be unsafe.

[fasterthanlime]

It'd be the same situation if you'd implemented Default, and we don't mark that trait as unsafe (remember: any unsafe extends to the entire module, just like Unpin is safe but can cause unsoundness elsewhere if implemented wrongly).

That's actually a really good argument!

[JSorngard]

It's also the logic serde uses. Deriving serde::Deserialize has these same pitfalls as deriving Facet does.

Though a method for enforcing invariants during deserialization would be very nice.

[fasterthanlime]

Though a method for enforcing invariants during deserialization would be very nice.

Any ideas on how to allow returning errors there without falling into "no context" or "&'static str only" or "woops we require an allocator now"?

It would be nice to get the details of what invariant failed.... maybe &'static str is not that bad.

[JSorngard]

I don't think I personally can think of anything except &'static str unfortunately, though I think something like this is beyond my expertise.
If the user-provided function that checks the invariant is given the (maybe incorrectly) deserialized value as input the user could at least return one of a few different &'static strs depending on which invariant is violated.

[fasterthanlime]

I think we can handle MyByteVec like this:

I think so too. The other thing was mutation once something is already fully initialized. We don't want people to be able to just drop the poke and leave the original mutably borrowed value in some invalid state. I honestly don't know how to do this.

I think the poke should just panic if it's dropped, but forget is safe, so there's basically no option for this.

The version of poke that works on a mutable reference rather than taking ownership of the thing is going to be unsafe no matter what, even if it checks the invariants.

[fasterthanlime]

Mutation is also an issue even once you get a fully initialized struct. For now, my solution is to go back to a PokeStructUninit, do your manipulations there, and then try to build in place again or something.

#188 landed, and it doesn't even allow direct mutation, only building from uninitiated to fully initialized.

Invariants can be checked by adding a facet(invariant = method_name) attribute on the container. More functionality can be tracked in other, smaller issues.

[tmandry]

Hi @fasterthanlime, I'm curious whether unsafe derives as in rust-lang/rfcs#3715 still be useful to you, or would they change the way you approached your implementation? Or did the way you ended up solving this feel unambiguously better to you?

[fasterthanlime]

Hi @fasterthanlime, I'm curious whether unsafe derives as in rust-lang/rfcs#3715 still be useful to you, or would they change the way you approached your implementation? Or did the way you ended up solving this feel unambiguously better to you?

I think so? I'm not entirely sure. I'm still torn because.. deriving Default if you have a len > 0 invariant is a bug, but are we going to make deriving Default unsafe too? If so, then Facet should be unsafe. If not, then no.

[tmandry]

Yeah, violating internal invariants does not itself justify unsafe. What would justify it is if there are invariants depended on by unsafe code (and then we can argue that it's up to the unsafe to ensure there are no derives like Facet that can violate that). There's also work on unsafe fields in Rust that might be interesting here; you could imagine a safe derive not working with those, but declaring an unsafe attribute like #[unsafe(facet_deserialize_safe)] or something like that on one of those.

[tyilo]

I think an important difference between Default (or serde::Deserialize or serde::Serialize) and Facet is that Default only has one purpose: Constructing a default value.

The Facet trait can used for many different things:

1. Inspecting the shape of a type.
2. Inspecting the fields of a value.
3. Constructing a value by initializing its fields one at a time.
4. Changing a field of an existing value.

Even if a type has invariants, I don't see how deriving Facet could cause problems with 1 and 2. However, deriving Default will either always or never cause problems for a type.

I see three options:

1. Keep the Facet trait as-is and document the criteria for when it is safe to derive.
2. Make Facet "unsafe" to derive somehow and document the criteria for when it is safe to derive.
3. Split Facet into two traits, one of them always safe to derive. The safe trait could cover 1 and 2 above and the unsafe trait could cover 3 and 4.

I think 3 would be the best to prevent footguns.

[GoldsteinE]

I think they both could be safe: when you’re deriving FacetMut (or whatever it’s going to be called), you know that you effectively make all your fields publicly writable.

I’m not sure that’s the best solution though. The problem with the full reflection design like Facet does is that it’s a giant semver hazard: if most types in the ecosystem derive Facet, then most types in the ecosystem make their internal structure public API. I feel like it’s important for Facet to follow privacy rules in some way, so you can derive Facet and use full reflection on your types inside of your own crate, but the downstream crates only see information about the public fields (and something like non_exhaustive: true). The easiest way I see to implement this is something like

impl TypeImplementingFacet {
    pub(crate) fn pub_crate() -> impl Facet;
    fn priv() -> impl Facet;
}

so to access information about private fields you have to go through .priv().

[fasterthanlime]

The Facet trait can used for many different things:

1. Inspecting the shape of a type.
2. Inspecting the fields of a value.
3. Constructing a value by initializing its fields one at a time.
4. Changing a field of an existing value.

3. is the same as Default insofar as, in safe code, facet doesn't let you leave something "partially uninitialized". The Wip API has been designed so that you have to initialize everything, or you can't get a value out. If it's not fully initialized, the inner fields will be dropped properly and everything will be freed. All in safe code. This is a big value proposition of facet actually, IMHO.

As for 4., it cannot be done in safe code at all in facet. Right now you can do it in that you can get the offset of the field, but then to write to it, you have to resort to unsafe.

So I stand by my argument that, with the new Wip interface (it wasn't the case before), deriving Facet trait is not more unsafe than deriving Default, or the offset_of! macro.

[tyilo]

So I stand by my argument that, with the new Wip interface (it wasn't the case before), deriving Facet trait is not more unsafe than deriving Default, or the offset_of! macro.

But what if I only want to derive Facet to inspect the type/value without allowing construction?

[tyilo]

I think they both could be safe: when you’re deriving FacetMut (or whatever it’s going to be called), you know that you effectively make all your fields publicly writable.

True :)

[fasterthanlime]

The problem with the full reflection design like Facet does is that it’s a giant semver hazard: if most types in the ecosystem derive Facet, then most types in the ecosystem make their internal structure public API.

@tyilo @GoldsteinE what use-cases do you see for in-place mutation besides like.. debuggers? Which are inherently unsafe?

It would be interesting if facet let crates annotate "this field is fine to change by yourself, here's the validation function" or something, but it's not... I feel It's not the primary objective.

And it's not relevant to the discussion of whether deriving Facet should be safe or unsafe. I'd be fine with either, but I still haven't heard a compelling argument why it's different from Default. I think @tmandry's phrase here:

Yeah, violating internal invariants does not itself justify unsafe

So that settles that as far as I'm concerned — the rest I'm interested in discussing of course (but maybe separately).

---

But what if I only want to derive Facet to inspect the type/value without allowing construction?

Then invariants returns false unconditionally? It'd be neat to have some marker trait or flag or something that says Unbuildable or whatever but... you can already today on facet 0.8 prevent building your types.

(You can also just declare it as scalar/opaque and then there's no way to build it through Wip either.)

[tyilo]

Then invariants returns false unconditionally? It'd be neat to have some marker trait or flag or something that says Unbuildable or whatever but... you can already today on facet 0.8 prevent building your types.

(You can also just declare it as scalar/opaque and then there's no way to build it through Wip either.)

Hmm, maybe it should be considered if implementers should be forced to write #[facet(invariants = none)] when deriving Facet for a type without any field invariants?

---

I guess having two traits could also move some errors from runtime to compile-time:

trait FacetConstruct: Facet {}

impl Facet for MyUnbuildableType {}

fn deserialize<T: FacetConstruct>(...) -> Result<T> { ... }

deserialize::<MyUnbuildableType>(...)
// ERROR: MyUnbuildableType doesn't implement FacetConstruct

[fasterthanlime]

Note: I converted this to a discussion since I don't consider it an issue to fix, but there's interesting conversation I don't want to lose track of.

[GoldsteinE]

what use-cases do you see for in-place mutation besides like.. debuggers?

I’m sorry, I think I was unclear here: I’m not talking about in-place mutation here specifically: reflection is inherently a semver hazard, even if in-place mutation is not supported (in fact, in-place mutation could be emulated by constructing a new value with a changed field and then overwriting the old value).

When you derive Facet, you make a semver promise that you’ll never delete any of the fields. When you derive FacetConstruct (i.e. allow user to create your struct field-by-field), you make a semver promise that you’ll never add any fields (without default values?). That’s effectively very close to making all the fields public. You also can make an argument that you promise not to reorder fields (as field order is observable with Facet, and may be important for e.g. binary serialization formats).

That said, this is not very far from the kinds of semver promises you make when you derive Serialize and Deserialize, so maybe that’s OK? I would love to see a more granular approach though. Every time #[derive] would make a semver promise you don’t want to make, you need to implement Facet by hand, and looking at the docs, I have a feeling that it might be harder than implementing Serialize.

[GoldsteinE]

I don't think that's necessarily true

Can you explain your reasoning here? As I see it, deriving (read-only) Facet provides at least as much access as theoretical

pub struct Foo {
    pub(read-only) field: Bar,
}

and I would definitely consider removing this field a breaking change.

[tyilo]

@GoldsteinE

Would you consider changing the output of Debug formatting of a type a server breaking change?

It is observable behavior but normally it isn't expected that you use the debug output for logic.

[GoldsteinE]

No, I would consider Debug representation to be exempt from stability guarantees: you’re not supposed to parse it.

On the other hand, Facet has a very broad scope, which explicitly includes serialization. I would totally consider changing implementation of Serialize incompatibly a breaking change.

[tyilo]

Ah, of course here Facet needs to decide if it wants to be a serialization library or a reflection library.

For serialization purposes a String normally only has a list of bytes or chars. For reflection purposes I would say it also has a capacity field.

[fasterthanlime]

I disagree with the binary framing (no pun intended), I think it’s still reflection if you consider trying to be a primitive or a scalar, which is exactly what facet does and I think what Scheme, Java etc. would do as well

[JSorngard]

Sorry to but in, but does this mean that if I make a type that has some internal invariant I should not derive Facet for it, regardless of whether that invariant is relied upon by unsafe code? Since if I do I can not rely on the invariant being upheld anywhere.

[fasterthanlime]

Right now, if you have some internal invariant you should use the invariants attribute, see:

https://github.com/facet-rs/facet/blob/19343cdd3fb2d36a357d1896b63345bd3ddf5ca6/facet-reflect/tests/wip/invariant.rs

[tyilo]

That example should probably be updated to set the field and not use an already constructed invalid instance.

[tmandry]

If I was thinking of this as a language feature, where perhaps something like #[derive(Facet)] was implicit and done for every type, I would design it so that

• The default derive did not give code outside your module permissions above what it could already write manually (i.e., no access to private fields).
• You can opt in to providing access, separately for both reading and writing, to private fields. For inspiration see the restrictions RFC.
• There is some notion of a visibility-scoped permission to reflection, i.e. code in the same module as the type is defined could do everything. This would be difficult to model in facet today, but I think it could be simulated with some kind of macro that defines a new private or pub(crate) Facet trait (with a new name, like FacetLocal) and imports a derive macro for that trait.