#13263: fix escaping of true and false in printing functions #13560
Conversation
Both `true` and `false` should not be escaped using the raw identifier syntax if they appear in a context where a variant constructor is expected.
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
This looks okay and it does improve the testsuite output. I made a few nitpick comments.
| let open Format_doc.Doc in | ||
| let longident l = Format_doc.doc_printer longident l.Location.txt in | ||
| let longident ?(is_constr=false) l = | ||
| Format_doc.doc_printer (any_longident ~is_constr) l.Location.txt in |
There was a problem hiding this comment.
Nitpick: I would re-declare longident and constr here.
| let name = Datatype_kind.label_name kind in | ||
| let pr = match kind with | ||
| | Datatype_kind.Record -> quoted_longident | ||
| | Datatype_kind.Variant -> quoted_constr |
There was a problem hiding this comment.
It smells a bit weird to me to handle record field labels and constructor variant labels differently here. I understand that true and false are variant constructors and not record fields, but I would vote for handling them in the same way here unless this creates issues.
There was a problem hiding this comment.
The point is that with raw identifiers true is now both a valid record field and a valid constructor, but the record field must be escaped
type r = { \#true: int }whereas the variant constructor must be left unescaped
type s = trueIn practice, the variant case doesn't happen because this error path is only reachable with qualified name, and
M.true is a syntax error.
There was a problem hiding this comment.
I guess that the root of evil here is to pretend that the constructor true is an identifier; things would be simpler if reserved keywords also had reserved AST constructs.
There was a problem hiding this comment.
Another option would be to forbid \#true and \#false because according to OCaml lexing rule true and false clearly start with a capital letter.
There was a problem hiding this comment.
I think that this would also be perfectly reasonable. I have no opinion and I think that not showing \#true in real-world programs is probably more important than precisely how we deal with oddball programs, so I would follow your preference.
There was a problem hiding this comment.
I am inclined to fix first the printing bugs with this PR and continue the discussion later in #13246 .
(cherry picked from commit 6756917)
2 participants
Both
trueandfalseshould not be escaped using the raw identifier syntax if they appear in a context where a variant constructor is expected. For instance, inwe don't want to escape
trueinSimilarly, it is better to print
Path.To.(true)asPath.To.(true)and notPath.To.truenorPath.To.\#true.This PRs tweak the constructor printer in
Oprintto handle this last case and adds a specific printer forvariant constructor longidentfor error messages inPprintast.Along the way, it removes a backpatched printer in
Envthat has not been needed since the implementation of thelongidentprinters has moved toPprintastrather thanPrinttyp, and it fixes a missinginline codetag in a nearby error message.Close #13263