-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
[RFC] Never parameters #18016
base: master
Are you sure you want to change the base?
[RFC] Never parameters #18016
Conversation
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.
Looks reasonable in general.
Zend/zend_compile.c
Outdated
if (decl->child[2] != NULL) { | ||
zend_error_noreturn( | ||
E_COMPILE_ERROR, | ||
"never cannot be used as a parameter type for methods with implementations" |
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.
Maybe this should say non-abstract? I consider interface methods abstract, but that's a bit philosophical.
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 consider interface methods abstract
I would probably find it confusing if "abstract methods" included methods that were not flagged with the abstract
keyword, but I agree it is probably philosophical - anyone else have thoughts on way or the other?
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.
FWIW, reflection does mark them as abstract. https://3v4l.org/GfGLK
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.
When attempting to implement a method that is abstract, the error message talks about the "body":
Fatal error: Interface function Foo::foo() cannot contain body in php-wasm run script on line 4
Fatal error: Abstract function Foo::foo() cannot contain body in php-wasm run script on line 4
So perhaps:
Function Foo::foo() containing a body may not use never as a parameter type
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.
Switched it to Fatal error: Function Demo::example() containing a body cannot use never as a parameter type in ...
Apologies for commenting here, but I couldn't work out how to comment on the base RFC. As others have commented on the actual RFC, this proposal allows widening to types that were never intended, and that associated types might be a more rigorous approach. To the above I would add the following additional points:
|
See https://www.php.net/mailing-lists.php
When no type is included (e.g.
As discussed on #12863 the use of type errors for the wrong types is part of the original specification of enums, this just aligns reflection information to the existing reality
That is something that is wildly different from anything I've seen before in any programming language I've used - while it could certainly be discussed, it should be discussed on the mailing list in a new thread, rather than here |
I am not sure that I understand this. So what definitions of Perhaps I don't actually understand what And... if I don't understand this then perhaps everything else I said is pure bunkum and should be ignored completely. But just in case it isn't bunkum...
Yes, exactly!!! The original specification was wrong because it breaks the fundamental concept of
As I said, I don't feel expert enough to argue this case in its own thread. I am simply asking you (as someone who clearly is expert enough) what you think? Have I got it completely wrong or are there merits, and if so then the benefits of my approach is that it perhaps:
|
I think, respectfully, that you do not understand LSP inheritance rules. https://php.watch/articles/php-lsp may be useful -
You can propose that separately - my fix is to remove the expectation that "BackedEnum::from will always accept either an int or string as a parameter" by instead declaring that "BackedEnum::from accepts NOTHING as a parameter" and then individual enums can expand to accept ints or strings.
I have to disagree with the assessment that I am expert enough, and regardless, this isn't the right place to discuss it. If you would like me to think about this more, please send me an email (daniel.e.scherzer at gmail.com) |
Regarding the previous discussion of interfaces and LSP: Frankly |
Don't worry, I am not insulted whether you state this respectfully or not. One thing I am honest enough about (with myself and others) is to admit when I don't know or am not sure or was wrong.
Ah - I see now.
From the LSP article it seems that my understanding was pretty much exactly how the php.watch article suggested: and the BackedEnum definition breaks this rule because BackedEnum is a sub-class of Enum yet the parameters of its methods are actually narrower rather than wider as required by the LSP. This seems to be a shared understanding. One way of fixing this architectural anomaly is to fix the BackedEnum implementation so that an int BackedEnum accepts string parameters - but this would be an implementation change that could create backwards compatibility issues with existing code by throwing a different exception for And, if I now understand it correctly, this RFC/PR is proposing an alternative way to fix the architectural anomaly using And of course, the However, whilst I do see the generic need for this, and it would indeed solve the architectural anomaly for the BackedEnum example, it would then allow me to define my own sub-class of BackedEnum with Maybe this is a good idea - maybe it would be beneficial to have Enums that returned a float e.g. enum Irrationals: float {
case Pi = 3.1415926535897932385;
case e = 2.7182818284590452354;
} or an array: enum Irrationals: array {
case Weekdays = ['Monday', 'Tuesday', 'Wednesday', 'Thursday' 'Friday'];
case Weekend = ['Saturday', 'Sunday'];
} But from a developers perspective, if you code try {
return $x->from(1);
} catch (\Exception $e) {
return $x->from('a');
} but now this is no longer sufficient. So whilst there is I think a generic use case for
I no longer think my But I am continuing this here because it seems to be relevant to the RFC and / or this PR. |
* Remove special messages for `never` parameters with default values or on property hooks * Reword error message for method with a body * Simplify some tests * Reduplicate registration of backed enum methods
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.
Looks very simple and concise now, thanks!
if (op_array->scope == NULL) { | ||
zend_error_noreturn( | ||
E_COMPILE_ERROR, | ||
"never cannot be used as a parameter type for functions" |
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 message can/should probably use the same wording as line 7719, because global functions naturally contain a body, no need to call them out specifically.
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.
Exactly because global functions naturally contain a body, I figured that an error message linking the problem to the having of a body would be less helpful than one explaining that non-method functions cannot every use never
parameters. I dropped the more specific error messages for property hooks and for parameters with defaults, given that both messages are generated right next to each other any extra overhead of tracking them both should be pretty minimal.
How about
Non-method foo() cannot use never as a parameter type
?
https://wiki.php.net/rfc/never-parameters-v2