Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Improved parser error message »

[RFC] Improved parser error message

php.internals

Felipe Nascimento Silva Pena

15 years ago

Hi all,
As I have proposed previously in an old thread... What about we name all the
tokens to have an improved parser error message? (i.e. anymore
T_PAAMAYIM_NEKUDOTAYIM, T_DOLLAR_OPEN_CURLY_BRACES in the messages etc)

Some examples:

$ sapi/cli/php -r 'function ""'
Patched:
Parse error: syntax error, unexpected quoted-string, expecting identifier or
'(' in Command line code on line 1

Current:
Parse error: syntax error, unexpected T_CONSTANT_ENCAPSED_STRING, expecting
T_STRING or '(' in Command line code on line 1


$ sapi/cli/php -r 'echo ::a;'
Patched:
Parse error: syntax error, unexpected :: in Command line code on line 1

Current:
Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in Command line
code on line 1

Other examples and patch at:
https://wiki.php.net/rfc/improved-parser-error-message

Any thoughts?

Thanks.

--
Regards,
Felipe Pena

Ferenc Kovacs

15 years ago

On Mon, May 16, 2011 at 2:15 PM, Felipe Pena <felipensp@gmail.com> wrote:

> Hi all,
> As I have proposed previously in an old thread... What about we name all
> the
> tokens to have an improved parser error message? (i.e. anymore
> T_PAAMAYIM_NEKUDOTAYIM, T_DOLLAR_OPEN_CURLY_BRACES in the messages etc)
>
> Some examples:
>
> $ sapi/cli/php -r 'function ""'
> Patched:
> Parse error: syntax error, unexpected quoted-string, expecting identifier
> or
> '(' in Command line code on line 1
>
> Current:
> Parse error: syntax error, unexpected T_CONSTANT_ENCAPSED_STRING, expecting
> T_STRING or '(' in Command line code on line 1
>
>
> $ sapi/cli/php -r 'echo ::a;'
> Patched:
> Parse error: syntax error, unexpected :: in Command line code on line 1
>
> Current:
> Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in Command
> line
> code on line 1
>
> Other examples and patch at:
> https://wiki.php.net/rfc/improved-parser-error-message
>
> Any thoughts?
>
> Thanks.
>
> --
> Regards,
> Felipe Pena
>

+1

do you think that this could cause some kind of BC?
I don't think that anybody out there parses the tokens as is, but there are
crazy people. :)
the other thing that I can imagine that the developers will be surprised
that they are getting meaningful error messages, but I think they can live
with that. :)

Tyrael

Felipe Nascimento Silva Pena

15 years ago

Hi,

2011/5/16 Ferenc Kovacs <info@tyrael.hu>

>
> +1
>
> do you think that this could cause some kind of BC?
> I don't think that anybody out there parses the tokens as is, but there are
> crazy people. :)
> the other thing that I can imagine that the developers will be surprised
> that they are getting meaningful error messages, but I think they can live
> with that. :)
>
> Tyrael
>

There is no BC. The changes doesn't affects token_*() functions for example.

$ sapi/cli/php -r 'var_dump(token_name(318));'
string(6) "T_ECHO"

I added such information to the wiki page. Thanks. :)

--
Regards,
Felipe Pena

Etienne Kneuss

15 years ago

Hi Felipe,

On May 16  9:15:24, Felipe Pena wrote:

> Hi all,
> As I have proposed previously in an old thread... What about we name all the
> tokens to have an improved parser error message? (i.e. anymore
> T_PAAMAYIM_NEKUDOTAYIM, T_DOLLAR_OPEN_CURLY_BRACES in the messages etc)
>
> Some examples:
>
> $ sapi/cli/php -r 'function ""'
> Patched:
> Parse error: syntax error, unexpected quoted-string, expecting identifier or
> '(' in Command line code on line 1
>
> Current:
> Parse error: syntax error, unexpected T_CONSTANT_ENCAPSED_STRING, expecting
> T_STRING or '(' in Command line code on line 1
>
>
> $ sapi/cli/php -r 'echo ::a;'
> Patched:
> Parse error: syntax error, unexpected :: in Command line code on line 1
>
> Current:
> Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in Command line
> code on line 1
>
> Other examples and patch at:
> https://wiki.php.net/rfc/improved-parser-error-message
>
> Any thoughts?

That's great, it will save millions of headaches.

Best,

Pierrick Charron

15 years ago

+1

Regards
Pierrick

On 16 May 2011 08:15, Felipe Pena <felipensp@gmail.com> wrote:

Florian Anderiasch

15 years ago

On 16.05.2011 14:15, Felipe Pena wrote:

> Other examples and patch at:
> https://wiki.php.net/rfc/improved-parser-error-message

Just for completeness, here's the lengthy thread
we had in October 2010 about this:

http://news.php.net/php.internals/49978

FWIW, I like the Easter Egg :)

Greetings,
Florian

Julien Pauli

15 years ago

I remember error messages have been improved since last decade, but
they were engine ones.
Parser ones have never been touched, so to improve them, I +1 your
idea Felipe :)

Julien

On Mon, May 16, 2011 at 7:52 PM, Florian Anderiasch <ml@anderiasch.de> wrote:

Felipe Nascimento Silva Pena

15 years ago

2011/5/16 Felipe Pena <felipensp@gmail.com>

> Hi all,
> As I have proposed previously in an old thread... What about we name all
> the tokens to have an improved parser error message? (i.e. anymore
> T_PAAMAYIM_NEKUDOTAYIM, T_DOLLAR_OPEN_CURLY_BRACES in the messages etc)
>

> [...]
>

>

Other examples and patch at:

> https://wiki.php.net/rfc/improved-parser-error-message
>
> Any thoughts?
>
> Thanks.
>

So... In case of no objections, I'll commit it in trunk and 5_4 branch, Is
it okay? :)

--
Regards,
Felipe Pena

Andi Gutmans

15 years ago

Hi Felipe,

Read the archives. We discussed T_PAAMAYIM_NEKUDOTAYIM extensively and agreed to keep it as a historical landmark. Let's not have the discussion yet again within the same year. Google it just to see the # of mentions.

What we could do as a compromise between history and improving many of the parse error messages is:
Parse error: syntax error, unexpected :: (T_PAAMAYIM_NEKUDOTAYIM) in Command line code on line 1

Generally speaking I'm OK with the other changes but I don't know if any extensions or scripts depend on them (probably not).
I suggest to give it another couple of days to see if anyone has any valid concerns on breakage this could cause.

Andi

Gustavo Lopes

15 years ago

Em Tue, 17 May 2011 17:01:26 +0100, Andi Gutmans <andi@zend.com> escreveu:

> Read the archives. We discussed T_PAAMAYIM_NEKUDOTAYIM extensively and
> agreed to keep it as a historical landmark. Let's not have the
> discussion yet again within the same year. Google it just to see the #
> of mentions.
>
> What we could do as a compromise between history and improving many of
> the parse error messages is:
> Parse error: syntax error, unexpected :: (T_PAAMAYIM_NEKUDOTAYIM) in
> Command line code on line 1
>

This proposal is not about changing the name of the token, only the
message that's shown to the user. The token name will remain as is in
zend_language_parser.y et al. If I recall correctly, there was not
objection raised to changing the message in the last discussion.

Anyway, +1 to the change from you.

--
Gustavo Lopes

Gustavo Lopes

15 years ago

Em Tue, 17 May 2011 17:07:09 +0100, Gustavo Lopes <glopes@nebm.ist.utl.pt>
escreveu:

> [...]
> Anyway, +1 to the change from you.
>

Sorry, it should read "from me".

--
Gustavo Lopes

Ferenc Kovacs

15 years ago

On Tue, May 17, 2011 at 6:01 PM, Andi Gutmans <andi@zend.com> wrote:

> Hi Felipe,
>
> Read the archives. We discussed T_PAAMAYIM_NEKUDOTAYIM extensively and
> agreed to keep it as a historical landmark. Let's not have the discussion
> yet again within the same year. Google it just to see the # of mentions.
>
>

Maybe you don't remember, but Felipe participated in that discussion, to be
exact he proposed this exact feature there:
http://www.mail-archive.com/internals@lists.php.net/msg48173.html
after Felipe's mail, nobody objected about replacing the constants with
meaningful names, as it was the original idea, but nobody found a clean way
to implement it before.

> What we could do as a compromise between history and improving many of the
> parse error messages is:
> Parse error: syntax error, unexpected :: (T_PAAMAYIM_NEKUDOTAYIM) in
> Command line code on line 1
>
>

maybe it's a little bit redundant, but it can be a good transition.

> Generally speaking I'm OK with the other changes but I don't know if any
> extensions or scripts depend on them (probably not).
> I suggest to give it another couple of days to see if anyone has any valid
> concerns on breakage this could cause.
>
>

as Felipe mentioned, his patch doesn't modify the token names(so the token_
functions aren't affected by this change), only the rendering of the error
messages.

Tyrael

Derick Rethans

