Fix longident locs

[patricoferris]

Previously, in our migration from the 503 AST to the 504 AST, we were adding Location.none locations to all of the segments of any long identifiers. This PR starts to try to fix that by reconstructing the locations of Ldot segments.

Partially fixes: #618

[gasche]

I wonder what is the current status of this fix. @giltho hit the same issue as #618 independently.

[patricoferris]

Thanks for the ping @gasche, I'll take a look this week, but there are two main issues why this PR is not undrafted.

1. We are not doing anything smart for reconstructing the locations of Lapplys -- unfortunately I don't think there is much we can do about that given we have no way of knowing in the original source code how much white space there could be (e.g. type t = int Map.Make (String).t is valid).
2. In the case where we migrate downwards we could in theory stash the true locations and reapply them in the migration back up, but I'm not sure that is really worth the hassle.

I'll need to take a closer look, for example, I just discovered that the following is broken:

module F (A : sig type t end) = struct
  module G (B : sig type t end) = struct
    type t = A.t * B.t
  end
end

type t = { v : F(Option).G(Int).t }

Which produces:

File "example/main.ml", line 7, characters 15-29:
7 | type t = { v : F(Option).G(Int).t }
                   ^^^^^^^^^^^^^^
Error: Modules do not match: (module Stdlib__Option) is not included in
       sig type t end
     ...
     Type declarations do not match:
       type 'a t = 'a option = None | Some of 'a
     is not included in
       type t
     They have different arities.

[patricoferris]

Having said all that, I think I am realising that we actually should only go the "serialisation route" for now. Our internal AST is 5.2 so the only time we go through the problematic 5.3 -> 5.4 migration is iff the user is running a compiler 5.4+ which would have the full locations already there. Reconstructing the locations would only be necessary if we bump the internal AST, something we do not plan to do any time soon. Let me prototype that shortly/this week for a full fix!

[giltho]

Naive question because I don't think I understand how AST migration is done internally but if ppxlib doesn't bump its internal AST anytime soon, how will modular explicits be supported in 5.5?

[patricoferris]

Naive question because I don't think I understand how AST migration is done internally but if ppxlib doesn't bump its internal AST anytime soon, how will modular explicits be supported in 5.5?

Not naive at all @giltho. It depends exactly what you mean by "supported".

If you mean, will ppxes work on OCaml code containing modular explicits, then the answer is yes (though not yet, as the parsetree changes have not be merged upstream). We will encode modular explicits into the AST which will preserve them as code is migrated downwards (towards our internal 5.2 AST) and eventually back up to 5.5.

If you mean, can my ppx use modular explicits in its logic, then answer is a little more tricky. The answer will most likely be yes, but it will not be as straight-forward as most are used to. @NathanReb outlined our new approach to supporting newer compilers and indeed there is a PR for handling labelled tuples like this (#607).

Which "supports" were you thinking of here?

[giltho]

I meant the first one! (Though this is mostly out of curiosity, thank you for taking the time 😊)

We will encode modular explicits into the AST which will preserve them as code is migrated downwards

Is the part of the AST that contains the modular implicit kept opaque? That means I wouldn't be able to use ppxes in "new" AST nodes I guess. Though, given it reuses first-class module syntax maybe that's not an issue

[NathanReb]

Is the part of the AST that contains the modular implicit kept opaque? That means I wouldn't be able to use ppxes in "new" AST nodes I guess. Though, given it reuses first-class module syntax maybe that's not an issue.

It would be kept opaque yes. ppx authors will have tools to expand such nodes if they are passed in their payload for instance.

If you're asking about whether ppx-es used in children of encoded nodes would be expanded then the answer at the moment is no but it does not necessarily have to be that way, we could have our AST rewriting pass convert and traverse those nodes using the same API I mentioned just above.

[giltho]

Makes sense, thank you for the explanation :)