Conversation
|
The current implementation, which relies on open recursion, lets people override the printer's behavior. The could be used e.g. to use special extension nodes to "undo" the effect of a ppx expander, etc. I'm not sure that simply dropping this open recursion scheme will be consensual. |
|
allow people to override its behavior was its original motivation to use open recursion |
|
I really don't think that the open recursion is worth it, it seems like extra complexity with little gain. Does anyone know of someone actually using it? Also, such a thing could easily be provided by a library -- it is not enough for it to be useful there must be a good reason for providing it in compiler-libs to justify its maintenance by the core team. |
You can do that by mapping on the ast before printing. I personally don't dislike open recursion in general, but as long as it's correct, I don't really care. I use the printer, but not the extensibility. |
|
Just to clarify my comment: I'm rather in favor of dropping the open recursion, we should just be careful to synchronize with the community. Mapping the AST is indeed more robust, because the current open recursion does not handle well the case were special forms should be recognized in contexts where the printer does some ad hoc treatment (and there are many such places). @bobzhang Are you aware of an actual project using the current API? |
|
What is the current quality of the testsuite coverage for pprintast? Is there another way to check that it is correct (eg. by reprinting and reparsing at parsing time, checking that the compiler can bootstrap this way), and if then, have you used then on the current patch proposal? Can we make them as simple as possible to use, so that we can trust that further changes to the printing code are correct? |
|
@alainfrisch I have some legacy code which used such api, but it is not very important. I do not have strong opinions on this issue, however, I don't see added value when switch to non-open recursion version neither. (with open recursion, if I found some bugs in pprintast, I can quickly fix it without waiting for upstream to fix it) |
|
@gasche after a quick look into @alainfrisch may I have your opinion on testing? |
|
No problem to include the script in the testsuite. It would also be useful to create a synthetic source file that exercise all current syntactic features, i.e. check with a code coverage tool that (i) all non-error branches in parser.mly are used; and (ii) all code is covered in pprintast.ml. This would be very useful also for migrating to a menhir based parser, and as a reference for alternative parsers in camlp4/campl5. |
The files under |
|
Thank you for the suggestions! Please delay this PR until I come up with a good test. |
|
May want to also investigate PR#7232. Confirmed that it's still broken with this PR. |
objmagic
force-pushed
the
fix-pprintast
branch
2 times, most recently
from
4128cee to
f2bf215
Compare
|
Test mechanism for cc @alainfrisch |
|
Thanks for the work! I very much like the work on the testsuite, which I think may be one of the long-term benefits of the change. |
|
Note: I removed the task description from the list of open tasks on the compiler hacking wiki, but I'm currently unable to edit the page Things previously worked on to add it there; could someone do it in my stead? The task description can be found in this diff. |
|
It would also be interesting to check how much how parser.mly is exercised with the test code. |
|
Perhaps also adding a script/makefile rule to automate running the coverage test? |
|
@alainfrisch that needs bisect_ppx dependency, which I am not sure if we should include it when building compiler. Do you mean we should include coverage test during Travis CI? Using bisect_ppx when building compiler is hard to me, so I extract only lexing and parsing part of the source code and run coverage test on them with your |
|
No need to include the coverage test in the default built or for the CI testing. It would just be useful to give instructions so that anyone can easily run the test again (e.g. after changing the printer or extending the language). |
Fix pprintast
Forcing_tag invalid argument to `Gc.finalise`
We're going to be calling `make_function_declaration_decision` from `Flambda_type`-land, so we need to split up the file to keep dependencies in check.
64235a3 flambda-backend: Change Float.nan from sNaN to qNaN (ocaml#466) 14a8e27 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (ocaml#569) c3cda96 flambda-backend: Add two new methods to targetint for dwarf (ocaml#560) e6f1fed flambda-backend: Handle arithmetic overflow in select_addr (ocaml#570) dab7209 flambda-backend: Add Target_system to ocaml/utils (ocaml#542) 82d5044 flambda-backend: Enhance numbers.ml with more primitive types (ocaml#544) 216be99 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (ocaml#536) 4b56e07 flambda-backend: Test naked pointer root handling (ocaml#550) 40d69ce flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (ocaml#537) f08ae58 flambda-backend: Implemented inlining history and use it inside inlining reports (ocaml#365) ac496bf flambda-backend: Disable the local keyword in typing (ocaml#540) 7d46712 flambda-backend: Bugfix for Typedtree generation of arrow types (ocaml#539) 61a7b47 flambda-backend: Insert missing page table check in roots_nat.c (ocaml#541) 323bd36 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (ocaml#534) d8956b0 flambda-backend: Persistent environment and reproducibility (ocaml#533) 4a0c89f flambda-backend: Revert "Revert bswap PRs (480 and 482)" (ocaml#506) 7803705 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (ocaml#376) 6199db5 flambda-backend: Improve unboxing during cmm for Flambda (ocaml#295) 96b9e1b flambda-backend: Print diagnostics at runtime for Invalid (ocaml#530) 42ab88e flambda-backend: Disable bytecode compilers in ocamltest (ocaml#504) 58c72d5 flambda-backend: Backport ocaml#10595 from upstream/trunk (ocaml#471) 1010539 flambda-backend: Use C++ name mangling convention (ocaml#483) 81881bb flambda-backend: Local allocation test no longer relies on lifting (ocaml#525) f5c4719 flambda-backend: Fix an assertion in Closure that breaks probes (ocaml#505) c2cf2b2 flambda-backend: Add some missing command line arguments to ocamlnat (ocaml#499) git-subtree-dir: ocaml git-subtree-split: 64235a3
Co-authored-by: Christine Rose <[email protected]>
8 participants
type t = < foo: int [@foo] >printed astype t = < foo[@foo ] :int >[%foo: < foo : t > ]printed as[%foo :< foo :t >]type foo += private A of intprinted astype foo += | A of int(private is dropped)let f : 'a . < .. > = assert falseprinted aslet f : 'a . < .. >= assert false(">=" is recognized as a token")
let module M = (functor (T : sig end) -> struct end)(struct end) in ()printed as
let module M = functor (T : sig end) -> struct end(struct end) in ()class c = object inherit ((fun () -> object end) ()) endprinted asclass c = object inherit (fun () -> object end ()) endThis task is one of OCaml Labs compiler hacking tasks (see detail)