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
Comments
Comment author: @lpw25 I think a better patch would be to just remove the special casing of 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. |
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? |
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. |
Comment author: @diml OK, fix applied to 4.02 in commit 16118. |
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. |
Comment author: @alainfrisch Ok, I propose to:
|
Comment author: @alainfrisch Commit 01bf671. |
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. |
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
Maybe we could add a -it option to ocaml to enable this kind of behaviour. |
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 |
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. |
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 _ = ()]
The text was updated successfully, but these errors were encountered: