Scalapb: (Sealed) OneOf without Empty case

[sideeffffect]

Currently, for one_of (both sealed_value or normal one_ofs) generates not just the normal described cases, but also an extra Empty value. E.g.

message ContainingMyOneof {
  oneof nonsealed_value {
    HelloRequest request = 1;
    HelloReply reply = 2;
  }
}

generates

final case class ContainingMyOneof(nonsealedValue: ….ContainingMyOneof.NonsealedValue)
…
sealed trait NonsealedValue …
case object Empty extends ….ContainingMyOneof.NonsealedValue
final case class Request(value: ….HelloRequest) extends ….ContainingMyOneof.NonsealedValue
final case class Reply(value: ….HelloReply) extends ….ContainingMyOneof.NonsealedValue

We would need ScapaPB not to generate these extra Empty case objects.
I see at lease two ways how to do that:

1. ContainingMyOneof#nonsealedValue would not be ContainingMyOneof.NonsealedValue, but Option[ContainingMyOneof.NonsealedValue]. That would be the most regular approach, ScalaPB encodes other fields this way too. The generated code would look like:

final case class ContainingMyOneof(nonsealedValue: Option[….ContainingMyOneof.NonsealedValue])
…
sealed trait NonsealedValue …
final case class Request(value: ….HelloRequest) extends ….ContainingMyOneof.NonsealedValue
final case class Reply(value: ….HelloReply) extends ….ContainingMyOneof.NonsealedValue

1. ScapaPB's decoder would throw an exception if it would encounter a would be Empty input. The generated code would look like:

final case class ContainingMyOneof(nonsealedValue: ….ContainingMyOneof.NonsealedValue)
…
sealed trait NonsealedValue …
final case class Request(value: ….HelloRequest) extends ….ContainingMyOneof.NonsealedValue
final case class Reply(value: ….HelloReply) extends ….ContainingMyOneof.NonsealedValue

Both of these solutions would be significant changes, so I presume they could be non-default, hidden behind some king of option/setting.

I would contribute the code to do that, but wanted to discuss this with ScalaPB maintainers before writing code.

relevant tickets/PRs:
https://github.com/scalapb/ScalaPB/issues/455
https://github.com/scalapb/ScalaPB/pull/526
https://github.com/scalapb/ScalaPB/issues/578

[thesamet]

One assumption that ScalaPB makes is that every message has a defaultInstance where all the fields are initialized to their default values (0 for numbers, "" for strings, and so on). When empties are not allowed, what would be the default value of this type?

[sideeffffect]

we're now talking about solution 2., right?
is it possible that there be no defaultInstance and when the decoder comes across an "empty", it would throw?

btw, would solution 1. be acceptable to ScalaPB?

[thesamet]

The title of this issue says "(Sealed) ..." but the rest of the ticket text is about non-sealed. To reduce confusion, can you edit the title and description to match?

Option 2 would require defaultInstance defined on ContainingMyOneof to throw. As a result every message containing ContainingMyOneof will have a defaultInstance that throws and so on. Doesn't sound like a good situation.

Option 1 is more doable, but like you said it needs to be provided in addition to the existing way, and it would come with additional maintenance cost and code complexity for the generator. Can you explain the motivation for this feature request?

[sideeffffect]

The title of this issue says

In my example I've used only non-sealed, but my suggestion applies to both sealed and non-sealed (both generate Empty). Does it make sense? At least I hope it does :)

Maybe I should emphasize the motivation to dispel possible misunderstanding:
The motivation is to make data modelling with ScalaPB easier. The rationale is similar to why null is bad. If I have an instance of NonsealedValue, I want to be _really_ sure that I indeed have an actual NonsealedValue, not Empty (or null). But if Empty gets auto-generated for every type, I have to _always_ check for it.

That's why I don't want to use Empty, and either have the decoder throw in case it's empty, or have NonsealedValue in an Option where I can _just once_ that it's Some and then just go with it.

Is that an understandable use-case? I suspect I'm not the only one who would like to use it that way.

[thesamet]

@sideeffffect thanks for explaining the motivation.

One difference between sealed and non-sealed: If we are going to wrap sealed oneofs in an Option to account for the empty case, then if we get a repeated field, we are going to end up with Seq[Option[SealedOneof]] - not sure if that's desirable. Non-sealed oneofs can't be repeated so this problem doesn't occur for them.

I wonder what do you think about the following option which I believe doesn't break compatibility. Example given for sealed oneofs (but I believe can similarly work for normal oneofs as well):

message MyADT {
  oneof sealed_value {
    TypeA a = 1;
    TypeB b = 1;
  }
}

generates:

object MyADT {
  sealed trait NonEmpty extends MyADT
}

sealed trait MyADT {
  def asOption: Option[MyADT.NonEmpty] = this match {
    case n: MyADT.NonEmpty => Some(n)
    case _ => None
  }
}
case class TypeA(..) extends MyADT with MyADT.NonEmpty
case class TypeB(..) extends MyADT with MyADT.NonEmpty
case object Empty extends MyADT

The difference from the current generated code is that we add a sealed trait MyADT.NonEmpty (which is the type you wanted to have inside the Option[_] in the original proposal). We also add a method asOption to the base trait to convert an instance to an Option[MyADT.NonEmpty]. This would accomplish the motivation you stated: you call asOption and get exactly the type you wanted.

The advantage is that it's an incremental improvement rather than introducing a new flavor of generated code. The downside is that we have two sealed traits around.

[sideeffffect]

Hmm, that's a very interesting proposal! I've remixed it a bit. What do you think? Would this also be viable?

object MyADT {
  sealed trait Nullable {
    def asOption: Option[MyADT] = this match {
      case n: MyADT => Some(n)
      case _ => None
    }
  }
  object Nullable {
    case object Empty extends MyADT.Nullable
  }
}

sealed trait MyADT extends MyADT.Nullable
case class TypeA(..) extends MyADT
case class TypeB(..) extends MyADT

[sideeffffect]

In this proposal MyADT.Nullable.Empty is an instance of MyADT

In my proposal? I hope not :) MyADT.Nullable.Empty is supposed to be a subtype of MyADT.Nullable only (so not MyADT). Sorry if I've made a typo or misunderstood something

[thesamet]

Your proposal makes sense but it breaks source compatibility since Empty is no longer a case of MyADT, so it needs to be under a flag for the code generator. I am trying to minimize options that end up generating very different APIs due to the increase in complexity it causes.

[sideeffffect]

oh, I see your point
yes, your approach would definitely be a step in the right direction

[thesamet]

This commits add a NonEmpty trait as described above and a method named asNonEmpty to convert the value to an Option[NonEmpty]. For the time being this is experimental. Feedback is welcome.

[sideeffffect]

thank you @thesamet ! :pray:

[ahjohannessen]

@thesamet I think the proposal by @sideeffffect makes a bit more sense than the NonEmpty as it treats the empty case as exceptional. I have never liked dealing with Empty and Unrecognized and what @sideeffffect proposed solves that and I do not mind if it is a breaking change as long as it makes the data modelling simpler. Consumption is also easier because Empty is not a part of MyADT and you are able to guard once at the edge for empty.

[thesamet]

@ahjohannessen : @sideeffffect proposed three solutions: (1) wrapping in Option, or (2) throwing an exception, (3) Introducing MyADT.Nullable in this comment. Which one is your preference? Based on this comment, wrapping in Option can be perceived as less desirable.

[ahjohannessen]

I would prefer (3) since it separates empty from MyADT. I know you are concerned by breaking changes, but as a consumer I do not mind. I think a flag would be great, it gives existing users some time to migrate.

[thesamet]

@ahjohannessen Got it. I like that proposal too. I am fine with a breaking change around Empty/NonEmpty (with a flag) for the next minor version.

[ahjohannessen]

@thesamet Awesome :) Can the same thing be done for enum?

[thesamet]