15 years ago

On Tue, 17 May 2011, Felipe Pena wrote:

> 2011/5/16 Felipe Pena <felipensp@gmail.com>
>
> > Hi all,
> > As I have proposed previously in an old thread... What about we name all
> > the tokens to have an improved parser error message? (i.e. anymore
> > T_PAAMAYIM_NEKUDOTAYIM, T_DOLLAR_OPEN_CURLY_BRACES in the messages etc)
> >
>
> > [...]
> >
>
> >
> Other examples and patch at:
> > https://wiki.php.net/rfc/improved-parser-error-message
> >
> > Any thoughts?
> >
> > Thanks.
> >
>
> So... In case of no objections, I'll commit it in trunk and 5_4 branch, Is
> it okay? :)

Go for it.

Derick

Zeev Suraski

15 years ago

+1 with Andi's improvement.

Gustavo - I realize it's not about changing the token's name, but nobody (=very few) looks at the token names.  The point is to keep the tradition of also exposing this specific token name to the user, but still making it clear that what was expected was :: - without the need to head to Google Translate...

Zeev

Stas Malyshev

15 years ago

Hi!

I think we need to keep token name in the message, since it makes it
easier to understand what parser expected if you need to debug the
parser (as opposed to your code). So I think we need to have both the
human-readable name and the token name, as Andi suggested.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Ferenc Kovacs

15 years ago

On Tue, May 17, 2011 at 6:49 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> Hi!
>
> I think we need to keep token name in the message, since it makes it easier
> to understand what parser expected if you need to debug the parser (as
> opposed to your code). So I think we need to have both the human-readable
> name and the token name, as Andi suggested.
>

on the sidenote, I don't think that many people out there would start
debuging the php src if they are presented with an error by the engine.
but I'm fine with Andi's suggestion.

Tyrael

Rasmus Lerdorf

15 years ago

On 05/17/2011 09:58 AM, Ferenc Kovacs wrote:

> On Tue, May 17, 2011 at 6:49 PM, Stas Malyshev<smalyshev@sugarcrm.com>wrote:
>
>> Hi!
>>
>> I think we need to keep token name in the message, since it makes it easier
>> to understand what parser expected if you need to debug the parser (as
>> opposed to your code). So I think we need to have both the human-readable
>> name and the token name, as Andi suggested.
>>
>
> on the sidenote, I don't think that many people out there would start
> debuging the php src if they are presented with an error by the engine.
> but I'm fine with Andi's suggestion.

While that is true, I did run across some code a while back that used
the tokenizer extension in conjunction with the error messages. If we
decouple the error messages completely from the tokens there would be no
way to write that same code. An edge case, but I don't think it hurts to
keep the token name there.

-Rasmus

Gustavo Lopes

15 years ago

Em Tue, 17 May 2011 17:49:53 +0100, Stas Malyshev <smalyshev@sugarcrm.com>
escreveu:

> I think we need to keep token name in the message, since it makes it
> easier to understand what parser expected if you need to debug the
> parser (as opposed to your code). So I think we need to have both the
> human-readable name and the token name, as Andi suggested.

I think this alternative has very little value for debugging the parser.
There are no repeated labels for the tokens, so even if you don't know the
token name by heart, you can look it up in 10 seconds.

Keeping the token name will only perpetuate the confusion it has caused. I
think it ought to be dropped, but if no consensus can be reached, this
would be better than the status quo.

--
Gustavo Lopes

Felipe Nascimento Silva Pena

15 years ago

2011/5/18 Gustavo Lopes <glopes@nebm.ist.utl.pt>:

> Em Tue, 17 May 2011 17:49:53 +0100, Stas Malyshev <smalyshev@sugarcrm.com>
> escreveu:
>
>> I think we need to keep token name in the message, since it makes it
>> easier to understand what parser expected if you need to debug the parser
>> (as opposed to your code). So I think we need to have both the
>> human-readable name and the token name, as Andi suggested.
>
> I think this alternative has very little value for debugging the parser.
> There are no repeated labels for the tokens, so even if you don't know the
> token name by heart, you can look it up in 10 seconds.
>
> Keeping the token name will only perpetuate the confusion it has caused. I
> think it ought to be dropped, but if no consensus can be reached, this would
> be better than the status quo.
>
> --
> Gustavo Lopes
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

So, following some suggestion I made a patch wich displays such messages:


$ sapi/cli/php -r 'class '
Current:
Parse error: syntax error, unexpected $end, expecting T_STRING

Patched:
Parse error: syntax error, unexpected end of file, expecting
'identifier' (T_STRING)

