Improve output of tokens in Parse Errors #5722

IMSoP · 2020-06-15T22:05:30Z

Currently, unexpected tokens in the parser are shown as the text found, plus the internal token name, including the notorious "unexpected '::' (T_PAAMAYIM_NEKUDOTAYIM)".

This PR replaces that with a more user-friendly format, with two types of token:

Tokens which always represent the same text are shown like unexpected token "::" and expected "::"
Tokens which have variable text are given a user-friendly name, and show like unexpected identifier "foo", and expected identifier.

As a special-case, quoted strings are not quoted an extra time, so show like unexpected quoted string "foo" rather than unexpected quoted string ""foo"" or unexpected quoted string "'foo'".

Examples

Parse error: syntax error, unexpected token "<<", expecting identifier or "static" or "namespace" or "\" in Command line code on line 1
Parse error: syntax error, unexpected token "::", expecting end of file in Command line code on line 1
Parse error: syntax error, unexpected identifier "foo", expecting "," or ";" in Command line code on line 1
Parse error: syntax error, unexpected quoted string "foo" in Command line code on line 1

For a larger selection of examples, see https://rwec.co.uk/x/php-parse-errors/comparison.html

TODO

Test for edge cases, such as zero-length tokens, backslashes.
Update all the phpt tests which expect a particular error output.
Put together some before and after examples for discussion / RFC.

nikic · 2020-06-16T09:15:53Z

Update all the phpt tests which expect a particular error output; if I can get column numbers working, I will update the tests for both patches in one go.

You can use scripts/dev/bless_test.php to update tests. For column numbers, I expect that you will need #3948 as a base, so I'd suggest leaving that for a separate PR.

IMSoP · 2020-06-16T14:15:01Z

You can use scripts/dev/bless_test.php to update tests.

Ah, thanks. I was thinking there must be a way to automate that. (I also need to sort out a proper VM to run tests under, because WSL confuses them.)

For column numbers, I expect that you will need #3948 as a base, so I'd suggest leaving that for a separate PR.

Yes, I have an initial prototype working with the default Bison location struct (first_line, first_column, last_line, last_column), and was indeed planning to open a separate PR once I get it working properly. I see that patch was reverted on performance grounds, which I had wondered about; I'll discuss some further thoughts once I raise the PR.

Zend/zend_language_parser.y

IMSoP · 2020-06-25T22:00:37Z