@ahjohannessen Yes. I'd like enums and sealed oneofs to have a similar API as much as possible. Although I still want to collect more feedback for this proposal.

In the proposed change, fields of type MyADT will generate aval of type MyADT.Nullable (instead of MyADT), so that would be some sort of noise at the code layer that interact with the generated code.

The term Nullable is overloaded from Java and can be misleading. I am looking for alternatives:
(a) Name it OrEmpty, we'd have MyADT.OrEmpty.
(b) Name it MyADTOrEmpty and generated it at the same scope of MyADT (not inside it). The Empty case object is generated also at that same scope.

There will be a short-lived file-level (or package-level) option for the migration that would generate the existing form of the code.

/cc @olafurpg for feedback

[ahjohannessen]

@thesamet (a) appeals most to me, but I have no strong opinion about it.

[thesamet]

It may be just me, but the part that I find counter-intuitive about the proposed code layout is that MyADT.OrEmpty is scoped inside MyADT, but is actually a super-type of MyADT. The first time I looked at it, I thought it implies Empty <: MyADT.Nullable <: MyADT, which is of course incorrect.

The code implies Empty <: MyADT.Nullable and MyADT <: MyADT.Nullable (but it doesn't match the nesting structure)

[ahjohannessen]

@thesamet That makes sense, perhaps (b) is more intuitive

[thesamet]

I added a new style of sealed oneof that can be invoked by naming it sealed_value_or_empty:

message Expr {
    oneof sealed_value_or_empty {
        Lit lit = 1;
        Add add = 2;
    }
}

This would result in a structure like this:

sealed trait ExprOrEmpty
sealed trait Expr extends ExprOrEmpty

object ExprOrEmpty {
  object Empty extends ExprOrEmpty
}

final case class Lit(...) extends Expr with ExprOrEmpty with ...
final case class Add(..) extends Expr with ExprOrEmpty with ...

This will be in 0.10.0-M1 which I'll release shortly.

[thesamet]

Released 0.10.0-M1, would appreciate feedback about this new sealed value style. I intend to keep both styles for the time being.

[ahjohannessen]

@thesamet That looks great :) I suppose final case class Lit(...) extends Expr with ExprOrEmpty with ... is the same as final case class Lit(...) extends Expr with ... because Expr inherits from ExprOrEmpty ?

[olafurpg]

@thesamet This is an interesting approach. I like using the sealed_value_or_empty name as a way to customize the code generation. I wonder if this new scheme goes far enough to satisfy the original request.

If I understand correctly, field references to sealed values still have the *OrEmpty type.

message Expr {
    oneof sealed_value_or_empty {
        Lit lit = 1;
        Add add = 2;
    }
}
message Add {
  Expr a = 1;
  Expr b = 1;
}

// current behavior
case class Add(a: ExprOrEmpty, b: ExprOrEmpty)
// desired behavior
case class Add(a: Expr, b: Expr)

[thesamet]

@ahjohannessen Yes, should be harmless. I think this came from some code sharing between the styles, in both of them the extend the empty and non-empty traits.

@olafurpg Thanks for checking it out! My understanding that this should satisfy the request. In the original post, option (1) was calling to use Option[Expr] rather than Expr, and subsequently, in this comment there's a suggestion to generate Expr.Nullable - presumably for field references.

If we'd like field reference to be of type Expr (in the style without an empty case), we need to define (1) Add.defaultInstance that doesn't throw, (2) handle inbound messages that set not value for Expr.

[thesamet]

Closing the issue as we have introduced here a new style of sealed value (sealed_value_or_empty) that excludes the empty case as requested.

[sideeffffect]

Hello @thesamet, @ahjohannessen and @olafurpg
First thanks @thesamet for implementing sealed_value_or_empty!
I'm sorry I'm again late to the party, but I have some observations and questions.

I think it's still preferable to express nullability explicitly via standard Scala Option[_], instead of an ad hoc Empty case object hiden in a sealed trait family.
The reason is that Option[_] is more malleable to automated transformation libraries for converting/validating ScalaPB generated classes to handwritten domain classes (like Chimney, henkan, custom marcro-based solution, ...).
Unfortunately, sealed_value_or_empty doesn't move us all the way in that direction.

