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

[swift] distinguish unknown from absent enum value for scheme evolution #8231

Open
blindspotbounty opened this issue Feb 6, 2024 · 5 comments

Comments

@blindspotbounty
Copy link
Contributor

We discovered that on enum scheme evolution we cannot distinguish absent value from unknown one.

There is an example of scheme evolution.
First version:

/// enums_v1.fbs
namespace EnumUnknown;

enum A: byte {
    a,
    b,
}

table foo {
    value:A=null;
}

Second version:

namespace EnumUnknown;

enum A: byte {
    a,
    b,
    c,
}

table foo {
    value:A=null;
}

Generating code for them:

flatc -o . --swift enums_v1.fbs
flatc -o . --swift enums_v2.fbs --conform enums_v1.fbs

Code for v1:

public enum EnumUnknown_A: Int8, Enum, Verifiable {
  public typealias T = Int8
  public static var byteSize: Int { return MemoryLayout<Int8>.size }
  public var value: Int8 { return self.rawValue }
  case a = 0
  case b = 1

  public static var max: EnumUnknown_A { return .b }
  public static var min: EnumUnknown_A { return .a }
}

public struct EnumUnknown_foo: FlatBufferObject, Verifiable {
...
  public var value: EnumUnknown_A? { let o = _accessor.offset(VTOFFSET.value.v); return o == 0 ? nil : EnumUnknown_A(rawValue: _accessor.readBuffer(of: Int8.self, at: o)) ?? nil }
...
  public static func createfoo(
    _ fbb: inout FlatBufferBuilder,
    value: EnumUnknown_A? = nil
  ) -> Offset {...}
}

Code for v2:

public enum EnumUnknown_A: Int8, Enum, Verifiable {
  public typealias T = Int8
  public static var byteSize: Int { return MemoryLayout<Int8>.size }
  public var value: Int8 { return self.rawValue }
  case a = 0
  case b = 1
  case c = 2

  public static var max: EnumUnknown_A { return .c }
  public static var min: EnumUnknown_A { return .a }
}

public struct EnumUnknown_foo: FlatBufferObject, Verifiable {

  public var value: EnumUnknown_A? { let o = _accessor.offset(VTOFFSET.value.v); return o == 0 ? nil : EnumUnknown_A(rawValue: _accessor.readBuffer(of: Int8.self, at: o)) ?? nil }
...
  public static func createfoo(
    _ fbb: inout FlatBufferBuilder,
    value: EnumUnknown_A? = nil
  ) -> Offset {...}

Now, from v2 I can create table with:

EnumUnknown_foo.createfoo(&fbb, .c) // unknown for v1 value

or:

EnumUnknown_foo.createfoo(&fbb) // absent enum value

If I send and read the same buffer in older version, it won't be possible to distinguish unknown value or its absence:

func process(_ table: EnumUnknown_foo) {
    table.value // is nil for both cases
}

Could you suggest if it would be possible to implement distinguishing these two variants, please?

@mustiikhalil
Copy link
Collaborator

Hej
Thanks for opening this issue.
1- A solution would be to create an init within the enums that check if that validates a value, and if it's not correct we return an unknown case. which would require a way to make sure that the user is not going to return that value (example: infinitely)
2- Another one would be to basically check if the value is there before _accessor.readBuffer(of: Int8.self, at: o) and having a bool for that to validate if it's
3- Throw and error?! if the value is there but couldn't be mapped

@blindspotbounty
Copy link
Contributor Author

Hi @mustiikhalil
Thank you for the quick answer!

I believe that ideally would be (1) with some extension (maybe under option) to generate RawRepresentable structure instead of enum.
E.g. like the following:

public struct EnumUnknown_A: RawRepresentable, Enum, Verifiable {
  public typealias T = Int8
  public static var byteSize: Int { return MemoryLayout<Int8>.size }
  public var value: Int8 { return self.rawValue }
    
  public var rawValue: Int8
    
  public static var a: EnumUnknown_A = .init(rawValue: 0)
  public static var b: EnumUnknown_A = .init(rawValue: 1)

  public static var max: EnumUnknown_A { return .b }
  public static var min: EnumUnknown_A { return .a }
}

That way, any unknown value can be propagated to the client code while structure remain RawRepresentable.
If it is inline with your envision, or you are okay to have this under option - I think I can add that to flatc.

What do you think about that?

I like both other options - they can always be a part of generated code.

@mustiikhalil
Copy link
Collaborator

mustiikhalil commented Feb 7, 2024

@blindspotbounty The whole idea of having it as an enum is that the options are constrained so I don't think making it a struct would be an option here.

However, after a bit of thinking maybe we can do something like this:

// Flatbuffers
// - Sources/EnumValues.swift
public enum EnumValues<T> where T: Enum {
 case found(v: T)
 case unknown(v: Any)
}

And then we can add an API that does the following:

public enum EnumUnknown_A: Int8, Enum, Verifiable {
...
  public static func wrapped(_ v: Int8) -> EnumValues<EnumUnknown_A> {
     guard let e = EnumUnknown_A(rawValue: v) else { return .unknown(v: v) }
     return .found(v: e)
   }
}

public struct EnumUnknown_foo: FlatBufferObject, Verifiable {
...
   /// New API?
  public var value: EnumValues<EnumUnknown_A>? { 
    let o = _accessor.offset(VTOFFSET.value.v)
    if o == 0 { return nil }
    return EnumUnknown_A.wrapped(_accessor.readBuffer(of: Int8.self, at: o)
   }

  // maybe keep the original one
  public var value: EnumUnknown_A? { let o = _accessor.offset(VTOFFSET.value.v); return o == 0 ? nil : EnumUnknown_A(rawValue: _accessor.readBuffer(of: Int8.self, at: o)) ?? nil }
...  
}

Or we can also leverage the Swift.Result<V, Error> so we don't reinvent the wheel and just return a FlatbufferError.unknownValue(v: Any).

What do you think?

From my point of view this is safer than allowing the user to get any value from the buffer, instead of just creating a struct that you can pass to it any value kinda

@blindspotbounty
Copy link
Contributor Author

@mustiikhalil thank you for you input and I apologize for the late answer.

That looks good to me. I think I can try propose PR next days if you don't mind, do you think we should do it under option or add some new accessors indefinitely?

Btw, have one more use case comes to mind (it is out of scope but nice to have in future) is to allow to propagate such field.
E.g. the following table:

namespace EnumUnknown;

enum A: byte {
    a,
    b,
    c,
}

table FooSnapshot {
    value:A=null;
}

table FooUpdate {
    value:A=null;
}

and the corresponding code:

extension EnumUnknown_Foo {
...
mutating func update(_ update: EnumUnknown_FooUpdate) {
    if let value = update.value {
        self.mutate(value: value)
    }
}

@mustiikhalil
Copy link
Collaborator

@blindspotbounty No worries

That looks good to me. I think I can try propose PR next days if you don't mind.

Yeah that would be amazing!

do you think we should do it under option or add some new accessors indefinitely?

So I don't want to add extra flags to the code generator so i would say just adding it normally would be alright. However we will keep the original one. So what i think would be useful is something like this:

// Original
public var value: EnumUnknown_A? { ... }

// + new one
public var unsafeValue: EnumValue<EnumUnknown_A>? { ... }

Since in theory we are reading memory that's not planned for with the generated code. and thus making it unsafe although its been verified that its within the buffer.

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

No branches or pull requests

2 participants