Add support for 5.4 bivariant type parameters in type declaration

[NathanReb]

This exhibits how I initially envisioned the encoding of bivariant type parameters, or of any other such AST change that cannot directly be encoded into an extension point.

The idea is that the feature is encoded locally at the parameter tuple level (which is already one level up the variance node), and then wrapped into an extension at the closest level, e.g. here, at the structure_item_desc level.

The reason why we don't simply use the parameter encoding is because it can to easily result in ppx-authors misinterpreting it, e.g. they could operate on the variance, ignoring the core_type in the tuple and therefore completely misinterpreting the AST.
We instead ensure that no one inadvertently traverse an encoded node by wrapping the whole structure_item in an extension.

This may seem overkill but protects us against nasty and hard to trace bugs by preventing tempering with encoded data which can lead to migration failure or worse, silent and invalid rewriting of the AST during migration.

The addition of bivariant type parameters makes for one of the worst possible case for our new encoding approach as it spreads quite far up the tree, especially considering it would have been ignored by most ppx-es. We chose the safe approach here, the tradeoff being that ppx won't be able to support bivariant parameters without updating.

The positive here is that this is such a niche feature that most users will never encounter this feature, let alone have it involved with any kind of ppx rewriting, that the encoding should not cause too much trouble in the wild.

Once again, this is still a net positive over the previous situation since old code will always preprocess and compile without troubles.

[NathanReb]

I'm opening a draft PR that only handles bivariant parameters in type declaration, in structures. We still need to add support for them in:

• type_declaration in signature
• type_declaration in module_type_desc
• type_extension
• class_declaration
• class_description
• class_type_declaration

This amounts to writing boilerplate that plugs in the existing encoding/decoding of type parameters and add the right extension point wrapping but is still a significant bit of work for a niche feature.

[NathanReb]

Checking that attributes are [] should be moved to the decode function here so that it leads to an error rather than to let an uninterpreted extension slip through.

I'm also conflicted on whether we should go for this case based approach vs having the decode function simply be something like:

val decode_pstr : loc:Location.t -> structure_item_desc -> structure_item_desc option

Any thoughts @patricoferris ?

[patricoferris]

This looks good to me @NathanReb.

Whilst I'm not stoked about the use of exceptions for non-exceptional control-flow, I think it is worthwhile given how little the migration code tends to move for older compiler versions.

[patricoferris]

Is the change from 504 to 5_4 something we should change elsewhere too? Is the reason to avoid confusion like we saw with my accidental use of 502 in the error message for Ptyp_open?

[patricoferris]

This does make me nervous about accidentally leaking a Location.none in if we ever come back and refactor this code. Beyond "code movement" is there any reason to not force passing a ~loc in anywhere we use copy_module_type_desc (or the other equivalents)?

[patricoferris]

TODO

[patricoferris]

I had a look at trying to abstract this behind some kind of map_bivariant_list function, something like:

and map_bivariant_list ~exn fn lst =
  let contains_bivariant = ref false in
  let cds =
    List.map
      (fun v ->
        match v with cd -> fn cd | exception e -> exn contains_bivariant e)
      lst
  in
  (!contains_bivariant, cds)

( *...*)
      let contains_bivariant, cd =
        map_bivariant_list
          ~exn:(fun v -> function
            | Bivariant_param.Class_decl cd ->
                v := true;
                cd
            | e -> raise e)
          copy_class_declaration x0
      in

What do you think? It doesn't really reduce the code but it might help with understanding the intention behind the code later on down the line? I'm not too fussed, just thought I would mention it given I gave it a look :)

(I just get nervous with the use of exceptions and references!)

[patricoferris]

Suggested change

	(copy_module_type x0, List.map copy_with_constraint x1)
	(copy_module_type x0, constraints)

Seems in the non-bivariant code path, we are doing the work twice?