OCaml5 merge conflicts: typing/typecore.ml
#213
Conversation
|
I said I might merge this prior to Leo's review, but there a number of other open PRs we'll need before this is useful, so I'll hold off. |
| Arg (Eliminated_optional_arg { next_arg_loc })) -> | ||
| next_arg_loc | ||
| | (_, Omitted _) -> None) | ||
| |> Option.value ~default:funct.exp_loc |
There was a problem hiding this comment.
I think the upstream code here is just wrong. The comment is incorrect: the arguments are not in reverse order of appearance. I also think that next_arg_loc is not a sensible value here -- again this is also true of upstreams version. We should get rid of next_arg_loc now, and fix the rest later. Maybe leave a CR by the comment pointing out that it is wrong.
There was a problem hiding this comment.
I agree next_arg_loc is kind of nonsense and will get rid of it.
To check: I believe rev_args are in reverse order of appearance in the type of the function. I think the point you are making is that may be different than the reverse order of the argument terms in the program due to labeled/optional argument. Correct?
There was a problem hiding this comment.
That's right. I think that when they differ this upstream code is going to produce confusing error locations.
| | None -> Not_a_function(ty_fun, explanation) | ||
| end | ||
| ======= | ||
| let ty_arg, ty_res = | ||
| with_local_level_iter_if separate ~post:generalize_structure begin fun () -> |
There was a problem hiding this comment.
This would probably be slightly easier to read if it used with_local_level_if and had an ad-hoc post function.
| @@ -9408,10 +7127,9 @@ and type_construct env (expected_mode : expected_mode) loc lid sarg | |||
| (instance ty_expected)); | |||
| end | |||
| in | |||
| ((ty_args, ty_res, texp), ty_res::ty_args) | |||
There was a problem hiding this comment.
Maybe better to just use with_local_level_if and write our own post function. That List.map offends my sensibilities.
| Pat.constraint_ ~loc:{pat.ppat_loc with Location.loc_ghost=true} pat sty | ||
| | _ -> pat | ||
| in | ||
| let pat_mode, exp_mode = |
There was a problem hiding this comment.
I'm not sure it makes much sense to pull this code out into this function. It seems to mix two separate concerns:
- Converting constraints within value bindings into equivalent constraints on patterns.
- Working out what modes the pattern and expression should be typed at
There was a problem hiding this comment.
Yeah, I waffled on this originally. The advantage of this approach is that it saves iterating and allocating an additional list, but now the name is bad and it does two unrelated things. We do want to build this list eventually, for later use by type_pattern_list.
In the absence of upstream, I think I would change the name of vb_pat_constraint to find_pat_constraints_and_modes (or something) and have it call two helpers that do the two things. But the cost of the extra list is small, and it's nice to stay in sync with upstream, so I'll just do what you suggest (leave vb_pat_constraint alone and build an extra list).
| <<<<<<< HEAD | ||
| if !Clflags.principal then begin_def (); | ||
| let op_path, op_desc = type_binding_op_ident env sop in | ||
| let op_type = op_desc.val_type in |
There was a problem hiding this comment.
We've lost the removal of the call to instance on this line. The call is harmless but also not needed and potentially confusing since this is a difference between our type_ident and the one upstream. We should also probably remove it from the other call site of type_binding_op_ident.
The beginning parts of this PR are not so bad, but towards the end I think it will be hard to review by reading the diff. When looking over the completed version myself, I found the easiest thing was to first create a file containing the (pat)diffs between (this file and the old ocaml-jst version) and another with the diff between (this file and the upstream version). Then, when reading a resolved diff, I would mainly look at those other files to understand how the diff was resolved and check nothing was dropped. (Though I confess past a certain point I ran out of steam for carefully reading what I'd written - will continue looking tomorrow)
Some major sources of conflicts:
5.1 has some of Nick's cleanup work in typecore (Remove arity-interrupting elaboration of module unpacks ocaml/ocaml#12117) but not all of it (Remove global state for typechecking patterns ocaml/ocaml#12229). This is mainly in the checking of patterns, and results in conflicts when PRs in both categories change the same line, and what we have is already ahead of upstream 5.1.
type_pathas been split into two functions upstream:type_patandcheck_counter_example_pat. These are meant to be kept in sync, but we didn't have the latter until now. I've tried to make surecheck_counter_example_pathas anything relevant we've added totype_pat, and we discussed the modes question elsewhere.Now that
type_patdoesn't take a~modeargument to distingish it fromcheck_counter_example_pat, it's very tempting to rename its~alloc_modeargument to~mode. But it seemed like that would make for confusing diff here, so it should be done separately.We've completely rewritten type_argument internally, and there are upstream changes in it. I proceeded by taking ours and then manually applying changes that had happened upstream. It looks like there are three main upstream PRs that have touched this code:
untyped_apply_arg, I just needed to add an (optional) location in theEliminated_optional_argcase. If I got it wrong, the worst case is a marginally worse error, which we can easily detect experimentally later.type_let has been completely refactored upstream, and we've made a bunch of changes to it for layouts and modes. Nothing too surprising here, just big diffs with confusing markers.