-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
new MyClass()->method() without parentheses #13029
new MyClass()->method() without parentheses #13029
Conversation
Idk. With parentheses it looks more logical and clear, like in math, first is done what’s enclosed in parentheses and later the rest. |
Please turn this into an RFC! I hate the fact that I hace to put parentheses after a new. |
Syntax changes most certainly need to go through the RFC process, due to the impact on IDEs and static analysis tools that need to learn about the new syntax. |
Zend/tests/new_obj_access_without_parentheses/new_anonymous_static_method.phpt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that to me any many others, the current syntax looks visually cleaner and is less ambiguous. However, that is probably due to the fact that we are all so used to having to add parentheses.
Instead of asking "Do I like this syntax?", the better question to ask might instead be "Do I wish that this is how the syntax had always been?".
When I was new to PHP, I distinctly remember having issues with this and being confused why I needed to put the class construction in parentheses just to chain methods on it. As a PHP beginner, I expected things to work like this change proposes, so I think that this change would indeed be beneficial to newcomers.
The question then remains, are the rest of us who have been using the old syntax for so long ready for this change? That is a question I alone cannot answer, and why this definitely needs an official RFC proposal.
Furthermore, we must also consider the potential for ambiguity that could arise from the proposed syntax. Even if the implementation solves for the ambiguity (thread), there is another factor to consider, one that is harder to solve for using code: how do developers mentally evaluate if new Something()
refers to a function or an object?
+1 I experienced the same when I started learning PHP, and I believe many others did. Therefore, this change could make life easier for newcomers. |
Also the examples doesn't show the case where () are missing from classes :
For what my opinion is worth : I don't think changing this could be easier on long terms. Maybe for new comers ot the php world. But at some point we all have to maintain code and this can heavily complexify things IMO. |
These are my overall concerns as well. Using |
Without parentheses PHP behaves in the same way it currently does: throws a syntax error. I will elaborate that in the RFC soon. |
I personally don't think that makes sense. |
How are you going to treat the ambiguity? class Test
{
public function method(): string
{
return DateTime::class;
}
}
$result = new Test()->method();
// Is it this where this results in $result being a string "DateTime"
$result = (new Object())->method();
// or is it going to be this, where $result ends up being a new DateTime object?
$result = (new Object()->method()); |
@caendesilva , you won't be able to do That's because PHP supports dynamic classes in new expressions. If you have |
@psihius, there's no ambiguity in your example if I get it right.
Also note that the bison grammar changes compiled without errors or notices, which proves that it is unambiguous. |
I'm inclined to agree. However, do we feel this way just because we are used to seeing the current implementation? It's hard take an objective look at something so subjective as code styles. |
@TimWolla , I will create an RFC by the end of the week, thank you! |
@vudaltsov Unless you already have wiki / RFC karma, I recommend requesting it early. There's a limited number of folks who can grant that karma and I would not be surprised if some of them are on vacation currently. |
Remember that even if the implementation is unambiguous, the way developers' brains mentally parses the code may still be ambiguous and unexpected. By the way, @vudaltsov, the reason for me making these types of comments is not to tear you down or to criticise the code and your hard work. It's my good faith effort to come with constructive criticism to find and resolve eventual pitfalls that could lead to this proposal being rejected. |
I do not like the fact that |
Note that GitHub is not the intended medium for non-technical discussions. Please use the internals mailing list instead. I realize there's no thread for this topic yet, the first step would be for @vudaltsov to start one. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Cool! I like it! |
the next simplification is |
The good thing about this proposal is that it does not force you to use the new syntax. The old syntax would still be perfectly valid. So you can choose the one that you prefer, instead of being forced to use the first one. +100 from me |
I think it would be a better idea to just add a default static constructor to all PHP classes that could be overrided if you want. Like this:
This would be clean, concise and avoids any ambiguity. |
This comment was marked as off-topic.
This comment was marked as off-topic.
…s is now a callable_variable
edbb91d
to
b95ecd8
Compare
b95ecd8
to
d8ef6d5
Compare
@nikic, thank you very much for your review! I've just fixed the implementation:
|
@olamedia, nothing has changed for invokables, see tests and RFC. Please, post a snippet that you consider prolematic. |
Zend/zend_language_parser.y
Outdated
@@ -1324,6 +1330,8 @@ function_call: | |||
{ $$ = zend_ast_create(ZEND_AST_STATIC_CALL, $1, $3, $4); } | |||
| variable_class_name T_PAAMAYIM_NEKUDOTAYIM member_name argument_list | |||
{ $$ = zend_ast_create(ZEND_AST_STATIC_CALL, $1, $3, $4); } | |||
| new_dereferenceable T_PAAMAYIM_NEKUDOTAYIM member_name argument_list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to list new_dereferenceable in quite this many places -- just having it in fully_dereferenceable and callable_expr should be enough. Something along these lines seems to work: https://gist.github.com/nikic/375e4a3a0c0adb7911ac75200c8f45c7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you, that's much cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I will merge this shortly, thank you for your RFC!
@vudaltsov Feel free to mark the RFC as implemented. I also noticed that you didn't announce the successful vote (https://externals.io/message/123293). It's not technically required by the RFC process (apparently, at least https://wiki.php.net/rfc/howto doesn't mention it), but it's almost universally done so that internals are aware of the result. |
Why is it closed as not merged? I know it landed, but why technically in Github it has invalid resolution? Is it a php-src convention or what? @vudaltsov congrats and thanks for your work 🙂. |
Because it was rebased and closed via |
In PHP-CS-Fixer we also enforce linear history, so we rebase and squash PRs, but these are marked as merged anyway (as we do this within Github, using its controls). Isn't it possible to achieve this for the php-src workflow? It just looks wrong... Disclaimer: I don't know the full process, so I'm probably missing something. I am just curious, because such details are important to me 😅. |
For the PR to appear as merged, you either need to merge/squash via GitHub UI, or you need to push to the branch again to make the commit hashes match. The former works fine, unless you have to make changes (like add an |
This seems to have broken one of Xdebug's tests, through a change in syntax error. Not normally an issue, but I think this new message is not better.
This script used to throw:
But now it throws:
Especially the |
@derickr This change looks reasonable to me, because |
Although
But this is certainly not the only place where the parser can make suggestions that end up not being valid. |
This PR allows to immediately access newly created object without parentheses.
This was requested and discussed several times, see:
Here's what you will be able to write after this change:
Parentheses still cannot be omitted around the new expression without constructor arguments' parentheses in non-anonymous classes, because in some cases this leads to an ambiguity:
For more details see the RFC: https://wiki.php.net/rfc/new_without_parentheses
TODO: