Display a clear error on empty character literals '' #10197

gasche · 2021-02-04T08:45:16Z

Before:

# '';;
Error: Syntax error

After:

# '';;
Error: The empty character literal '' is invalid. We expect a character
       between single quotes (possibly a space ' ') or a single quote for
       type variables 'a.

Before, this input would get correctly lexed into QUOTE QUOTE and then
fail in the parser with a generic "Syntax error" message. (Even if we
had better error messages in the parser, here the parser-level error
would not be very illuminating as ' is never the start of a valid
expression or structure item).

Fixes #10196.

dra27

Code seems obviously fine - possible alternate message

dra27 · 2021-02-04T09:16:42Z

parsing/lexer.mll

+        "The empty character literal '' is invalid. \
+         We expect a character between single quotes (possibly a space ' ') \
+         or a single quote for type variables 'a."


Over here, using "we" when you are singular is generally reserved for the monarch 🙂 However, I think it's more consistent with the other messages to describe the properties of what is expected:

Suggested change

"The empty character literal '' is invalid. \

We expect a character between single quotes (possibly a space ' ') \

or a single quote for type variables 'a."

"Character expected between two single quotes (' ', for example). \

The empty character literal '' is invalid. \

A single quote is used for type variables 'a."

I'm unconvinced by your proposition: I have doubts that "Character expected ... ." is easier to read than "We expect a character ... .", and your message splits the allowed forms in two halves, one before and one after the diagnostic, I find this confusing.

If the We is really a problem (we, the authors of the compiler), I can change to "Invalid empty character literal ''.", but I would rather keep this structure -- or something else. But then there are many occurrences of "we" in the compiler text, mostly in comments or in the manual, but also a handful in error messages. (git grep ' [Ww]e ' | wc -l returns 2440 on my copy of trunk.)

If the We is really a problem (we, the authors of the compiler),

Personally except for the notable exception of did you mean-like error messages, I dislike the anthropomorphisation of computer program error messages.

Also there are already errors patterns in the character space with which consistency could be maintained e.g. this use of "illegal" vs "invalid".

# "bl\a";; Warning 14: illegal backslash escape in string. - : string = "bl\\a"

Here would be my proposal:

illegal empty character literal ''. Did you mean ' ' ?

The use of "we" in the manual or comments is entirely distinct from messages displayed by the tools. We may write the compiler, but on encountering an error the message comes from the compiler, not its authors. There may be other error messages elsewhere in the compiler which do this, but the other lexer error messages right around this one do not.

@dbuenzli's suggestion looks much better to me.

Okay, I went with

Error: Illegal empty character literal; did you mean ' ' or a type variable 'a ?

I would make the hint part more distinct:

Error: Illegal empty character literal Hint: Did you mean ' ' or a type variable 'a ?

(With the hope of one day using pretty printer for hints).

Error: Illegal empty character literal; did you mean ' ' or a type variable 'a ?

Sorry for the extreme hci bikesheding @gasche.

Since the original report is about the difficulty to distinguish spaces with certain fonts I think it would be good to write down the empty '' as was in your your original message and in my proposal.

That way a direct, local, comparison between '' and ' ' can be made which should easily show the difference; the context switch to the actual code may make you loose the distinction.

Also I notice the existing format for reporting Did you mean is slightly different:

# let fab x = x;; val fab : 'a -> 'a = <fun> # fib;; Error: Unbound value fib Hint: Did you mean fab?

Not that I'm necessarily in favor of that one. However I'm slightly averse to have the did you mean after a semicolon, rather than a ..

In this case I don't mind the bikeshedding, especially given that you all had good reasons to suggest changes. I will follow the standard "did you mean" format suggested by @Octachron, but include the '' back. (I agree it's interesting to show it.)

alainfrisch · 2021-02-04T10:18:35Z

Another confusing error is when we compile e.g. let c = 'é' with source code encoded in utf-8; I wonder if we could to something about it as well. We must be careful not to reject type 'foo' bar = ' (* wait for it... *) foo', though, which is, if not really ok, perfectly valid today.

gasche · 2021-02-04T10:57:13Z

We could have a regexp for just some sequences that are single-symbol-wide and likely culprits of this mistakes, but doing anything in the lexer would still prevent people from naming their type variable é' if they want to. I don't think we can do much without horrible hacks that are not worth it. (The best change would be to change the syntax of type ('a, 'b) list = ... into type list a b = ....)

alainfrisch · 2021-02-04T13:34:18Z

but doing anything in the lexer would still prevent people from naming their type variable é' if they want to

'é' is neither a valid type variable nor a valid character literal when the source code is encoded in utf-8. And when the source is encoded in latin-1, it's parsed as a character literal; you need to add some whitespace or comment between the quote and the name ' é'. So when seeing 'é' (utf-8) in source code, it's likely because of some recoding of a source code previously in latin-1 (or just someone not knowing enough about OCaml characters vs Unicode); in both case, one can safely assume the intent was to write a character literal, and complain with a proper error message. Anyway, this is really not the topic of this PR...

Octachron

The error message looks good to me and recognizing '' as an invalid character literal (or type variable name) seems sensible.

Octachron · 2021-02-05T10:24:06Z

parsing/lexer.mll

+  | Empty_character_literal ->
+      Location.errorf ~loc
+        "Illegal empty character literal ''\n\
+         Hint: Did you mean ' ' or a type variable 'a?"


Nitpicking: the hint could be a suberror message:

