Extra types in path #11568
Extra types in path #11568
Conversation
| @@ -154,7 +154,7 @@ let extension_descr ~current_unit path_ext ext = | |||
| in | |||
| let existentials, cstr_args, cstr_inlined = | |||
| constructor_args ~current_unit ext.ext_private ext.ext_args ext.ext_ret_type | |||
| path_ext (Record_extension path_ext) | |||
| Path.(Pextra_ty (path_ext, Pext_ty)) (Record_extension path_ext) | |||
There was a problem hiding this comment.
We discussed this line during our type-system meeting: it is weird that before we would have path_ext on both sides and now we pass two different passes. @HyunggyuJang and @garrigue say that they have looked carefully and they believe that this is the right choice.
| @@ -460,6 +460,11 @@ let of_path ~find_shape ~namespace = | |||
| | Pident id -> find_shape ns id | |||
| | Pdot (path, name) -> proj (aux Module path) (name, ns) | |||
| | Papply (p1, p2) -> app (aux Module p1) ~arg:(aux Module p2) | |||
| | Pextra_ty (path, extra) -> begin | |||
| match extra with | |||
| Pcstr_ty _ -> aux Type path | |||
There was a problem hiding this comment.
This case (should ideally start with a | and) does not fully make sense, but I think that the issue is with the Shape code, which is not robust enough here to be extended with new paths, and not with the PR itself. I don't think this problem needs to be fixed in this PR.
More details in case Shape people are reading this (again, irrelevant for this PR I think): what we want to do here is to return the cda_shape field of the constructor_data record in the typing environment. But this is not possible with the prototype of this function, which only lets us query the environment on identifiers, not full paths. This is frustrating because Env.find_shape (which is not exposed in env.mli) does exactly the right thing in the Extension_constructor case.
I think that we should change the prototype of the Make_reduce functor to take a Sig_component_kind.t -> Path.t -> Shape.t function, and get rid of of_path. But this is a change that affects Merlin as well.
Another option would be to guarantee that we can recover precise shape information from aux Type path here, enough to compute the path of the constructor itself. (Our input is a path of the form M.t.Foo, meaning the inline-record type of the constructor Foo in the type M.t.) For this, the shape of M.t itself should not be a Leaf, but a product structure containing the shape of each of its constructors. This makes sense semantically, we may do something like this eventually, and it would be necessary if we wanted to decouple the shape-computation from the typer (currently we store all shapes in the environment, but in theory we could do without), but it is a slightly more complex change.
HyunggyuJang
force-pushed
the
extra-types-in-path-tmp
branch
from
bc13de9
to
aa5276d
Compare
Reflect reviews Reflect review Pacify hygine
HyunggyuJang
force-pushed
the
extra-types-in-path-tmp
branch
from
aa5276d
to
c2f0179
Compare
|
@gasche Ready to be merged. |
This PR replaces #11315.
Encodes inline record types in Path.t
They replace the decoding through Path.typath used before
The goal of the changes is to remove encodings through names that were used before:
Hash types will be removed by a separate PR.
The only remaining such encoding is for abstract row types.
Written with @garrigue and @t6s