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

-dsource output of ppx-deriving show has errant "in" #7111

Closed
vicuna opened this Issue Jan 1, 2016 · 7 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

commented Jan 1, 2016

Original bug ID: 7111
Reporter: leonidr
Status: closed (set by @xavierleroy on 2017-09-24T15:31:47Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.3
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: ~DO NOT USE (was: OCaml general)
Related to: #7002
Monitored by: @hcarty

Bug description

See steps below. I've tried it with various type declarations; all of the ones on the ppx_deriving documentation.

Steps to reproduce

$ cat test.ml
type a = Foo [@@deriving show]
$ PPD=ocamlfind query ppx_deriving
$ ocamlc -c -I $PPD -ppx "$PPD/ppx_deriving $PPD/ppx_deriving_show.cma" -dsource test.ml 2> test_part2.ml
$ echo $?
0
$ cat test_part2.ml
type a =
| Foo[@@deriving show]
let rec pp_a : Format.formatter -> a -> Ppx_deriving_runtime.unit=
in
((let open! Ppx_deriving_runtime in
fun fmt -> function | Foo -> Format.pp_print_string fmt "Test.Foo")
[@ocaml.warning "-A"])
and show_a : a -> Ppx_deriving_runtime.string=
fun x -> Format.asprintf "%a" pp_a x

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 4, 2016

Comment author: furuse

This happens when Pexp_let has an empty list for its binding. You should report it at ppx_deriving issues too.

In pprintast.ml, bindings method does nothing when the binding list is empty. I guess it should raise an exception instead.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 4, 2016

Comment author: @alainfrisch

Is anyone opposed to having the type-checker reject empty let bindings?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 4, 2016

Comment author: @diml

I'm for rejecting empty let bindings. There are a few other inconsistencies between the typer and pprintast. For instance empty P(str|sig)_type are ignored by the typer but cause an [assert false] in pprintast.

To improve things we can share the ill_formed_ast errors in the Syntaxerr module:

val empty_tuple : Location.t -> 'a
val empty_let : Location.t -> 'a
...

This is strictly better than getting [assert false] or incorrect output with [-dsource].

But I think it would be even better to check all the AST invariants once and for all after applying the ppx rewriters. To ensure that we are testing the same invariants between the typer and pprintast. I'll look into this.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 4, 2016

Comment author: @alainfrisch

But I think it would be even better to check all the AST invariants once and for all after applying the ppx rewriters.

Indeed. In addition to ensuring coherence between various "clients" of the Parsetree, it will formally document the invariants which are not expressed in the Parsetree definition.

I'll still prefer to reject empty let bindings in 4.03, even if this would be subsumed by your more ambitious idea later on. Or do you think you can work on it very soon? Of course, a first version does not need to enforce absolutely all invariants.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 4, 2016

Comment author: @diml

I'll still prefer to reject empty let bindings in 4.03, even if this would be subsumed by your more ambitious idea later on. Or do you think you can work on it very soon? Of course, a first version does not need to enforce absolutely all invariants.

I have a first working version that subsumes all the uses of [Syntaxerr.ill_formed_ast]. I'll create a PR this afternoon

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 4, 2016

Comment author: @diml

#392

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2016

Comment author: @damiendoligez

Empty let-bindings are rejected since #392 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.