Fix expansion of type constructors#153
Conversation
azzsal
force-pushed
the
Fix-expansion-of-type-constructors
branch
from
b5007b9 to
f7ab738
Compare
NathanReb
left a comment
NathanReb
left a comment
There was a problem hiding this comment.
This is looking good! Indeed it will likely need a bit more tests, at least for the cases we expect to find commonly in the wild.
There are a few things that can be simplified but it seems to be working well, congrats!
| | Changed change -> Changed change) | ||
| | Changed change -> ( | ||
| match | ||
| type_expr ~typing_env ~expand:false ~ref_params ~cur_params |
There was a problem hiding this comment.
You re-introduced an extra parameter used for recursion here. I'm not sure we really need the expand parameter. As we were doing before we can run the expansion mechanism and match over the resulting expressions:
- if both are
Tconstr, we diff them withconstrdirectly, without going throughtype_expragain - if they aren't, we forward them to
type_expr
| args:Types.type_expr list -> | ||
| Types.type_expr option | ||
| (Types.type_expr * Types.type_declaration option) list | ||
| (** Recursively expand the given path and args, looking up the environment for |
There was a problem hiding this comment.
The doc comment is not correct anymore!
| | _ -> (expr, Some td) :: acc) | ||
| in | ||
| aux None path args | ||
| aux [ (type_expr, None) ] path args |
There was a problem hiding this comment.
This is a bit confusing, what does the return type of this function represent? What are the left and right hand side of the pairs?
| | None, None -> false | ||
| | Some ref_td, Some cur_td | ||
| when Option.is_none | ||
| (type_declarations ~typing_env ~name:"" ~reference:ref_td |
There was a problem hiding this comment.
We're diffing the whole type declaration here? Isn't that overkill considering we only want to compare params and manifest? If there is no manifest there is no point comparing the rest of the definition as we should treat them as equal based on their path, e.g. if a record M.N.t from both versions of the API changed, we still want to treat it as equal. I don't think the code would be correct in this case.
Here we should think of an example to test this.
Reference:
type ('a, 'b) t = { a : 'a; b : 'b }
type 'a t1 = ('a, string) t
type t2 = int t1
val x : t2Current:
type ('a, 'b) t = { a : 'a; b : 'b; flag : bool }
type 'a t1 = ('a, string) t
type t2 = float t1
val x : t2In this example, I think the diff for the type of x should be shown as [-int-]{+float+} t1.
| @@ -0,0 +1,97 @@ | |||
| open Api_watch | |||
There was a problem hiding this comment.
For unit tests we don't need one file per function tested. One unit test file per module (as in .ml file) is enough. Can you merge the two together?
| path:Path.t -> | ||
| args:Types.type_expr list -> | ||
| Types.type_expr option | ||
| (Types.type_expr * Types.type_declaration option) list |
There was a problem hiding this comment.
This needs to be clarified a bit, I'm not sure how to interpret the option and I don't think we need the whole type declaration either.
|
|
||
| let fully_expand_tconstr ~typing_env ~path ~args = | ||
| let rec aux last path args = | ||
| let expansion_lst ~typing_env ~type_expr ~path ~args = |
There was a problem hiding this comment.
It'd be good if we could avoid passing the type_expr here, I think it ends up messing with the return type more than anything an clutters a bit the function when we don't really need it.
We can see it as returning the expansion chain from the tconstr made of path and args. The caller could eventually use this and the type_expr it used to get the path and args to build another list, more suited to their own need. In other words, I think injecting the type_expr here is the caller code leaking into the implementation of this function.
2 participants
No description provided.