# Improving output of syntax errors - Thread: https://externals.io/message/110761 - Original archive: https://news-web.php.net/php.internals/110761 - Started by Rowan Tommins on 2020-06-28T18:53:21+00:00 - Messages: 8 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #110761 — Rowan Tommins — 2020-06-28T18:53:21+00:00 URL: https://externals.io/message/110761#110761 Hi all, During the discussion of "T_PAAMAYIM_NEKUDOTAYIM", there was broad agreement that internal token names should not be included in errors shown to users. I now have an implementation of this. Note that we currently only customise the token descriptions placed into Bison's hard-coded template, so the format remains "syntax error, unexpected %s, expecting %s or %s or %s". I have distinguished between two types of token: * Tokens which always represent the same text (e.g. keywords and operators) are represented by their standard form, e.g. 'unexpected token "static", expecting "function" ...' * Tokens which have variable content are given a user-friendly name, shown as well as the actual text when possible, e.g. 'unexpected identifier "foo", expecting quoted string ...' As a special-case, quoted strings show the string's *content* in double quotes, e.g. 'unexpected quoted string "foo" ...' rather than 'unexpected quoted string ""foo"" ...' or 'unexpected quoted string "'foo'" ...'. A "..." is also included if the text is longer than 30 bytes (where previously it would have been silently truncated). For example, given the following: <<<<<<<<<<<< The current 8.0 alpha will show this: Parse error: syntax error, unexpected '<<' (T_SL), expecting identifier (T_STRING) or static (T_STATIC) or namespace (T_NAMESPACE) or \\ (T_NS_SEPARATOR) in filename.php on line N The proposed patch will instead show this: Parse error: syntax error, unexpected token "<<", expecting identifier or "static" or "namespace" or "\" in filename.php on line N For more examples, see: https://rwec.co.uk/x/php-parse-errors/comparison.html The patch can be reviewed at: https://github.com/php/php-src/pull/5722 I am happy to post a small RFC if people think this requires a vote. Any other feedback is welcome. (As an aside, the other commonly requested change was to include column numbers; this appears to be feasible, but definitely more complex, and with potential performance trade-offs. I hope to re-visit this later.) Regards, -- Rowan Tommins (né Collins) [IMSoP] --- ## #110762 — Stanislav Malyshev — 2020-06-28T21:03:44+00:00 URL: https://externals.io/message/110761#110762 · In reply to: #110761 Hi! > For more examples, see: > https://rwec.co.uk/x/php-parse-errors/comparison.html > > The patch can be reviewed at: https://github.com/php/php-src/pull/5722 I think this is great, thanks for implementing it! > I am happy to post a small RFC if people think this requires a vote. > > Any other feedback is welcome. I personally don't think it requires a vote, as error messages are an implementation detail and nobody should ever rely on it. Let's wait for a bit though if somebody argues that it needs an RFC, but otherwise I think we can merge it. > (As an aside, the other commonly requested change was to include column > numbers; this appears to be feasible, but definitely more complex, and > with potential performance trade-offs. I hope to re-visit this later.) Column numbers would be awesome too. -- Stas Malyshev smalyshev@gmail.com --- ## #110763 — Joe Watkins — 2020-06-28T22:08:57+00:00 URL: https://externals.io/message/110761#110763 · In reply to: #110762 Nicely done. No need for an RFC. Thanks. > Hi! > > > […] > > I think this is great, thanks for implementing it! > > > […] > > I personally don't think it requires a vote, as error messages are an > implementation detail and nobody should ever rely on it. Let's wait for > a bit though if somebody argues that it needs an RFC, but otherwise I > think we can merge it. > > > […] > > Column numbers would be awesome too. > > -- > Stas Malyshev > smalyshev@gmail.com > > -- > > To unsubscribe, visit: https://www.php.net/unsub.php --- ## #110764 — Claude Pache — 2020-06-29T06:27:33+00:00 URL: https://externals.io/message/110761#110764 · In reply to: #110761 > As a special-case, quoted strings show the string's *content* in double quotes, e.g. 'unexpected quoted string "foo" ...' rather than 'unexpected quoted string ""foo"" ...' or 'unexpected quoted string "'foo'" ...'. For me, in this case, you have dropped the wrong pair of quotes. That is, I prefer to see the quotes used in the source text (“unexpected string 'foo' ...” or “unexpected string "foo" ...”), over the quotes that make error messages consistent with other error messages. This is because I like to have hints that help me to visually identify the offending string in the source text when it is of length 0 or 1, but on other hand I don’t care about consistency across error messages. —Claude --- ## #110766 — Rowan Tommins — 2020-06-29T08:29:56+00:00 URL: https://externals.io/message/110761#110766 · In reply to: #110764 Hi Claude, > > > > […] > quotes, e.g. 'unexpected quoted string "foo" ...' rather than 'unexpected > quoted string ""foo"" ...' or 'unexpected quoted string "'foo'" ...'. > > For me, in this case, you have dropped the wrong pair of quotes. That is, > I prefer to see the quotes used in the source text (“unexpected string > 'foo' ...” or “unexpected string "foo" ...”), over the quotes that make > error messages consistent with other error messages. This is because I like > to have hints that help me to visually identify the offending string in the > source text when it is of length 0 or 1, but on other hand I don’t care > about consistency across error messages. > I did actually have strings matching the quote marks used in an earlier version of the patch, but dropped it partly to simplify the implementation - because it sometimes truncates strings, it can't just use the real quotes, but has to strip and re-add them. The consistency should aid anyone trying to parse the error message, since the pattern /unexpected ([^"]+) "(.*?)"/ will always give you a token name (even if just 'token') and its content. I can re-visit that part if you feel strongly, though. Regards, -- Rowan Tommins [IMSoP] --- ## #110767 — Claude Pache — 2020-06-29T09:42:07+00:00 URL: https://externals.io/message/110761#110767 · In reply to: #110766 > Le 29 juin 2020 à 10:29, Rowan Tommins a écrit : > > > I did actually have strings matching the quote marks used in an earlier > version of the patch, but dropped it partly to simplify the implementation > - because it sometimes truncates strings, it can't just use the real > quotes, but has to strip and re-add them. > > The consistency should aid anyone trying to parse the error message, since > the pattern /unexpected ([^"]+) "(.*?)"/ will always give you a token name > (even if just 'token') and its content. > > I can re-visit that part if you feel strongly, though. At this point, I don’t feel strongly, because currently, when I see “unexpected '''' (T_CONSTANT_ENCAPSED_STRING)”, my automatism is not looking for a string with specific quote style (which is not evident by looking at four consecutive quotes), but for a missing or misplaced punctuation mark, because, by experience, it is most of the time the root cause of an error message containing that specific token name. Another way to give the programmer the appropriate information, is replacing “quoted string” with “single-quoted string” and “double-quoted string”. And while I am thinking about it: a case that tends to cause divergent squint is: unexpected '"', which you changed into: unexpected token """. In that specific case, it would be nice if it the common name of the offending token was spelt out, something like: unexpected double quotation mark. —Claude --- ## #110773 — Rowan Tommins — 2020-06-29T11:15:40+00:00 URL: https://externals.io/message/110761#110773 · In reply to: #110767 > > At this point, I don’t feel strongly, because currently, when I see > “unexpected '''' (T_CONSTANT_ENCAPSED_STRING)”, > my automatism is not looking for a string with specific quote style (which > is not evident by looking at four consecutive quotes), but for a missing or > misplaced punctuation mark, because, by experience, it is most of the time > the root cause of an error message containing that specific token name. > Yes, there's a bunch of cases where what the parser sees and expects isn't really that helpful, and what's parsed as a zero-length string may not actually look that way in the source. There's probably diminishing returns trying to handle that at the message level, rather than extending the parser itself as has been done for mis-matched brackets. > Another way to give the programmer the appropriate information, is > replacing “quoted string” with “single-quoted string” and “double-quoted > string”. > That would actually make a lot of sense. I'll have a look if it can be done without changing the actual parser definitions (which recognise both forms as the same token type). > And while I am thinking about it: a case that tends to cause divergent > squint is: unexpected '"', which you changed into: unexpected token """. In > that specific case, it would be nice if it the common name of the offending > token was spelt out, something like: unexpected double quotation mark. > That would be a reasonable special-case; theoretically, the same would apply for a lone single-quote, although I think the parser happens never to see that as a separate token. Thanks for the feedback, -- Rowan Tommins [IMSoP] --- ## #110845 — Rowan Tommins — 2020-07-05T16:49:28+00:00 URL: https://externals.io/message/110761#110845 · In reply to: #110767 > Another way to give the programmer the appropriate information, is > replacing “quoted string” with “single-quoted string” and > “double-quoted string”. > > And while I am thinking about it: a case that tends to cause divergent > squint is: unexpected '"', which you changed into: unexpected token """ I've pushed up a new version of the patch with three new special cases: * unexpected token """ -> unexpected double-quote mark * unexpected quoted string "foo" -> unexpected single-quoted string  "foo" / unexpected double-quoted string "foo" * unexpected illegal character "_" -> unexpected character 0xNN (where _ is almost certainly a control character, and NN is instead the hexadecimal value of the byte) I've updated the example table with a new column showing those additions: https://rwec.co.uk/x/php-parse-errors/comparison.html If there's no other comments or objections, I would appreciate if someone could merge this in so it can be tested in the next alpha :) Regards, -- Rowan Tommins (né Collins) [IMSoP]