Backport PR #1800

[liam923]

Backport PR #1800 from upstream , which fixes a bug in destructing functions with optional arguments.

[goldfirere]

I can confirm that this PR is a correct backport of the patch from upstream merlin. So that's good.

But I think there is a better way to do this: just fix Untypeast. The fix as written is fragile, in that it will do the wrong thing in the presence of ghost locations from ppxes.

I would think a better solution would be to record, in typedtree, whether an optional argument was actually passed or not. Maybe by editing arg_label, or maybe by using Omitted somehow.

That said, it's probably best just to merge this patch for now. I predict, though, that we'll get reports of new odd behavior soon.

[ncik-roberts]

Do you know if there's a reason why the code doesn't also look for whether the argument is None? That would help keep this heuristic from being broken by a ppx, I think.

[liam923]

@goldfirere For what it's worth, Merlin destruct already does the wrong thing in the presence of ppxes. For example, destructing let%map x = y in z will produce match Let_syntax.map y ~f:(fun x -> x) with .... This preserves semantics, unlike the potential bad interaction you're talking about, but I would argue it is just as bad because users will still go back and modify the code produced by Merlin.

I do agree though that modifying the typedtree would be a nicer solution. An even nicer solution in my opinion would be recovering the original text typed by the user and using it instead, which would circumvent any ppx issues.

I think @ncik-roberts solution of checking whether the argument is None is a reasonable improvement on this backport. I'll do that.

[liam923]

I'm going to hold off merging this until Nick's suggestion has filtered into upstream and then replace my changes in this PR with that. That way, we keep in sync with upstream.

[ncik-roberts]

ocaml/merlin#1807 appears to have been merged into ocaml/merlin.

[liam923]

ocaml/merlin#1807 appears to have been merged into ocaml/merlin.

I've brought in that change now.

[goldfirere]

Looks good to merge