Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix T_*_CAST syntax issues that caused argument list edge cases #1667

Closed
wants to merge 5 commits into from

Conversation

marcioAlmada
Copy link
Contributor

@marcioAlmada marcioAlmada commented Dec 8, 2015

Second, and more complete, attempt after #1629. Example of edge case: https://3v4l.org/eE5bq

<?php

const INT = 1;

function foo(){};

foo(INT);

Parse error: syntax error, unexpected '(INT)' (int) (T_INT_CAST) in /in/eE5bq on line 7

Relates to #1633, in some way

@marcioAlmada
Copy link
Contributor Author

Hi! @nikic, @TazeTSchnitzel do you think this fix is doable?

@hikari-no-yume
Copy link
Contributor

This issue had been bothering me for a long time. While I'd rather we just drop the cast tokens entirely and make the type names reserved words, we probably can't do that any time soon. This at least fixes the var_dump(int); thing, so I'm in favour of it.

@krakjoe
Copy link
Member

krakjoe commented Nov 13, 2016

@marcioAlmada there are conflicts, and this should target 7.0 ... if it will rebase clean then please just fix the conflicts ;)

ping me directly when you have done it ...

ta

@nikic
Copy link
Member

nikic commented Nov 13, 2016

@krakjoe As this is a major change to lexer output, I don't think it can target a patch release. For example, it will break PHP-Parser.

@krakjoe
Copy link
Member

krakjoe commented Nov 13, 2016

okay then

@marcioAlmada make that 7.1 ...

"object"|
"unset"
){TABS_AND_SPACES}")" {
zend_copy_value(zendlval, (yytext+1), (yyleng-2));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't take into account possible whitespace around the cast name.


zend_ast_destroy(cast_ast);

return zend_ast_create_ex(ZEND_AST_CAST, cast_type, op0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be in a .c file.

@nikic
Copy link
Member

nikic commented Nov 13, 2016

Not sure about this change in general. For example, doesn't the same issue also apply to a number of other places using parenthesized expressions? This PR would allow writing call(INT), but something like if (INT) {} would still fail.

I remember there is some reason for it, but doesn't come immediately to mind. Why is it that we handle this in the lexer instead of parsing "(" T_STRING ")" (where T_STRING can also be T_ARRAY and T_UNSET, of course)?

@nikic
Copy link
Member

nikic commented Nov 13, 2016

I guess the reason is that we'd have a shift/reduce conflict on '(' expression ')' vs. '(' T_STRING ')' expression.

@hikari-no-yume
Copy link
Contributor

Ideally we'd reserve these words properly and then add '(' T_INT ')' etc to the parser. But I was cautious with my scalar types RFC about minimising BC impact, so I didn't do this. Alas.

@marcioAlmada
Copy link
Contributor Author

marcioAlmada commented Nov 13, 2016

@krakjoe this PR was created because of the callable types RFC, that if passed would have made it mandatory to fix the following edge case:

function x(callable(int):int $callback) {
#                  ^^^^^
}

But the callable types RFC was rejected. Considering important tools like PHP-Parser will break, this is the kind of technical debt we can live with until the next major release IMMO, specially if it doesn't get in the way of some important language feature until there.

I'd suggest to close this PR for now. If there is any urge to address this issue it will be trivial to resume. I just don't feel very comfortable to deliver a BC break now "just" for the sake of correctness.

/pings @nikic @Andrea

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 466159a on marcioAlmada:cleanup/T_CAST into ** on php:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants