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

Empty let-bindings accepted by Ast_helper / pretty printer #7002

Closed
vicuna opened this issue Sep 30, 2015 · 3 comments
Closed

Empty let-bindings accepted by Ast_helper / pretty printer #7002

vicuna opened this issue Sep 30, 2015 · 3 comments
Labels
Milestone

Comments

@vicuna
Copy link

vicuna commented Sep 30, 2015

Original bug ID: 7002
Reporter: choeger
Status: closed (set by @xavierleroy on 2017-09-24T15:31:46Z)
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: #7111
Monitored by: @diml @hcarty

Bug description

Currently, the Ast_helper function Exp.let_ allows an empty set of bindings, which is not covered by later users (e.g. the pretty priner). This might yield to errors in the concrete syntax of dumped ASTs.

Steps to reproduce

#require "compiler-libs.bytecomp"
Pprintast.expression Format.str_formatter (Ast_helper.Exp.let_ Nonrecursive []
(Ast_helper.Exp.array [])) ; Format.flush_str_formatter () ;;

Additional information

Find attached a patch that solves the specific issue. I do not know what the best general strategy is, though.

File attachments

@vicuna
Copy link
Author

vicuna commented Sep 30, 2015

Comment author: @diml

We should probably have an Ast_invariant module to check that all the properties the compiler expect are satisfied after pre-processing. Apart from empty let-bindings, one can also produce identifiers that clash with keywords, empty tuples, ... and it's better to fail cleanly at the beginning than wait and get incomprehensible errors.

@vicuna
Copy link
Author

vicuna commented Sep 30, 2015

Comment author: @gasche

Another option in this case would be to have Exp.let_ just return the let body on empty bindings. If this is to be thought of a convenience module, I believe it should do it for me.

@vicuna
Copy link
Author

vicuna commented Feb 3, 2016

Comment author: @damiendoligez

The Ast_invariant idea was implemented in #392.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant