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

[RFC] Never parameters #18016

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DanielEScherzer
Copy link
Member

Copy link
Member

@iluuu1994 iluuu1994 left a 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.

if (decl->child[2] != NULL) {
zend_error_noreturn(
E_COMPILE_ERROR,
"never cannot be used as a parameter type for methods with implementations"
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member

@TimWolla TimWolla Mar 11, 2025

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

Copy link
Member Author

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 ...

@Sophist-UK
Copy link

Sophist-UK commented Mar 11, 2025

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:

never vs. nothing

I am far from being a PHP expert, so I may have misunderstood this, but I am unclear what the difference is between public static function from(never $value): static; and public static function from($value): static;. I think an explanation would be beneficial in the RFC.

Fixing existing BackedEnum

It seems to me that the current implementation actually breaks the entire concept of Interfaces. When I use an Interface as a parameter or type, I am expecting this to define exactly what I can expect to pass to that interface without getting run-time errors. The current BackedEnum definition actually breaks this concept because I should be allowed to pass either a string or int to the from or tryFrom methods and NOT get any error from tryFrom and at worst get the same error with from as I would get if I passed an invalid value of the right type.

Surely the first thing to do would be to fix the current implementation so that #12863 is addressed and that actualised BackedEnums actually match the existing interface definition.

More general narrowing

Why couldn't we provide a narrows alternative to extends so that you can do the following to the existing interface definition for BackedEnums:

interface IntBackedEnum narrows BackedEnum {
  public string $value;
 
  public static function from(int $value): static;
  public static function tryFrom(int $value): ?static;
}

IntBackedEnum would have to include (perhaps inherited if not overridden) all the existing definitions of BackedEnum but would NOT be considered a sub-interface i.e. if you have a parameter that requires a BackedEnum then an IntBackedEnum for that parameter would be an invalid type.

Obviously classes can both narrow rather than extend other classes, in the same way. So considering an existing language construct class ClassA extends ClassB implements InterfaceC, InterfaceD {}, you might want to do something like class ClassA narrows ClassB implements InterfaceC implements-narrowing InterfaceD {}; In the original example, ClassA would type-match ClassB, InterfaceC and InterfaceD, and in the 2nd it would type-match only InterfaceC. But the general rules about methods defined in InterfaceC and InterfaceD needing to be implemented would still apply subject only to the type restrictions being relaxed to allow a choice of alternative types.

@DanielEScherzer
Copy link
Member Author

Apologies for commenting here, but I couldn't work out how to comment on the base RFC.

See https://www.php.net/mailing-lists.php

never vs. nothing

When no type is included (e.g. public static function from($value): static;) then any value is valid from a type perspective; when never is used, then no value is valid from a type perspective

Fixing existing BackedEnum

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

More general narrowing

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

@Sophist-UK
Copy link

never vs. nothing

When no type is included (e.g. public static function from($value): static;) then any value is valid from a type perspective; when never is used, then no value is valid from a type perspective.

I am not sure that I understand this. So what definitions of from can be made in a class that implements public static function from($value): static;? If as you say no value is valid from a type perspective, wouldn't that make the method uncallable?

Perhaps I don't actually understand what public static function from(int|string $value): static; actually means - or more likely that when you define an object with this, you are saying that the specific object implementation will accept and handle either class, but when it is used in an interface or abstract method it is saying that you can define an actual implementation which can be either int $value or string $value or int|string value (or since it is semantically equivalent) string|int $value.

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...

Fixing existing BackedEnum

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

Yes, exactly!!! The original specification was wrong because it breaks the fundamental concept of interface inheritance specifically that BackedEnum::from will always accept either an int or string as a parameter!! The only way to fix this is as #12863 originally suggested and make int BackedEnums accept strings as parameters to from and tryFrom and simply treat them the same as integer values that are not part of the enumeration.

More general narrowing

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.

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:

  1. Doesn't break interface inheritance because instead it provides something akin to inheritance without actually creating any parent relationship.

  2. Allow you to narrow the types allowed in an interface or object to a subset of the original without creating any architectural glitches or paradoxes.

@TimWolla TimWolla added the RFC label Mar 11, 2025
@DanielEScherzer
Copy link
Member Author

never vs. nothing

When no type is included (e.g. public static function from($value): static;) then any value is valid from a type perspective; when never is used, then no value is valid from a type perspective.

I am not sure that I understand this. So what definitions of from can be made in a class that implements public static function from($value): static;? If as you say no value is valid from a type perspective, wouldn't that make the method uncallable?

Perhaps I don't actually understand what public static function from(int|string $value): static; actually means - or more likely that when you define an object with this, you are saying that the specific object implementation will accept and handle either class, but when it is used in an interface or abstract method it is saying that you can define an actual implementation which can be either int $value or string $value or int|string value (or since it is semantically equivalent) string|int $value.

And... if I don't understand this then perhaps everything else I said is pure bunkum and should be ignored completely.

I think, respectfully, that you do not understand LSP inheritance rules. https://php.watch/articles/php-lsp may be useful - public static function from(int|string $value): static; means that all implementations/subclasses need to accept both ints and strings.

Yes, exactly!!! The original specification was wrong because it breaks the fundamental concept of interface inheritance specifically that BackedEnum::from will always accept either an int or string as a parameter!! The only way to fix this is as #12863 originally suggested and make int BackedEnums accept strings as parameters to from and tryFrom and simply treat them the same as integer values that are not part of the enumeration.

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.

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?

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)

@TimWolla
Copy link
Member

Regarding the previous discussion of interfaces and LSP: Frankly static methods have no business of belonging in an interface anyway, since they cannot be substituted anyways: You are not dealing with an existing object and PHP doesn't have class types that can be used in a type declaration.

@Sophist-UK
Copy link

Sophist-UK commented Mar 12, 2025

I think, respectfully, that you do not understand LSP inheritance rules. https://php.watch/articles/php-lsp may be useful - public static function from(int|string $value): static; means that all implementations/subclasses need to accept both ints and strings.

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.

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.

Ah - I see now. function type(never $value): static; is actually the polar opposite of function type($value): static;. never can be widened by any other type whereas specifying no type is already infinitely wide and you cannot implement with a specific class because that would narrow it.

Yes, exactly!!! The original specification was wrong because it breaks the fundamental concept of interface inheritance specifically that BackedEnum::from will always accept either an int or string as a parameter!! The only way to fix this is as #12863 originally suggested and make int BackedEnums accept strings as parameters to from and tryFrom and simply treat them the same as integer values that are not part of the enumeration.

From the LSP article it seems that my understanding was pretty much exactly how the php.watch article suggested:

image

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 from and returning null instead of throwing an exception for tryFrom.

And, if I now understand it correctly, this RFC/PR is proposing an alternative way to fix the architectural anomaly using never to change the underlying interface definition. This will have a smaller backwards incompatibility because only reflection value will change.

And of course, the never solution would have wider architectural applicability than just BackedEnums.

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 function type(float $value): static; or indeed function type(myClass $value): static;.

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 function myThing(BackedEnum $x); then you have to assume that when you get an implemented BackedEnum as $x then $x->from($y) might need $y to be an array or a indeed any specific type of object - so it seems to me that you are now introducing a significant backwards compatibility issue because previously you could be certain that it would need either an integer or a string and do something like:

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 never, the specific BackedEnum example and indeed other generic use cases need a different solution with an interface of union-type parameters that can be narrowed. In which case shouldn't a syntax like function type(int&string $value): static; or function type(int||string $value): static; be allowed in interface definitions and abstract function definitions allowing implementations to then choose int, string, int|string, string|int as types but not float or array or sub-classes of int or string?

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)

I no longer think my narrows proposal has merit - this PR is a minor architectural tweak whereas narrows would be a major architectural change that doesn't AFAIK have any analogies in either OO theory or other OO languages.

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
Copy link
Member

@iluuu1994 iluuu1994 left a 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"
Copy link
Member

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.

Copy link
Member Author

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

?

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

Successfully merging this pull request may close these issues.

Reflection parameter type could be more precise about specific BackedEnum::from() and tryFrom() type
4 participants