improve option shape detection in Datarepr

[anmonteiro]

while reviewing #22, I noticed that the current code in Datarepr.constructor_descrs would assign a type such as the following an optional shape, when that's clearly wrong:

type 'a t = None of 'a | Some

cc @andreypopp

[andreypopp]

Are you referring to the fact that the order is switched or the names are different (well it is both in this case).

To be honest I thought that any data type isomorphic to 'an option will have the same repr and this is by design. But not sure if same data type with constructor order switched is the same for melange runtime.

[anmonteiro]

| (
    [ (a_id, ({cstr_args = []} as a_descr) )  ;
      (b_id, ({ cstr_args = [_]} as b_descr))
    ] |
    [ (a_id, ({cstr_args = [_]} as a_descr) )  ;
      (b_id, ({ cstr_args = []} as b_descr))
    ]
   ) when (Ident.name a_id = "Some" && Ident.name b_id = "None") ||
         (Ident.name a_id = "None" && Ident.name b_id = "Some")

This pattern matching seems wrong because it accepts a_id with 0 arguments as Some, or b_id with one argument as None.

[anmonteiro]

I applied this fix locally and saw no generated code changes. This tells me:

• should be safe to apply the change
• we don't have a test that covers this bug

[andreypopp]

This pattern matching seems wrong because it accepts a_id with 0 arguments as Some, or b_id with one argument as None.

Doesn't this define a data type isomorphic to 'a option though?

type 'a opt_rev = None of 'a | Some

So the question holds:

To be honest I thought that any data type isomorphic to 'an option will have the same repr and this is by design. But not sure if same data type with constructor order switched is the same for melange runtime.

Why is this a bug? Is anything matches on constructor names in runtime (or their order)?

[anmonteiro]

Here's a playground link that shows the bug I'm speaking about.

Doesn't this define a data type isomorphic to 'a option though?

type 'a opt_rev = None of 'a | Some

No, we handle None as the constructor carrying no data.

[anmonteiro]

Hmm, actually my snippet just manages to reproduce the issue you pointed out.

[andreypopp]

No, we handle None as the constructor carrying no data.

Can you point me to the place in code which does this? My understanding is that this happens after translcore which translates None-like constructors (within types isomorphic to 'a option) to Pt_shape_none.

[andreypopp]

The thing is, even if you strengthen checks for a type to be exactly None | Some of 'a (same names, same order) then it will still be possible to define the same shaped but completely unrelated (from the point of view of typing) type in another module.

So maybe it makes sense to treat any type isomorphic to 'a option the same, that way it seems to be more consistent to me.

[anmonteiro]

Can you point me to the place in code which does this? My understanding is that this happens after translcore which translates None-like constructors (within types isomorphic to 'a option) to Pt_shape_none.

I thought the relevant section in translcore.ml from your patch in #22 did this.

So maybe it makes sense to treat any type isomorphic to 'a option the same, that way it seems to be more consistent to me.

which would probably render this PR useless?

[andreypopp]

I thought the relevant section in translcore.ml from your patch in #22 did this.

Well, yeah, but the check before was half baked anyway, e.g. checking only for None, the other constructor name wasn't checked IIUC.

which would probably render this PR useless?

Probably.

Anyway, need to create proper test suite for such behaviour.