So the question is, would something like this be possible? The trick here would be controlling this proposed style of code generation with the _option suffix.

Normal oneof would behave like this:

message ContainingMyOneof {
  oneof value_option {
    HelloRequest request = 1;
    HelloReply reply = 2;
  }
}

message HelloMsg {
  ContainingMyOneof hello = 1;
}

final case class ContainingMyOneof(value: Option[….ContainingMyOneof.Value])
…
object ContainingMyOneof {
  sealed trait Value …
  final case class Request(value: ….HelloRequest) extends ….ContainingMyOneof.Value
  final case class Reply(value: ….HelloReply) extends ….ContainingMyOneof.Value
}
…
final case class HelloMsg(hello: Option[ContainingMyOneof])

The special sealed_value would behave like this:

message ContainingMyOneof {
  oneof sealed_value_option {
    HelloRequest request = 1;
    HelloReply reply = 2;
  }
}

message HelloMsg {
  ContainingMyOneof hello = 1;
}

sealed trait ContainingMyOneof …
final case class Request(value: ….HelloRequest) extends ….ContainingMyOneof
final case class Reply(value: ….HelloReply) extends ….ContainingMyOneof
…
final case class HelloMsg(hello: Option[ContainingMyOneof])

What do you guys think?

[thesamet]

I'll look into sealed_value_option soon. To keep the code small and simple, I am going to remove sealed_value_or_empty - sounds like it's not needed.

[sideeffffect]

having sealed_value_option would render sealed_value_or_empty unnecessary, that's true. But it's better than nothing, if you see what I mean :wink:

[thesamet]

sealed_value_optional is available on master. Feedback is welcome. One thing to note about it s that if you have:

message Expr {
  oneof sealed_value_option {
    Lit lit= 1;
    Add add = 2;
  }
}

then:

Expr expr=1;               // becomes Option[Expr]
repeated Expr exprs = 2;   // becomes Seq[Option[Expr]]`

[sideeffffect]

Expr expr=1; // becomes Option[Expr]

wonderful!

repeated Expr exprs = 2; // becomes Seq[Option[Expr]]

do you think it would be desirable to have it as a flat Seq[Expr] instead? :thinking:

[thesamet]

@sideeffffect This is the most precise representation given that each Expr can be empty. I can see why it may seem unexpected at first sight though. I don't think ScalaPB should flatten it and by doing so lose information. Than can happen when a sender adds a case, but the receiver stills run with an old version of the proto that doesn't have that case. Probably better leave it for the user to handle. Any suggestions?

[sideeffffect]

@thesamet that's a very good point! :ok_hand:

One outstanding question that I have is what can we do about non-sealed oneofs? (https://github.com/scalapb/ScalaPB/issues/633#issuecomment-552935658) Something like this, for example:

message ContainingMyOneof {
  oneof value_option {
    HelloRequest request = 1;
    HelloReply reply = 2;
  }
}

message HelloMsg {
  ContainingMyOneof hello = 1;
}

final case class ContainingMyOneof(value: Option[….ContainingMyOneof.Value])
…
object ContainingMyOneof {
  sealed trait Value …
  final case class Request(value: ….HelloRequest) extends ….ContainingMyOneof.Value
  final case class Reply(value: ….HelloReply) extends ….ContainingMyOneof.Value
}
…
final case class HelloMsg(hello: Option[ContainingMyOneof])

[thesamet]

A single message can contain a number of oneofs. Are you suggesting to change the behavior for when there is an _option suffix regardless of the name? I am concerned that this might have unintended consequences for existing protos.

[sideeffffect]

I was concerned with non-sealed_values, because I thought I couldn't use them -- but I figured out a way.
Thank you @thesamet so much! :pray: I think we can close this issue now :tada:
I will test sealed_value_option once it gets released

[thesamet]

0.10.0-M2 was released today.