enum refactoring #6158

krojew · 2020-10-05T09:04:57Z

This PR is an alternative solution to unsoundess in #5581. The benefits over #6098 are:

Enums are kept as proper enum types.
Untrusted buffers now have fallible enum accessors for safe and future-compatible access.
Trusted buffers have unsafe accessor which doesn't incur any performance penalty due to transmutation. The user is responsible for ensuring the buffer is trusted.

This solution is also compatible with potential future verifier returning trusted buffer types.

ImmemorConsultrixContrarie · 2020-10-05T12:30:21Z

Currently thiserror does not support no_std environment, and using this as a dependency would make transition to no_std harder.

And generated code for the checks could have some problems. For example, big matches are awful for debug builds, see this issue. This problem could be fixed by range matching, i.e.

match x {
    x if -1 <= x && x <= 2 => (),
    _ => return Err(()),
}
Ok(unsafe { transmute(x) })

CasperN · 2020-10-05T18:29:41Z

I think you need a way to write unrecognized enums into a flatbuffer.

An example use case is a proxy service that talks to embedded devices that report an integer that's really an enum. Maybe after a firmware update a new int is reported and a monitoring service behind the proxy recognizes it but the proxy hasn't been updated yet. In this scenario, being unable to write an invalid enum value will lead to either an outage or a coupling of releases between the proxy service and the embedded devices's firmware.

Imo Result<NativeEnum, i8> would be the better representation.

Also, I think instead of transmuting in unsafe, you should safely return the base type. Maybe _raw is a good suffix for that. Users can do the unsafe transmute themselves.

krojew · 2020-10-05T18:34:39Z

Rather than returning an int as an error, it's better to put it in an error. Will wait for more comments and make changes tomorrow.

aardappel · 2020-10-05T19:25:20Z

I will restate that I think it is a really bad idea for an enum to have an accessor to return Result. This is really unergonomic, and is going to cause future schema changes to unexpectedly break old code.

FlatBuffer enum values are a set of integers of which you may not know all the possible values. This concept apparently does not match cleanly onto Rust enums, so I'd suggest using an implementation technique that represents them better, such as #6098

FlatBuffers has its own semantics, and it is important that all participating languages are able to access that semantics as faithfully as possible, by whatever means they have available. For example, in Java, we don't use the built-in enum feature either, because it is a heap allocated object :) Heck, Java (and most languages) can't even represent FlatBuffer strings directly, and its rather painful.

krojew · 2020-10-05T19:29:58Z

@aardappel this PR contains direct int to enum casts exactly for the case you describe - when someone does not want to deal with a Result and trusts a buffer. It's exactly what the c++ version does. For people wanting to be extra safe, the Result version is also provided. This way we're satisfying both use cases while keeping the code idiomatic.

krojew · 2020-10-05T19:34:04Z

As for compatibility - this change also solves this problem with both apis present. There is no way to distinguish programmatically if an incoming invalid value comes from an updated schema, or from an invalid buffer, so using the Result for people wanting to protect against such situations, gives them the possibility to do so in a Rust way without sacrificing strong enum type.

aardappel · 2020-10-05T19:42:18Z

@krojew this seems to leak unsafe into the caller which the other PR doesn't seem to do.

Multiple accessors is not great. At the very least I'd switch the naming, such the ergonomic method has the field name, and the other one appends _checked or whatever. Or maybe we can emit a generic range-check function for people that want to check their enums manually.

aardappel · 2020-10-05T19:45:08Z

In the end, it's for you rustaceans to decide. I am just giving the FlatBuffers perspective :)

krojew · 2020-10-05T19:48:57Z

We can tweak the naming, although in Rust, the conversion is to use _unchecked where checks are omitted, not the other way around. Given people use IDEs with code completion, I suggest going the "traditional" route, although I'm fine either way.
The unsafe leak is intentional, since such c++ style casts are inherently unsafe. Rust wants this to be explicit, while c++ doesn't care, as usual 🙂 Such accessor is for people knowing that, hence unsafe.

CasperN · 2020-10-05T19:51:13Z

this PR contains direct int to enum casts exactly to the case you describe - when someone does not want to deal with a Result and trusts a buffer. It's exactly what the c++ version does. For people wanting to be extra safe, the Result version is also provided. This way we're satisfying both use cases while keeping the code idiomatic.

If you're referring to the unsafe transmute thing, that is not forwards compatible. Its UB. Its actually Rust-only UB too since C++ is not that opinionated about enums.

As for compatibility - this change also solves this problem with both apis present. There is no way to distinguish programmatically if an incoming invalid value comes from an updated schema, or from an invalid buffer, so using the Result for people wanting to protect against such situations, gives them the possibility to do so in a Rust way without sacrificing strong enum type.