Location.error ~loc ~sub:[Location.msg "Hint: Did you mean ' ' or a type variable 'a?"] "Illegal empty character literal ''"

gasche · 2021-02-05T10:48:37Z

I changed the error message slightly:

  # '';;
  Error: Illegal empty character literal ''
    Hint: Did you mean a space character literal ' ' or a type variable 'a?

gasche · 2021-02-05T10:51:14Z

Shedding my own bike:

# '';;
Error: Illegal empty character literal ''
  Hint: Did you mean ' ' (a space character literal) or 'a (a type variable)?

Octachron · 2021-02-05T11:07:02Z

I liked the original error message better.
If you think that one need to be more explicit the second version is alright.
However, I am really not fond of the last version: parentheses only add visual noise in such a short message.

gasche · 2021-02-05T11:11:17Z

One thing I don't like about the short version is 'a?, having a punctuation mark immediately after code (that is not clearly delimited visually).

Before: > # '';; > Error: Syntax error After: > # '';; > Error: Illegal empty character literal '' > Hint: Did you mean ' ' or a type variable 'a? Before, this input would get correctly lexed into QUOTE QUOTE and then fail in the parser with a generic "Syntax error" message. (Even if we had better error messages in the parser, here the parser-level error would not be very illuminating as ' is never the start of a valid expression or structure item). Fixes ocaml#10196.

gasche · 2021-02-05T11:12:59Z

(Still went back to the short version. Maybe it's allright now?)

Octachron · 2021-02-05T11:25:55Z

My approval still stands for this version. I sympathize with the punctuation collision issue, but I don't see a way to circumvent it (without an uniform code quoting standard in error messages).

gasche · 2021-02-05T13:13:00Z

Merged, then. Thanks to everyone!

gasche mentioned this pull request Feb 4, 2021

Could the error message when passing an empty char be better? #10196

Closed

dra27 reviewed Feb 4, 2021

View reviewed changes

gasche force-pushed the error-empty-character-literal branch 4 times, most recently from 908c5f2 to 2b7916a Compare February 4, 2021 20:03

Octachron approved these changes Feb 5, 2021

View reviewed changes

gasche force-pushed the error-empty-character-literal branch from 2b7916a to 49cd6c4 Compare February 5, 2021 10:48

gasche force-pushed the error-empty-character-literal branch from 49cd6c4 to 1e712d7 Compare February 5, 2021 10:50

gasche force-pushed the error-empty-character-literal branch from 1e712d7 to 966d10c Compare February 5, 2021 11:12

gasche merged commit 243e39f into ocaml:trunk Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a clear error on empty character literals '' #10197

Display a clear error on empty character literals '' #10197

gasche commented Feb 4, 2021 •

edited

dra27 left a comment

dra27 Feb 4, 2021

gasche Feb 4, 2021

dbuenzli Feb 4, 2021 •

edited

dra27 Feb 4, 2021

gasche Feb 4, 2021 •

edited

Octachron Feb 4, 2021 •

edited

dbuenzli Feb 4, 2021

gasche Feb 4, 2021

alainfrisch commented Feb 4, 2021 •

edited

gasche commented Feb 4, 2021

alainfrisch commented Feb 4, 2021

Octachron left a comment

Octachron Feb 5, 2021 •

edited

gasche Feb 5, 2021

gasche commented Feb 5, 2021

gasche commented Feb 5, 2021 •

edited

Octachron commented Feb 5, 2021 •

edited

gasche commented Feb 5, 2021

gasche commented Feb 5, 2021

Octachron commented Feb 5, 2021

gasche commented Feb 5, 2021

Display a clear error on empty character literals '' #10197

Display a clear error on empty character literals '' #10197

Conversation

gasche commented Feb 4, 2021 • edited

dra27 left a comment

Choose a reason for hiding this comment

dra27 Feb 4, 2021

Choose a reason for hiding this comment

gasche Feb 4, 2021

Choose a reason for hiding this comment

dbuenzli Feb 4, 2021 • edited

Choose a reason for hiding this comment

dra27 Feb 4, 2021

Choose a reason for hiding this comment

gasche Feb 4, 2021 • edited

Choose a reason for hiding this comment

Octachron Feb 4, 2021 • edited

Choose a reason for hiding this comment

dbuenzli Feb 4, 2021

Choose a reason for hiding this comment

gasche Feb 4, 2021

Choose a reason for hiding this comment

alainfrisch commented Feb 4, 2021 • edited

gasche commented Feb 4, 2021

alainfrisch commented Feb 4, 2021

Octachron left a comment

Choose a reason for hiding this comment

Octachron Feb 5, 2021 • edited

Choose a reason for hiding this comment

gasche Feb 5, 2021

Choose a reason for hiding this comment

gasche commented Feb 5, 2021

gasche commented Feb 5, 2021 • edited

Octachron commented Feb 5, 2021 • edited

gasche commented Feb 5, 2021

gasche commented Feb 5, 2021

Octachron commented Feb 5, 2021

gasche commented Feb 5, 2021

gasche commented Feb 4, 2021 •

edited

dbuenzli Feb 4, 2021 •

edited

gasche Feb 4, 2021 •

edited

Octachron Feb 4, 2021 •

edited

alainfrisch commented Feb 4, 2021 •

edited

Octachron Feb 5, 2021 •

edited

gasche commented Feb 5, 2021 •

edited

Octachron commented Feb 5, 2021 •

edited