Add parameter location information to parsetree/typedtree

[ncik-roberts]

As a follow-up to #12236, this PR adds parameter locations to the parsetree/typedtree for merlin's benefit. I discussed this change with @voodoos, who suggested this PR would be a good idea — maybe he would be a good reviewer as a merlin maintainer?

As an example:

Prior to #12236:

let f x ~(y : int) = ()
        ^^^^^^^^^^^^^^^ exp_loc2
      ^^^^^^^^^^^^^^^^^ exp_loc1

where exp_loc1 is for the outer Texp_function and exp_loc2 is for the inner Texp_function.

After #12236:

let f x ~(y : int) = ()
          ^ pat_loc2
      ^ pat_loc1
      ^^^^^^^^^^^^^^^^^ exp_loc

There's now just a single Texp_function. Notably, there's no location that identifies the tilde or the opening paren as being associated with the y param — previously this could have been obtained from the start position of the exp_loc2 location.

After this PR:

let f x ~(y : int) = ()
        ^^^^^^^^^^ param_loc2
      ^ param_loc1
      ^^^^^^^^^^^^^^^^^ exp_loc

(pat_loc1 and pat_loc2 are the same as the previous example.) Now, merlin can see if the user's cursor is on any part of the parameter, including the tilde or the surrounding parens.

I also take this opportunity to reverse a change in the column number indicated by a Match_failure exception that was introduced in #12236 and that we discussed in the comments. It seems better to avoid changing the exception's arguments if we can, and this PR makes it easy to do this.

[gasche]

Overall I like this PR, we want to keep Merlin happy and adding more locations is rarely a bad move. I decided to add some decision fatigue with a bike-sheddy question on the parsetree definition, to resolve before merging. Otherwise I read the code and it is fine.

[gasche]

I am not sure whether one should rather use this or something like

and function_param = function_param_desc loc

which is isomorphic.

Today in the parsetree codebase:

• We only use the _desc / _loc patterns for records that have other metadata fields than just locations (typically attributes, sometimes more). This suggests using ` loc .
• We only use foo loc for "external" foos that are themselves defined in Parsetree, typically string or label or string option. This suggests using a _desc / _loc record.

Any opinion?

[ncik-roberts]

My opinion is that we should use a dedicated record for the second reason you gave. As another point in that direction, the two fields of loc are loc and txt, and txt does not seem like a very good name for a field that contains the parsed function parameter.

[voodoos]

Thanks Nick. I didn't have a close look at the code but the proposed changes should effectively allow Merlin to give more relevant information when queried on the enclosing parenthesis or another token that is not part of the parameter name itself.

[gasche]

My bike-shedding question has been answered, I think this is good to go.

[gasche]

(This needs a rebase apparently?)

[ncik-roberts]

All rebased.

[ncik-roberts]

I rebased to resolve a small merge conflict with #12428.

[gasche]

(cc @panglesd)

[gasche]

Thanks @ncik-roberts, and apologies for missing the merge window last time.

[panglesd]

Thanks @gasche for the ping!

I opened ocaml-ppx/ppxlib#451 two days ago when discovering #12044 and #12236, but I keep missing parsetree changes PRs until after they are merged...

On #12236, when writing the migration functions, I indeed noticed that some locations were missing. I will update my PR with this one to check if the migrations can now be written successfully.

If the new parsetree does not include at least all the information for a downgrade migration, using ppxlib with an old version of OCaml will mean working with a parsetree where some informations have been lost...
In order to test that the new parsetree has at least as much information as before, I think it would be really nice to ping us ppxlib maintainer before merge: so that we can write the migration functions before the PR is merged, and suggest modification to the parsetree if needed.

By the way, I liked the parsetree-change label, I think it helps PPX-related people! It's missing here, and in the previous two PRs

[Octachron]

The lack of label was a lapse on my side, I have fixed it, but this sounds like a task well-suited for a bot.

[gasche]

@panglesd would you like to contribute some form of automation that enforces a label on PRs that modify parsetree.mli? One way I personally know how to do it is to reuse the logic in the "typo" CI check that looks for Changes modifications, and can be disabled by using the no-changes-related label. (The UI if we do this would be: if people forget to add the label, they get a CI failure. It is more manual than an entirely automated labelling, but better than what we have today.)

[panglesd]

would you like to contribute some form of automation that enforces a label on PRs that modify parsetree.mli?

Sure! Thanks for the idea, I think with the typo example I should be able to do it. (Also, that's on github's size but I would love to be able to "watch" a specific label, but that seems to not be possible...)