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

Incorrect parsing of toplevel "let%foo _ = expr" #6865

Closed
vicuna opened this issue May 11, 2015 · 12 comments
Closed

Incorrect parsing of toplevel "let%foo _ = expr" #6865

vicuna opened this issue May 11, 2015 · 12 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented May 11, 2015

Original bug ID: 6865
Reporter: @diml
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2017-02-16T14:16:15Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.1
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: back end (clambda to assembly)
Related to: #6464
Monitored by: @gasche

Bug description

The parsing of toplevel "let" statements is inconsistent. Currently these:

let%foo x = expr
let%foo _ = () and _ = ()
let%foo _ = ()

are parsed as:

[%foo let x = expr]
[%foo let _ = () and _ = ()]
let _ = [%foo ()]

The last one should instead be parsed as:

[%foo let _ = ()]

@vicuna
Copy link
Author

vicuna commented May 11, 2015

Comment author: @diml

Patch on github: #184

If there is no objection I'll apply it to 4.02 as the fix is quite simple.

@vicuna
Copy link
Author

vicuna commented May 11, 2015

Comment author: @lpw25

I think a better patch would be to just remove the special casing of let _ = ....

Damien and I had a look, and this special case dates from before the beginning of the CVS repository. I think we can safely assume that the reason for its existence no longer applies.

@vicuna
Copy link
Author

vicuna commented May 13, 2015

Comment author: @diml

It does slightly change the byte-code as a variable is introduced in the lambda and never eliminated. This mean that the evaluation of a module will use one more word on the stack per toplevel [let _ = expr]. There probably is a better way of doing this than the special case in the parser, but are we still happy with removing the special case in 4.02?

@vicuna
Copy link
Author

vicuna commented May 14, 2015

Comment author: @damiendoligez

Let's keep the special case for 4.02, but keep this PR open as a reminder to remove it later.

FTR, the problem with this special case is that it's an unexpected trap for ppx rewriters.

OK for applying your fix as is to 4.02.

@vicuna
Copy link
Author

vicuna commented May 15, 2015

Comment author: @diml

OK, fix applied to 4.02 in commit 16118.

@vicuna
Copy link
Author

vicuna commented Jul 13, 2015

Comment author: @diml

I applied the same patch as for 4.02 for now (commit 16199).

Removing the special casing is a bit annoying as to be consistent we should update the printer as well. And the printer must print [Pstr_eval _] as [let _ = e] as it doesn't print double-semicolons.

@vicuna
Copy link
Author

vicuna commented Dec 2, 2015

Comment author: @alainfrisch

Ok, I propose to:

  • Remove the special case in the parser. This removes the unexpected trap for authors of ppx.

  • Adapt the compilation of let bindings to avoid introducing a binding when compiling "let _ = ..." (also for local bindings).

  • Adapt the printer to print [Pstr_eval e] as [;; e]. This is ok since ";;" is now allowed at the beginning of structures.

@vicuna
Copy link
Author

vicuna commented Dec 2, 2015

Comment author: @alainfrisch

Commit 01bf671.

@vicuna
Copy link
Author

vicuna commented Dec 2, 2015

Comment author: @alainfrisch

This changes the behavior of the toplevel. Previously, typing "let _ = 10;;" was equivalent to "10;;", which printed:

- : int = 10

Now, this is interpreted as a normal let binding with 0 bound values, i.e. it shows nothing. I don't know why people would write "let _ = 10;;" in the toplevel, though, and this behavior is more logical.

If if this considered bad, it would be easy enough to add a special case to toploop.ml.

@vicuna
Copy link
Author

vicuna commented Dec 3, 2015

Comment author: @johnwhitington

I don't think the "let _ = 10" change in the toploop is a problem, it never worked with multiple structure items anyway:

let x = 1 let _ = 10;;

val x : int = 1

A little off-topic, but Standard ML systems have the special name 'it' for when we don't want to name things in the top level loop, which is rather pleasant:

Poly/ML 5.5.2 Release

4 + 3;;
val it = 7: int
it + 4;;
val it = 11: int

Maybe we could add a -it option to ocaml to enable this kind of behaviour.

@vicuna
Copy link
Author

vicuna commented Dec 3, 2015

Comment author: @gasche

I do think that a special case in toploop would be nice to avoid regressions. I'm thinking of users, for example, that #use a file with a series of let _ = ..., and expect (they tested it before) to see those intermediate results printed.

@vicuna
Copy link
Author

vicuna commented Dec 3, 2015

Comment author: @alainfrisch

Ok, as per Gabriel's request, I've restored the previous behavior for the toplevel (printing the result of 'let _ = expr'). Commit 9879d54.

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

No branches or pull requests

2 participants