parser change to "let" desugaring not quite right yet

[ceastlund]

The form let x : t = e desugars to let (x : t) = (e : t). Prior to 4.14, both type constraints (on the pattern and on the expression) had loc_ghost=true. In 4.14, now (x : t) has loc_ghost=true and (e : t) has loc_ghost=false.

I think this is a step forward, in that yes, one occurrence of the type constraint should be annotated as "real" syntax, for purposes of merlin and so forth. But it seems like (x : t) should have loc_ghost=false instead, and (e : t) should have loc_ghost=true. The type constraint originally appears next to the pattern, so that seems clearly the "real" occurrence, and the locations will line up better that way.

cc @lpw25

[ceastlund]

In fact, I think not only should (e : t) have loc_ghost=true, I think it would be best if that second occurrence of t had loc_ghost=true on all of its locations.

[ccasin]

FWIW, it looks like the 4.14 change was introduced here:

ocaml/ocaml#10555

The discussion seems mostly about type annotations in record expressions (the helper function mkexp_constraint, where the "ghost" flag was turned off, is shared with the let binding code). Even in the record case, I'm a bit surprised by their choices in terms of what to make ghost and what not, but I suppose I am missing some context.

[ccasin]

OK, staring a little at the discussion, I think I understand their motivation. The key example seems to be { lbl : ty } a punned record expression with a type annotation, which gets parsed as something like { lbl = lbl : ty }. I guess I can see why here it makes sense for the RHS to be non-ghost and the LHS to be ghost. They seemed to want patterns and expressions to be parsed consistently, leading to the behavior you observed. Maybe that's a mistake.

[ceastlund]

Why is that different from anything else? The pattern is a punned record, too.

[ccasin]

Sorry - I don't understand your question.

To be clear, I think the change you want sounds reasonable. My comments above were just trying to work out what the change to the parser in 4.14 was, considering it seems like obviously the wrong choice for which part of an annotated let binding should be marked "ghost", as you observe.

[ceastlund]

Ah, got it. I had misunderstood what you were saying, then.

[github-actions]

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

[ncik-roberts]

I'm closing this as let x : t = e no longer desugars to anything in the 5.1 parsetree — it has its own dedicated encoding, via Pvc_constraint. So, there's no longer any node that should be marked ghost. (Though ppxlib's conversion functions between 5.1 and 4.14 might reintroduce a ghost node. But that's solved by migrating ppxlib to 5.1.)