$ sapi/cli/php -r 'class abc foo'
Current:
Parse error: syntax error, unexpected T_STRING, expecting '{' in
Command line code on line 1

Patched:
Parse error: syntax error, unexpected 'foo' (T_STRING), expecting '{'
in Command line code on line 1


As can be noticed, I added the actual scanned string in the
"unexpected" part. This might be useful for finding really which makes
the parser error. (It was a bit tricky though :D)


Any thoughts?
https://wiki.php.net/rfc/improved-parser-error-message

--
Regards,
Felipe Pena

Jordi Boggiano

15 years ago

Thanks for the great work. More should be done on the front of helping
newcomers solve trivial issues imo.

On 23.06.2011 20:08, Felipe Pena wrote:

> $ sapi/cli/php -r 'class '
> Current:
> Parse error: syntax error, unexpected $end, expecting T_STRING
>
> Patched:
> Parse error: syntax error, unexpected end of file, expecting
> 'identifier' (T_STRING)

identifier should not be quoted. The quotes are fine for "expecting '{'"
and "unexpected 'foo'" because those are text literals, but this implies
you were supposed to actually type "class identifier", which is
obviously not the intended message.

> $ sapi/cli/php -r 'class abc foo'
> Current:
> Parse error: syntax error, unexpected T_STRING, expecting '{' in
> Command line code on line 1
>
> Patched:
> Parse error: syntax error, unexpected 'foo' (T_STRING), expecting '{'
> in Command line code on line 1
>
>
> As can be noticed, I added the actual scanned string in the
> "unexpected" part. This might be useful for finding really which makes
> the parser error. (It was a bit tricky though :D)

Good stuff, maybe "unexpected string 'foo' (T_STRING)" or "unexpected
string (T_STRING) 'foo'" would be more clear, but no big deal really.

Cheers

--
Jordi Boggiano
@seldaek - http://nelm.io/jordi

Paul Dragoonis

15 years ago

On Thu, Jun 23, 2011 at 7:16 PM, Jordi Boggiano <j.boggiano@seld.be> wrote:

> Thanks for the great work. More should be done on the front of helping
> newcomers solve trivial issues imo.
>
> On 23.06.2011 20:08, Felipe Pena wrote:
> > $ sapi/cli/php -r 'class '
> > Current:
> > Parse error: syntax error, unexpected $end, expecting T_STRING
> >
> > Patched:
> > Parse error: syntax error, unexpected end of file, expecting
> > 'identifier' (T_STRING)
>
> identifier should not be quoted. The quotes are fine for "expecting '{'"
> and "unexpected 'foo'" because those are text literals, but this implies
> you were supposed to actually type "class identifier", which is
> obviously not the intended message.
>
> > $ sapi/cli/php -r 'class abc foo'
> > Current:
> > Parse error: syntax error, unexpected T_STRING, expecting '{' in
> > Command line code on line 1
> >
> > Patched:
> > Parse error: syntax error, unexpected 'foo' (T_STRING), expecting '{'
> > in Command line code on line 1
> >
> >
> > As can be noticed, I added the actual scanned string in the
> > "unexpected" part. This might be useful for finding really which makes
> > the parser error. (It was a bit tricky though :D)
>
> Good stuff, maybe "unexpected string 'foo' (T_STRING)" or "unexpected
> string (T_STRING) 'foo'" would be more clear, but no big deal really.
>
> Cheers
>
> --
> Jordi Boggiano
> @seldaek - http://nelm.io/jordi
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

Good work Felipe! the $end was very ambiguous :-)

Stas Malyshev

15 years ago

Hi!

On 6/23/11 11:08 AM, Felipe Pena wrote:

> As can be noticed, I added the actual scanned string in the
> "unexpected" part. This might be useful for finding really which makes
> the parser error. (It was a bit tricky though :D)
>
>
> Any thoughts?
> https://wiki.php.net/rfc/improved-parser-error-message

Looks cool. Any tests we'd need to fix for that? Except for that, I
think it's all good.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Sebastian Bergmann

15 years ago

On 06/23/2011 08:08 PM, Felipe Pena wrote:

> Patched:
> Parse error: syntax error, unexpected end of file, expecting
> 'identifier' (T_STRING)

At least for developers who do not know how a compiler works, I think
that we should think about removing the information duplication of
"parse error" and "syntax error". Just "Syntax error: ..." should be
enough.

--
Sebastian Bergmann                    Co-Founder and Principal Consultant
http://sebastian-bergmann.de/                           http://thePHP.cc/