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

Cannot re-bind "exception False" #5778

Closed
vicuna opened this issue Oct 9, 2012 · 17 comments
Closed

Cannot re-bind "exception False" #5778

vicuna opened this issue Oct 9, 2012 · 17 comments

Comments

@vicuna
Copy link

vicuna commented Oct 9, 2012

Original bug ID: 5778
Reporter: beckerb
Assigned to: @diml
Status: closed (set by @xavierleroy on 2015-12-11T18:21:18Z)
Resolution: won't fix
Priority: normal
Severity: minor
Version: 4.00.1
Target version: later
Category: -for Camlp4 use https://github.com/ocaml/camlp4/issues

Bug description

Re-binding an exception [font=courier]False[/font] causes an error "Unbound exception constructor" when using Camlp4 with original syntax.

[code]

module M = struct exception False end;;

module M : sig exception False end

exception False = M.False;;

Error: Unbound exception constructor
M. False
[/code]

Note the space before [font=courier]False[/font] in the error message.

BTW, there is a possibly related glitch when pretty-printing that exception in revised syntax:
[code]

exception False;

exception false
[/code]

@vicuna
Copy link
Author

vicuna commented Oct 9, 2012

Comment author: @diml

Re-binding an exception [font=courier]False[/font] causes an error "Unbound exception constructor" when using Camlp4 with original syntax.

This part is fixed. Commits 12994 and 12995.

I think there are other places where the conversion is not done correctly: for example in "module False = struct end" the space at the beginning of False is kept.

[code]

exception False;

exception false
[/code]

It is another problem in the conversion from the Camlp4 Ast to the OCaml Ast.

@vicuna
Copy link
Author

vicuna commented Oct 9, 2012

Comment author: @bobzhang

can you pinpoint which printer is buggy? probably it's the toplevel printer (Camlp4Top/Rprint.ml)

@vicuna
Copy link
Author

vicuna commented Oct 9, 2012

Comment author: @bobzhang

camlp4>camlp4r -str 'exception False;' -printer o
exception False
Yes, I think it's the Rprint's problem, Rprint's not very useful in my opinion

@vicuna
Copy link
Author

vicuna commented Oct 9, 2012

Comment author: @diml

Actually it is not a printing problem. "False" can be used anywhere in revised syntax and Camlp4Ast2OCaml converts "False" to "false" (because the camlp4 ast uses "True" and "False" instead of "true" and "false", as in revised syntax):

$ echo 'exception False;' > exn.ml
$ ocamlc -dparsetree -pp camlp4r exn.ml
[
structure_item (exn.ml[1,0+0]..exn.ml[1,0+15])
Pstr_exception "false"
[]
]

@vicuna
Copy link
Author

vicuna commented Oct 9, 2012

Comment author: @bobzhang

I see, the compiler does not check in 'Pstr_exception str' str is an uidentifier?

In my branch I keep the original syntax and revised syntax the same, they both use true, false, other syntax also the same, like 'a:=!b ' instead of 'a.val:=b.val', how do you think of it?

@vicuna
Copy link
Author

vicuna commented Mar 5, 2013

Comment author: furuse

Today I found the same problem at

type t = [ True | False ]

which True and False are translated to True and False.

When fixing, could you check they are also resolved?

@vicuna
Copy link
Author

vicuna commented Mar 5, 2013

Comment author: @bobzhang

This might be a design mistake, in Fan, there is no such translation, like the original syntax, 'true', 'false' are keywords instead. Given that Camlp4 is already deprecated internally, I am not sure such bugs might be fixed in time

@vicuna
Copy link
Author

vicuna commented Mar 5, 2013

Comment author: @alainfrisch

Given that Camlp4 is already deprecated internally, I am not sure such bugs might be fixed in time

I think this is an over-statement. While there are efforts to move away from Camlp4 (Fan; -ppx and attributes; runtime types), it is fair to believe that Camlp4 will remain alive and used in large code bases for some more months/years.

@vicuna
Copy link
Author

vicuna commented Mar 5, 2013

Comment author: @diml

I already tried to fix this issue, and it is just a nightmare. There are lot of places where the conversion from one way to the other is missing, and we have to be careful not to double convert True and False, i.e. not to end up with: "module false". I'll try to find the courage to give it another try.

I think the best solution would be to remove these kind of conversions but there is a good chance it would break many syntax extensions, which often use revised syntax inside quotations.

The best workaround for now is to avoid using True and False with camlp4...

@vicuna
Copy link
Author

vicuna commented Mar 5, 2013

Comment author: @bobzhang

Yes, it's quite tricky, especially in the meta level, and make sure to know that Camlp4 support anti-quotation by two ways: 1. the normal Ant node (this is fine) 2. string mangling via "\$" prefix ( a place to introduce number of potential bugs)

@vicuna
Copy link
Author

vicuna commented Mar 6, 2013

Comment author: furuse

Everyone hates P4 but it is also true that it survives few more years.

If fixing is too hard, how about providing better error message? Is it also too hard to reject creating malformed idents/paths at rebuilding OCaml Parsetree? Currently, the error we get around True and False are so cryptic and need to run camlp4o to see what is happening. This is very cumbersome.

@vicuna
Copy link
Author

vicuna commented Mar 6, 2013

Comment author: @diml

Avoiding " True" and " False" should be feasible. This still means that sometimes we'll get "true" instead of "True" or the opposite.

BTW the printer and the camlp4 ast to ocaml parsetree converter are both mapping true and false independently so the output of camlp4o may be different to what is given to the compiler. [-dparsetree] or [-dsource] should be more helpful.

@vicuna
Copy link
Author

vicuna commented Mar 6, 2013

Comment author: @bobzhang

@dim, Maybe it's possible to remove some printers? the pprintast in the parsing directory is written using objects.

@vicuna
Copy link
Author

vicuna commented Jul 11, 2013

Comment author: @damiendoligez

What is the status of this PR? The status field says "assigned" (which is a flavour of "open"), but the "Fixed in Version" says "4.00.2+dev" (which implies that it is fixed.

@vicuna
Copy link
Author

vicuna commented Jul 16, 2013

Comment author: @diml

It is still open, the "Fixed in Version" was an error.

Actually if nobody objects or want to reassign the issue to himself I'll set the resolution to "won't fix". I don't see any simple fix and since camlp4 is going away I don't think it is worth spending too much time on this.

@vicuna
Copy link
Author

vicuna commented Jul 16, 2013

Comment author: @bobzhang

I would suggest a fundamental solution : no conversion table. What do you think?

@vicuna
Copy link
Author

vicuna commented Jul 16, 2013

Comment author: @diml

Well, that would indeed be much simpler, but that would break extensions written using revised syntax quotations, which are quite common.

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

1 participant