I agree, the main distinction between this and #6098 is confining the handling of unrecognized variants to the Err of the Result. This allows exhaustive matching in the Ok branch, in which case code breaking is desirable. I would never do this in a project involving more than like 3 teams, but I think its a justifiable design choice.

I still think going the zero-copy + language neutral route in #6098 is the right way to go, but my (perhaps terrible) compromise is that we can offer the result-y thing behind the object API which is meant to be more native.

@rw pls decide for us

krojew · 2020-10-05T19:56:51Z

@CasperN for forwards compatibility the Result version is provided. Or rather will be because I forgot to put the actual invalid value in the error, which I'll fix tomorrow. The transmutation is for people trusting the buffer is valid at this point in time. The PR, in fact, combines the Rust and c++ approach into one. The user can decide which is appropriate for a given situation.

rw · 2020-10-05T21:24:47Z

Thanks @krojew for making this PR!

Here is my perspective:

Transmute probably won't work for enum types that are wider than one byte.
While we could add _unchecked methods to all accessors, I think that I'd rather solve the bigger and more fundamental problem of us writing a Verifier in Rust. Once a Verifier is written, we can use that to figure out a way to safely elide bounds checks (if the user desires to use unsafe). This would allow us to talk about the entire buffer as being "checked" or "unchecked", not just particular fields.

Idea: Allow users to specify a feature flag in the crate that prevents the use of unsafe code.

CasperN · 2020-10-05T22:54:00Z

I think that I'd rather solve the bigger and more fundamental problem of us writing a Verifier in Rust

Let's consider that out of scope for now. I filed #6161 to track that effort.

krojew · 2020-10-06T05:44:05Z

@rw I absolutely agree buffer validation is the answer, but I think we should have something in place before we get one. Having c++-like casts solves the enum unsoundness issue we have for now. Also - why do you think transmute won't work for wider types? Their bit representation is the same, after all.

krojew · 2020-10-06T07:27:59Z

I brought back thiserror dependency since it makes life easier and we're not no_std anyway. We'll cross that bridge when we get there.

rw · 2020-10-06T09:02:42Z

@krojew transmute isn't endian-safe and we support big-endian hosts.

krojew · 2020-10-06T09:07:06Z

@krojew transmute isn't endian-safe and we support big-endian hosts.

get() already takes care of endianess, so we're transmuting proper scalars.

krojew · 2020-10-06T10:48:12Z

I think you need a way to write unrecognized enums into a flatbuffer.

An example use case is a proxy service that talks to embedded devices that report an integer that's really an enum. Maybe after a firmware update a new int is reported and a monitoring service behind the proxy recognizes it but the proxy hasn't been updated yet. In this scenario, being unable to write an invalid enum value will lead to either an outage or a coupling of releases between the proxy service and the embedded devices's firmware.

I'm on the fence about this. On one hand it's a legitimate use case, but on the other, it's putting invalid (at given point in time) values in the buffer in hope the receiving end will know how to interpret them. I personally see this as abusing the schema. Such fields should rather be integers than enums. @aardappel what do you think?

CasperN · 2020-10-06T15:44:59Z

rust/flatbuffers/src/error.rs

