rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

[Feature Request] Print pattern matching exhaustiveness warnings in rescript syntax

Open amiralies opened this issue 4 years ago • 10 comments

Some (Done _)

->

Some(Done(_))

amiralies avatar Feb 16 '21 08:02 amiralies

@bobzhang I'm going to work on this do you think it should be done in syntax codebase as a module for printing patterns (like what we have for outcome tree) or it should be done inside current parmtch.ml module which prints in ml syntax?

amiralies avatar Aug 19 '21 02:08 amiralies

cc @IwanKaramazow Do you need outcomtree printer -> res syntax or parsetree -> res syntax?

bobzhang avatar Aug 19 '21 10:08 bobzhang

It seems that pretty_val in parmatch.ml operates on Typedtree.pattern. Am I correct that this pretty prints the error? If we can convert the Typedtree.pattern to the outcome tree then we can reuse the outcome printer, otherwise we'll need to implement a new one.

@bobzhang do you have a preference for whether this is done in the syntax repo or in the compiler repo?

IwanKaramazow avatar Aug 19 '21 12:08 IwanKaramazow

I had a look at parmatch.ml, it seems that it implements an ad-hoc Typedtree.pattern printer, so

  • We just adapted it for rescript syntax (minimal changes, drop the ml syntax for error message)
  • Typedtree.pattern --> Untypedast -> Parsetree.pattern -> print

IIRC, outcometree does not have patterns, it is only for values.

@amiralies which route would you prefer?

bobzhang avatar Aug 20 '21 00:08 bobzhang

I think both are okay. the second approach would look something like this

let print_pat (p: Typedtree.pattern) =
  let pattern = Untypeast.default_mapper.pat Untypeast.default_mapper p in
  let doc = Res_printer.printPattern pattern Res_comments_table.empty in
  Res_doc.toString ~width:80 doc

is this ok for the above function to live in Parmatch? if so, how can i access to napkin modules? i'm not familiar with build configurations in the project.

amiralies avatar Aug 20 '21 05:08 amiralies

Currently Napkin depends on compiler-libs which is ml directory, so here you have cycles.

You can use a reference let res_pattern_printer = fun _ -> assert false and populate it with the right one in bs_conditional_initial.

When you make the contributions, make sure the diff is small so that it is easy to review, the review itself would take lots of time, thanks!

If you miss any API for pattern printer, @IwanKaramazow is the best to reach out

bobzhang avatar Aug 20 '21 06:08 bobzhang

Okay I got something working but the way Untypeast.default_mapper.pat untypes patterns is not going well with pattern printer in Res_printer so I think i've to introduce another function to untype a pattern.

Now I think it's the right way the we are going to use the pattern printer from Res_printer rathar than a custom pattern printer like the one in parmatch for ml because it results in duplicated logic and there are some edge case like exotic idents/variants etc that are well handled in Res_printer

amiralies avatar Aug 21 '21 01:08 amiralies

but the way Untypeast.default_mapper.pat untypes patterns is not going well with pattern printer in Res_printer

Can you elaborate

bobzhang avatar Aug 22 '21 00:08 bobzhang

When converting Tpat_construct and Tpat_record it uses pattern's longident to convert into Ppat_... For example for a constructor like Done the longident is like #$Done0 i think it should use cstr_desc.cstr_name. same story for record labels.

amiralies avatar Aug 22 '21 01:08 amiralies

@bobzhang sorry for pinging, should i create a new untypeast mapper and use cstr_name or just go with updating printer in parmatch.ml to print rescript syntax?

amiralies avatar Dec 11 '21 16:12 amiralies

This is already done in the meantime.

cknitt avatar May 27 '23 19:05 cknitt