Expose printPattern

[amiralies]

No description provided.

[cristianoc]

The analysis will complain a little until this is used.
The suggestion changes depending on what follows from this PR. (Add an annotation or make it used somewhere).

[amiralies]

I will put this on hold until we decided if want to merge rescript-lang/rescript#5492 .
it results in printing patterns like this in partial match warning.

  Warning number 8
  /.../fixtures/warnings5.res:15:1-17:1

  13 │ }
  14 │ 
  15 │ switch y {
  16 │ | {typ: WithPayload(true)} => Js.log("first")
  17 │ }
  18 │ 
  19 │ let arr = [1]

  You forgot to handle a possible case here, for example: 
  {typ: WithPayload(false), _}
| {
  typ: Variant | (One | (Two | (Three | (Four | (Five | (Six | Seven(_))))))),
  _,
}

[cristianoc]

Let's go ahead with this.
You can add a @live annotation so the analysis does not complain about dead code.

[amiralies]

just pushed a commit.
I couldn't test locally since reanalyze is not working on my machine (for some reason it fails with cmi format error).
feel free to modify PR if the annotation is not correct.

[cristianoc]

CI is green so the annotation was fine.

opam install reanalyze should take care of your issue

[amiralies]

I got reanalyze installed.

OCAMLRUNPARAM=b make reanalyze 
dune build
reanalyze.exe -set-exit-code -all-cmt _build/default -suppress testrunner,compiler-libs-406 -exclude-paths compiler-libs-406 
Fatal error: exception Cmi_format.Error(_)
Raised at Cmt_format.read.(fun) in file "file_formats/cmt_format.ml", line 136, characters 11-72
Called from Misc.try_finally in file "utils/misc.ml", line 31, characters 8-15
Re-raised at Misc.try_finally in file "utils/misc.ml", line 45, characters 10-56
Called from Cmt_format.read_cmt in file "file_formats/cmt_format.ml", line 142, characters 8-21
Called from Dune__exe__Reanalyze.loadCmtFile in file "src/Reanalyze.ml", line 4, characters 18-52
Called from Stdlib__Array.iter in file "array.ml", line 95, characters 31-48
Called from Stdlib__Array.iter in file "array.ml", line 95, characters 31-48
Called from Dune__exe__Reanalyze.runAnalysis in file "src/Reanalyze.ml", line 89, characters 2-26
Called from Dune__exe__Reanalyze in file "src/Reanalyze.ml", line 225, characters 0-6
make: *** [Makefile:25: reanalyze] Error 2

seems like a compiler version mismatch or something?

[cristianoc]

Reanalyze can't work for ReScript and ocaml at the same time.
You probably have installed what works for ReScript (the npm version)

[cristianoc]

If you build from source there are 2 build commands for the 2 targets

From opam it's always the ml target

npm is always the ReScript (ie 4.06) target

[amiralies]

reanalyze binary points to opam installed version

will will investiage what is going on

Edit: Had some cmi/cmt files emited by a different compiler version. rm **/*.cm* got it working.

[cristianoc]

reanalyze binary points to opam installed version

will will investiage what is going on

Edit: Had some cmi/cmt files emited by a different compiler version. rm **/*.cm* got it working.

What did you change? Asking how to avoid this for the next contributor who tries.

[amiralies]

I don't know what was the exact cause but maybe I messed up my switch when upgrading compiler version or I left some old files compiled with different vesrion of compiler when switching between branchs.

I think best way to fix similar issues is to do a complete git clean and re-initate switch.

[cristianoc]

I don't know what was the exact cause but maybe I messed up my switch when upgrading compiler version or I left some old files compiled with different vesrion of compiler when switching between branchs.

I think best way to fix similar issues is to do a complete git clean and re-initate switch.

Was considering whether to make reanalyze resilient to wrong format, but I'm afraid that would only mask issues.

[cristianoc]

Is this ready to merge?

[amiralies]

This change by itself is ok but I think we can't use it for partial match warnings due this #595 (comment)

[cristianoc]

This change by itself is ok but I think we can't use it for partial match warnings due this #595 (comment)

I did not notice all the parens in that example.
Does it happen also when one formats some example code containing analogous patterns?

[amiralies]

if the input code contains parens in rhs or patterns it prints like that

https://rescript-lang.org/try?code=DYUwLgBAFAaghgJwJZwHaQD7QPKpBLKAFQHcB7A6IgCwRH0IDEyBXBSqRpANwegGUkAD0r8QvVFAD6ASjny5EALwRBQgFBA

[cristianoc]

Is it possible to generate the other way round so the printer is OK?
I guess that might be a change in the compiler?

[amiralies]

The printer seems to be fine (I couldn't produce adding unexpected parens) but the question is does it actually matter to preserve parens in or patterns? ocamlformat eliminates all of them.

I think it can be fixed in compiler too. compiler should generate patterns like

(or (p1 p2) p3)

instead of

(or p1 (or p2 p3))

[amiralies]

I guess parens doesn't matter to type checker but may affect code generator?

[cristianoc]

Changing the printer is definitely the less invasive option. Sounds like the way to go.

[amiralies]

We can ask @IwanKaramazow if it's fine to change printer behavior or not

[IwanKaramazow]

We have to be careful that if we change the printer, the output still parses correctly. I vaguely recall there being a difference in left vs right recursion

[amiralies]

There are some comments in reason's parser about this too

[cristianoc]

There are some comments in reason's parser about this too

Would you add a link to those comments.

[amiralies]

here it is https://github.com/reasonml/reason/blob/master/src/reason-parser/reason_pprint_ast.ml#L3156
not too much info tho

[cristianoc]

It might be just about technical properties.
For round trip tests to go through you need every valid ast to be representable after printing.
And this is not possible if left biased and right biased are both printed without paren.

On the other side one of the two could be considered invalid. This means parsing must always bias one way even if parens say otherwise. In other works parsing would also need to do the same work as the printer.

This technically would make round trip tests go through.

There would be ocaml asts not representable in ReScript. But that's is already true today.

None of this is a suggestion on how to proceed. Just stating the implications.

[IwanKaramazow]

We can also just implement a printer for this case, seems like not a lot of code at all: https://github.com/rescript-lang/ocaml/blob/1559b1d73e5fb725cabd4f90dd9a076172833558/typing/parmatch.ml#L386-L489