ocaml icon indicating copy to clipboard operation
ocaml copied to clipboard

Regression in printing of shadowed predef types

Open ncik-roberts opened this issue 2 years ago • 6 comments

(It's likely this is more widespread than predef types, but I can only easily come up with examples that use predef types.)

#11515 makes the type printer more predictable, but I find at least one surprising change in the type printer. For an example, run ocamlc -i on a .ml file containing this program:

type nonrec unit = unit

let x = ()

Prior to that PR, the output was:

type nonrec unit = unit
val x : unit

Following that PR, the output is:

type nonrec unit = unit
val x : unit/2

That is, unit/2 is printed for the predef unit type, even though unit is a less confusing way to name it.

This is problematic for the Base opam package, as it exports aliases type unit = Unit.t from base.ml so that it's able to add ppx derivers to those declarations. (Unit.t is defined as unit.) The worse printing is currently exhibited in e.g. utop for 5.1:

# #require "base";;
# open Base;;
# ();;
- : unit/2 = ()

ncik-roberts avatar Nov 14 '23 05:11 ncik-roberts

I can reproduce with my 5.1.0 toplevel, but only if -short-paths is not passed. With -short-paths the toplevel prints unit as desired.

gasche avatar Nov 14 '23 05:11 gasche

Thanks for opening the issue, I agree that this is a regression for base. The general solution is probably to enable -short-path as mentioned by @gasche. But it may make sense to have an ultra-light short-path rule for disambiguation for type re-exports sharing the same base name.

Octachron avatar Nov 14 '23 09:11 Octachron

Something else appears to be going on here, beyond just -short-paths. I agree that -short-paths makes the printing better for the small reproduction I gave, but opening Base in utop gives the /2 type even with -short-paths:

$ utop -short-paths
# #require "base";;
# open Base;;
# ();;
- : unit/2 = ()

I haven't yet been able to get this down to a smaller reproduction.

ncik-roberts avatar Nov 14 '23 12:11 ncik-roberts

I can reproduce this when the declaration comes from another compilation unit.

$ cd /tmp
$ echo "type nonrec unit = unit" > u.ml
$ ocamlc u.ml
$ ocaml -short-paths u.cmo
# open U;;
# ();;
- : unit/2 = ()

gasche avatar Nov 14 '23 13:11 gasche

This being said: we have been in discussion for years about replacing the current short-paths implementation with a better one that is in Merlin ( https://github.com/ocaml/ocaml/issues/6600#issuecomment-473035999 ). Given the lack of availability of anyone to work on this, @Octachron has volunteered but he is busy with other things and hasn't come around to it yet. If I was @Octachron I would ignore this issue until the Merlin implementation of -short-paths has been looked at for real (it may even be that it fixes the issue by itself). Fixing minor issues in the legacy (upstream) implementation is probably a waste of time.

gasche avatar Nov 14 '23 13:11 gasche

Out of curiosity I tried to write a test in Merlin for 5.1 and I think I successfully failed to reproduce the issue:

  $ cat >u.ml <<'EOF'
  > type nonrec unit = unit
  > EOF

  $ $OCAMLC -c u.ml

  $ cat >main.ml <<'EOF'
  > open U;;
  > ();;
  > EOF

  $ $MERLIN single type-enclosing -position 2:1 -filename main.ml <main.ml | 
  > jq '.value[0].type' 
  "unit"

I always looked at printtyp from a very precautionary distance and it has always been a difficult part in upgrading Merlin to a new compiler version. I would be interested in learning more about how both the compiler and merlin implementations work, but right now I don't feel knowledgeable enough about it to try to fix this issue by myself.

voodoos avatar Jan 11 '24 14:01 voodoos