Rebased to current master
Fixed handling of zero-length tokens
Blessed most tests to reflect the new format (there's a couple I want to look at more closely as they may be real bugs)

KalleZ · 2020-06-27T14:49:12Z

@hikari-no-yume

hikari-no-yume · 2020-06-27T14:51:53Z

Instead of 'foo' or 'bar' or 'baz', would it be possible to do 'foo', 'bar', or 'baz'?

IMSoP · 2020-06-27T15:02:29Z

Instead of 'foo' or 'bar' or 'baz', would it be possible to do 'foo', 'bar', or 'baz'?

The bulk of the error message is generated by Bison; we currently only over-ride a function that formats the individual tokens, which are then substituted into a printf-style template. For the same reason, it's tricky to capitalise the "s" of "syntax error".

We could potentially abuse the i18n facility by providing our own YY_ macro to re-map the message templates, but I'm not sure the improvement is worth the complexity, and it might not even work correctly on all tool versions, as it's explicitly marked internal.

nikic

Looks good, just some style nits.

Zend/zend_language_parser.y

Currently, unexpected tokens in the parser are shown as the text found, plus the internal token name, including the notorious "unexpected '::' (T_PAAMAYIM_NEKUDOTAYIM)". This commit replaces that with a more user-friendly format, with two main types of token: * Tokens which always represent the same text are shown like 'unexpected token "::"' and 'expected "::"' * Tokens which have variable text are given a user-friendly name, and show like 'unexpected identifier "foo"', and 'expected identifer'. A few tokens have special cases: * unexpected token """ -> unexpected double-quote mark * unexpected quoted string "'foo'" -> unexpected single-quoted string "foo" * unexpected quoted string ""foo"" -> unexpected double-quoted string "foo" * unexpected illegal character "_" -> unexpected character 0xNN (where _ is almost certainly a control character, and NN is the hexadecimal value of the byte) The \ token has a special case in the implementation just to stop bison making a mess of escaping it and it coming out as \\

IMSoP · 2020-07-12T20:15:23Z

Thanks for the review @nikic

Tidied, rebased to master, and with updated examples for the last string change here: https://rwec.co.uk/x/php-parse-errors/comparison.html

kocsismate · 2020-07-12T20:17:36Z

Zend/tests/attributes/019_variable_attribute_name.phpt

@@ -8,4 +8,4 @@ class A {}

 ?>
 --EXPECTF--
-Parse error: syntax error, unexpected '$x' (T_VARIABLE), expecting identifier (T_STRING) or static (T_STATIC) or namespace (T_NAMESPACE) or \\ (T_NS_SEPARATOR) in %s on line %d
+Parse error: syntax error, unexpected variable "$x", expecting identifier or "static" or "namespace" or "\" in %s on line %d


Could the syntax error start with a capital letter? If it's not too difficult to achieve then I'd love it, since a lot of messages have been capitalized for PHP 8. If this change would require a non-negligible amount of work, please ignore this request :)

@kocsismate I looked at that, and unfortunately that string comes from a different function somewhere deep in Bison's generated code. :(

The best approach I can think of is to copy it to a new string and fix the capital letter somewhere in the process of creating the Error object.

If it requires quirks like that then I think we can also live with a lowercase letter :) Thanks for checking it!

nikic · 2020-07-13T09:08:32Z

Nice work, I really like the final outcome! Merged as 55a15f3.

IMSoP changed the title ~~DRAFT: Improve output of tokens in Parse Errors~~ Improve output of tokens in Parse Errors Jun 15, 2020

nikic reviewed Jun 25, 2020

View reviewed changes

Zend/zend_language_parser.y Show resolved Hide resolved

IMSoP force-pushed the better-parse-error-tokens branch 2 times, most recently from 4f64bc2 to ea696b4 Compare June 25, 2020 21:57

IMSoP force-pushed the better-parse-error-tokens branch 3 times, most recently from 3e426bd to 022e6cd Compare June 28, 2020 15:09

IMSoP marked this pull request as ready for review June 28, 2020 17:01

IMSoP force-pushed the better-parse-error-tokens branch 2 times, most recently from da71157 to d9c1c90 Compare July 5, 2020 16:01

nikic reviewed Jul 9, 2020

View reviewed changes

IMSoP force-pushed the better-parse-error-tokens branch 3 times, most recently from 239bde7 to cea470c Compare July 12, 2020 18:32

IMSoP added 2 commits July 12, 2020 18:36

Fix tests to match new parse error format

b7fa69d

IMSoP force-pushed the better-parse-error-tokens branch from cea470c to b7fa69d Compare July 12, 2020 18:36

kocsismate reviewed Jul 12, 2020

View reviewed changes

nikic approved these changes Jul 13, 2020

View reviewed changes

nikic closed this Jul 13, 2020

carusogabriel added this to the PHP 8.0 milestone Jul 13, 2020

Improve output of tokens in Parse Errors #5722

Improve output of tokens in Parse Errors #5722

Uh oh!

Conversation

IMSoP commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Examples

TODO

Uh oh!

nikic commented Jun 16, 2020

Uh oh!

IMSoP commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

IMSoP commented Jun 25, 2020

Uh oh!

KalleZ commented Jun 27, 2020

Uh oh!

hikari-no-yume commented Jun 27, 2020

Uh oh!

IMSoP commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IMSoP commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kocsismate Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

IMSoP Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

kocsismate Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Jul 13, 2020

Uh oh!

Uh oh!

IMSoP commented Jun 15, 2020 •

edited

Loading

IMSoP commented Jun 16, 2020 •

edited

Loading

IMSoP commented Jun 27, 2020 •

edited

Loading

IMSoP commented Jul 12, 2020 •

edited

Loading

kocsismate Jul 12, 2020 •

edited

Loading