+#[derive(Error, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash, Debug)]
+pub enum ConvertError<T: Debug + Display> {
+    #[error("invalid value in buffer: {0}")]
+    InvalidValue(T),


I think this should be called UnknownVariant or UnrecognizedVariant and that it is incorrect to say a variant is "invalid" since it could be created by a future version of the schema.

That's a good point.

CasperN · 2020-10-06T15:55:48Z

tests/monster_test_generated.rs

+        }
+    }
+}
+
 #[allow(non_camel_case_types)]
 pub const ENUM_VALUES_RACE: [Race; 4] = [


Please deprecate ENUM_VALUES_RACE, ENUM_NAMES_RACE, and enum_name_race, and make them associated constants of Race; or leave a TODO to do so in a future pull

These might be useful. Are there reasons to deprecate them?

I think its more idiomatic for these to be associated constants with namespaced names rather than old-c-style prefixy names.
e.g. Race::VALUES instead of ENUM_VALUES_RACE.

While we're renaming things we should give people some opportunity to refactor their code, i.e. deprecate and give a deletion timeline.

CasperN · 2020-10-06T16:05:30Z

tests/monster_test_generated.rs

+    self._tab.get::<i8>(Monster::VT_SIGNED_ENUM, Some(-1)).map(|value| value.try_into()).unwrap()
+  }
+  #[inline]
+  pub unsafe fn signed_enum_unchecked(&self) -> Race {


There should be an method for getting the raw i8 value from signed_enum directly.

I'll add reverse TryFrom.

Or rather From, since it's infallible.

Or rather From, since it's infallible.

No, not a Enum as i8, but instead a safe infallible getter that gets i8 from the struct.

aardappel · 2020-10-06T22:51:48Z

@krojew

I'm on the fence about this. On one hand it's a legitimate use case, but on the other, it's putting invalid (at given point in time) values in the buffer in hope the receiving end will know how to interpret them. I personally see this as abusing the schema. Such fields should rather be integers than enums. @aardappel what do you think?

FlatBuffers semantics say that such unknown values are possible, and implementations should just ignore them if they don't know what they are. If you must assume that an unknown integer is a valid enum value "from the future", then being able to copy these values is legit as well.

Or put differently, given any particular schema, there are known integer values, and every other integer value is fair game for a future enum value. Thus, no integer value (that fits the underlying type) is ever illegal.

krojew · 2020-10-07T05:31:00Z

FlatBuffers semantics say that such unknown values are possible, and implementations should just ignore them if they don't know what they are. If you must assume that an unknown integer is a valid enum value "from the future", then being able to copy these values is legit as well.

From my understanding, we are not interpreting, nor copying an unknown value, but producing one.

aardappel · 2020-10-07T15:35:18Z

@krojew

From my understanding, we are not interpreting, nor copying an unknown value, but producing one.

while that is certainly more odd, it is not something that would count as illegal from the FlatBuffers side.

enum refactoring

e2f1b58

removed thiserror dependency

8665f99

CasperN mentioned this pull request Oct 5, 2020

Rework enums in rust. #6098

Merged

storing invalid value in an error

cbfba91

ImmemorConsultrixContrarie mentioned this pull request Oct 6, 2020

Implement Flatbuffer Verifier for Rust #6161

Closed

CasperN reviewed Oct 6, 2020

View reviewed changes

krojew added 3 commits October 6, 2020 18:14

Merge branch 'master' into rust-enums

5e4aea1

renamed error

eb928dd

raw enum accessors

19a8981

deprecated globals in favor of associated constants

7e807ca

krojew closed this Oct 21, 2020

krojew deleted the rust-enums branch October 21, 2020 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enum refactoring #6158

enum refactoring #6158

krojew commented Oct 5, 2020

ImmemorConsultrixContrarie commented Oct 5, 2020

CasperN commented Oct 5, 2020

krojew commented Oct 5, 2020

aardappel commented Oct 5, 2020

krojew commented Oct 5, 2020 •

edited

Loading

krojew commented Oct 5, 2020

aardappel commented Oct 5, 2020

aardappel commented Oct 5, 2020

krojew commented Oct 5, 2020

CasperN commented Oct 5, 2020

krojew commented Oct 5, 2020

rw commented Oct 5, 2020 •

edited

Loading

CasperN commented Oct 5, 2020

krojew commented Oct 6, 2020

krojew commented Oct 6, 2020

rw commented Oct 6, 2020

krojew commented Oct 6, 2020

krojew commented Oct 6, 2020

CasperN Oct 6, 2020

krojew Oct 6, 2020

CasperN Oct 6, 2020

krojew Oct 6, 2020

CasperN Oct 6, 2020

CasperN Oct 6, 2020

krojew Oct 6, 2020

krojew Oct 6, 2020

ImmemorConsultrixContrarie Oct 6, 2020

krojew Oct 6, 2020

aardappel commented Oct 6, 2020

krojew commented Oct 7, 2020

aardappel commented Oct 7, 2020

enum refactoring #6158

enum refactoring #6158

Conversation

krojew commented Oct 5, 2020

ImmemorConsultrixContrarie commented Oct 5, 2020

CasperN commented Oct 5, 2020

krojew commented Oct 5, 2020

aardappel commented Oct 5, 2020

krojew commented Oct 5, 2020 • edited Loading

krojew commented Oct 5, 2020

aardappel commented Oct 5, 2020

aardappel commented Oct 5, 2020

krojew commented Oct 5, 2020

CasperN commented Oct 5, 2020

krojew commented Oct 5, 2020

rw commented Oct 5, 2020 • edited Loading

CasperN commented Oct 5, 2020

krojew commented Oct 6, 2020

krojew commented Oct 6, 2020

rw commented Oct 6, 2020

krojew commented Oct 6, 2020

krojew commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aardappel commented Oct 6, 2020

krojew commented Oct 7, 2020

aardappel commented Oct 7, 2020

krojew commented Oct 5, 2020 •

edited

Loading

rw commented Oct 5, 2020 •

edited

Loading