#1293 Facet inheritance

msl Fri 5 Nov 2010

Is there any way to specify that an implementation of a mixin must be @Serializable.

I was hoping something like the following would work:

$ cat Sample.fan

@Serializable
mixin SampleMixin {}

class SampleClass : SampleMixin {}

class Main {
  Void main() {
    Env.cur.out.writeObj(SampleClass())
  }
}

But unfortunately it results in a sys::IOErr:

$ fan Sample.fan

sys::IOErr: Not serializable: Sample_0::SampleClass
fanx.serial.ObjEncoder.writeObj (ObjEncoder.java:84)
fan.sys.OutStream.writeObj (OutStream.java:269)
fan.sys.OutStream.writeObj (OutStream.java:266)
Sample_0::Main.main (/tmp/Sample.fan:8)
java.lang.reflect.Method.invoke (Method.java:597)
fan.sys.Method.invoke (Method.java:536)
fan.sys.Method$MethodFunc.callOn (Method.java:214)
fan.sys.Method.callOn (Method.java:148)
fanx.tools.Fan.callMain (Fan.java:137)
fanx.tools.Fan.executeFile (Fan.java:88)
fanx.tools.Fan.execute (Fan.java:34)
fanx.tools.Fan.run (Fan.java:236)
fanx.tools.Fan.main (Fan.java:274)

Obviously putting @Serializable on the class works as expected - but it's something that's desirable (atleast in my use case) to have as part of the mixin.

M

brian Fri 5 Nov 2010

Renamed from @Serializable on Mixins to Facet inheritance

brian Fri 5 Nov 2010

Well the problem here doesn't really have anything to do with serialization.

Rather it has to do with the fact that facets are not inherited today. They used to be in the old model, but I guess when I reworked them that got overlooked. I'm surprised this is the first time this issue has popped up.

So big question for today, should facets be inherited?

  1. They are not inherited (same design as today)
  2. They are not inherited automatically by Type.facets and Type.facet, but we augument those methods or add new methods as convenience to take inheritance into account (this was old facet model)
  3. We add a facet to the facet to say whether they should be inherited (I believe this is the Java approach)

So what does everyone say?

DanielFath Fri 5 Nov 2010

Well I guess it depends. I'd like it to be as simple and powerful as possible so I'm leaning towards 2 or 3. Could you elaborate 2 a bit more? If 2 was the old facet model why was it changed to current form?

dmoebius Fri 5 Nov 2010

+1 for 3

tonsky Fri 5 Nov 2010

You now have a choice to inherit it (manually) or not. If they will be inherited by default, you won't have a technique to turn it off. I believe that choice must be in hands of client, so my vote for 1.

As for 3, it'll be better to have a special facet (or keyword) to explicitly state that I want to inherit all parent's facet. It must not be specified on the facet itself.

msl Fri 5 Nov 2010

I believe that choice must be in hands of client, so my vote for 1.

Option 2 gives the same control. I think Brian is suggesting something like sys::Type replacing:

Facet? facet(Type type, Bool checked := true)
Facet[] facets()

With:

Facet? facet(Type type, Bool inherited := false, Bool checked := true)
Facet[] facets(Bool inherited := false)

Personally, I'm not too fussed what the default of inherited is, so much as the current API overrides it appropriately (eg the original point: serialization)

I do also like the idea of the facet being able to define how it should be used, so perhaps this isn't straight either/or question? Is there some elegant mix of 2 and 3?

helium Sat 6 Nov 2010

+1 for 3

brian Sun 7 Nov 2010

I did a little digging into Java annotations. It turns out the @Inherited annotation on a annotation only works on classes, not interfaces. Also I didn't see anything that lets you hide it once defined or any support for method/field overrides.

In the old model inheritance was a lot simpler because everything was just name/value pairs. This meant that inheritance decisions were client driven not key/facet type driven. It meant it was easy to remove a facet by just overriding it be false.

I am inclined to use model 3 with no support for turning a facet "off" in a subclass. In this way inherited facets would work like types. If a class defines the @Serializable facet, then it is creating a contract which must be honored by all subclasses. You can't turn off a mixin/super class above your hierarchy, so it makes sense to follow this convention with facets.

So my proposal in more detail is to add a new facet called sys::Inherited which when applied to a facet would cause that facet to be inherited. For now the Inherited facet will be ignored for facets on slots. For type facets:

brian Sun 7 Nov 2010

Promoted to ticket #1293 and assigned to brian

qualidafial Mon 8 Nov 2010

+1 to same inheritance rules for facets as for classes or mixins.

rosarinjroy Tue 9 Nov 2010

I am wondering what is the rationale behind making the inheritance optional (based on @Inherited facet). I feel that a facet is also a part of the contract. Could someone please share a couple of use cases when not inheriting the facet makes sense?

DanielFath Tue 9 Nov 2010

Not sure about Fantom but in Java world there is no point in inheriting @Table or @Entity facets (at least not with their current attribute values) from JPA.

brian Tue 9 Nov 2010

I am wondering what is the rationale behind making the inheritance optional

Sometimes facets are purely meta-data (vs contracts) in which case you wouldn't want them to be inherited. Dumb, but simple example would be Author facet that defined who wrote the class.

rosarinjroy Tue 9 Nov 2010

Got it. Thank you for your explanation.

brian Wed 10 Nov 2010

Actually after digging into this, I think the .NET design is a little cleaner. Instead of different facets for target, inherited, etc they just have one AttributeUsage attribute. This would be a place to stick future information about facets in one class instead of littering the sys pod with a bunch of littles facets.

So my revised design is:

facet class FacetMeta
{
  const Bool inherited := false
}

brian Wed 10 Nov 2010

Ticket resolved in 1.0.56

Added new FacetMeta class and enhanced Type.facets and facet to build its data structures using inheritance.

Login or Signup to reply.