Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in printing of shadowed predef types #12738

Open
ncik-roberts opened this issue Nov 14, 2023 · 6 comments
Open

Regression in printing of shadowed predef types #12738

ncik-roberts opened this issue Nov 14, 2023 · 6 comments
Assignees
Labels
Milestone

Comments

@ncik-roberts
Copy link
Contributor

(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 = ()
@gasche
Copy link
Member

gasche commented Nov 14, 2023

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.

@Octachron
Copy link
Member

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.

@ncik-roberts
Copy link
Contributor Author

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.

@gasche
Copy link
Member

gasche commented Nov 14, 2023

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
Copy link
Member

gasche commented Nov 14, 2023

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 ( #6600 (comment) ). 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.

@voodoos
Copy link
Contributor

voodoos commented Jan 11, 2024

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.

@Octachron Octachron added the bug label Feb 1, 2024
@Octachron Octachron self-assigned this Mar 6, 2024
@Octachron Octachron modified the milestones: 5.2, 5.3 Apr 29, 2024
@OlivierNicole OlivierNicole modified the milestones: 5.3.0, 5.4 features Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants