[DISCUSSION] ReflectionType::accepts

  113016
January 29, 2021 08:14 brendt@stitcher.io (Brent Roose)
Hi Internals

Since the addition of union types, it has become a little more cumbersome to determine whether a given parameter type accepts specific input. Personally I've been reusing this code blob in several places because of it:

```
/** @var \ReflectionNamedType[] $types */
$types = match ($type::class) {
    ReflectionUnionType::class => $type->getTypes(),
    ReflectionNamedType::class => [$type],
};

foreach ($types as $type) {
    if (! is_subclass_of($type->getName(), ShouldBeStored::class)) {
        continue;
    }

    // …
}
```

I wonder whether we would discuss adding a method on ReflectionType that accepts any given input, and tells you whether that input is valid for that type or not. I was thinking about `ReflectionType::accepts(string|object $input): bool` but we could discuss another name. With it, the above example could be refactored like so:

```
if (! $type->accepts(ShouldBeStored::class)) {
    return;
}

// …
```

I believe this method should accept both class names and objects to be consistent with the existing reflection API. It would also work with intersection types, if they are to be added some day.

What do you think?

Kind regards
Brent
  113017
January 29, 2021 08:18 pierre-php@processus.org ("Pierre R.")
Le 29/01/2021 à 09:14, Brent Roose a écrit :
> Hi Internals > > Since the addition of union types, it has become a little more cumbersome to determine whether a given parameter type accepts specific input. Personally I've been reusing this code blob in several places because of it: > > ``` > /** @var \ReflectionNamedType[] $types */ > $types = match ($type::class) { > ReflectionUnionType::class => $type->getTypes(), > ReflectionNamedType::class => [$type], > }; > > foreach ($types as $type) { > if (! is_subclass_of($type->getName(), ShouldBeStored::class)) { > continue; > } > > // … > } > ``` > > I wonder whether we would discuss adding a method on ReflectionType that accepts any given input, and tells you whether that input is valid for that type or not. I was thinking about `ReflectionType::accepts(string|object $input): bool` but we could discuss another name. With it, the above example could be refactored like so: > > ``` > if (! $type->accepts(ShouldBeStored::class)) { > return; > } > > // … > ``` > > I believe this method should accept both class names and objects to be consistent with the existing reflection API. It would also work with intersection types, if they are to be added some day. > > What do you think? > > Kind regards > Brent
Hello, I think this would be a good addition. I personally had to write the exact same algorithm to solve this exact same problem yesterday night. Cheers, -- Pierre
  113018
January 29, 2021 08:19 sebastian@php.net (Sebastian Bergmann)
Am 29.01.2021 um 09:18 schrieb Pierre R.:
> I think this would be a good addition. I personally had to write the exact > same algorithm to solve this exact same problem yesterday night.
https://github.com/sebastianbergmann/type is the implementation that PHPUnit uses.
  113019
January 29, 2021 09:33 nikita.ppv@gmail.com (Nikita Popov)
On Fri, Jan 29, 2021 at 9:15 AM Brent Roose <brendt@stitcher.io> wrote:

> Hi Internals > > Since the addition of union types, it has become a little more cumbersome > to determine whether a given parameter type accepts specific input. > Personally I've been reusing this code blob in several places because of it: > > ``` > /** @var \ReflectionNamedType[] $types */ > $types = match ($type::class) { > ReflectionUnionType::class => $type->getTypes(), > ReflectionNamedType::class => [$type], > }; > > foreach ($types as $type) { > if (! is_subclass_of($type->getName(), ShouldBeStored::class)) { > continue; > } > > // … > } > ``` > > I wonder whether we would discuss adding a method on ReflectionType that > accepts any given input, and tells you whether that input is valid for that > type or not. I was thinking about `ReflectionType::accepts(string|object > $input): bool` but we could discuss another name. With it, the above > example could be refactored like so: > > ``` > if (! $type->accepts(ShouldBeStored::class)) { > return; > } > > // … > ``` > > I believe this method should accept both class names and objects to be > consistent with the existing reflection API. It would also work with > intersection types, if they are to be added some day. > > What do you think? > > Kind regards > Brent >
There's two possible notions of what "accepts" does: * acceptsValue(), whether a given value satisfies the type. Probably needs a flag to determine whether strict_types semantics should be used or not. * isSubTypeOf(), whether a given type satisfies the type in a subtyping relationship. You seem to be mixing these two up into one concept, even though they are quite different. Which is the one you actually need? (I am much more open to providing the former than the latter.) Nikita
  113020
January 29, 2021 10:16 brendt@stitcher.io (Brent Roose)
Hi Nikita

I was indeed thinking about the former: acceptsValue; but I would excpect that if the concrete value is a subtype of the current type, the fuction would also return true. What I'm basically interested in: will this method throw a type error when I pass this value? In other words: "will this method accept this value?"

Is that in line with your view on `acceptsValue`? 

Kind regards
Brent

> On 29 Jan 2021, at 10:33, Nikita Popov ppv@gmail.com> wrote: > > On Fri, Jan 29, 2021 at 9:15 AM Brent Roose <brendt@stitcher.io <mailto:brendt@stitcher.io>> wrote: > Hi Internals > > Since the addition of union types, it has become a little more cumbersome to determine whether a given parameter type accepts specific input. Personally I've been reusing this code blob in several places because of it: > > ``` > /** @var \ReflectionNamedType[] $types */ > $types = match ($type::class) { > ReflectionUnionType::class => $type->getTypes(), > ReflectionNamedType::class => [$type], > }; > > foreach ($types as $type) { > if (! is_subclass_of($type->getName(), ShouldBeStored::class)) { > continue; > } > > // … > } > ``` > > I wonder whether we would discuss adding a method on ReflectionType that accepts any given input, and tells you whether that input is valid for that type or not. I was thinking about `ReflectionType::accepts(string|object $input): bool` but we could discuss another name. With it, the above example could be refactored like so: > > ``` > if (! $type->accepts(ShouldBeStored::class)) { > return; > } > > // … > ``` > > I believe this method should accept both class names and objects to be consistent with the existing reflection API. It would also work with intersection types, if they are to be added some day. > > What do you think? > > Kind regards > Brent > > There's two possible notions of what "accepts" does: > > * acceptsValue(), whether a given value satisfies the type. Probably needs a flag to determine whether strict_types semantics should be used or not. > > * isSubTypeOf(), whether a given type satisfies the type in a subtyping relationship. > > You seem to be mixing these two up into one concept, even though they are quite different. Which is the one you actually need? (I am much more open to providing the former than the latter.) > > Nikita
  115911
September 1, 2021 08:56 nikita.ppv@gmail.com (Nikita Popov)
On Fri, Jan 29, 2021 at 11:16 AM Brent Roose <brendt@stitcher.io> wrote:

> Hi Nikita > > I was indeed thinking about the former: acceptsValue; but I would excpect > that if the concrete value is a subtype of the current type, the fuction > would also return true. What I'm basically interested in: will this method > throw a type error when I pass this value? In other words: "will this > method accept this value?" > > Is that in line with your view on `acceptsValue`? > > Kind regards > Brent >
I just took a quick look at this, and apart from the strict_types distinction, another annoying edge case here is that our type checks aren't side-effect free. As of PHP 8.1, we have at least the deprecation warning for precision loss in weak float -> int casts, and more generally, arbitrary side-effects during __toString() calls on objects. So just performing a normal type check may not be suitable for this use case. Regards, Nikita
> On 29 Jan 2021, at 10:33, Nikita Popov ppv@gmail.com> wrote: > > On Fri, Jan 29, 2021 at 9:15 AM Brent Roose <brendt@stitcher.io> wrote: > >> Hi Internals >> >> Since the addition of union types, it has become a little more cumbersome >> to determine whether a given parameter type accepts specific input. >> Personally I've been reusing this code blob in several places because of it: >> >> ``` >> /** @var \ReflectionNamedType[] $types */ >> $types = match ($type::class) { >> ReflectionUnionType::class => $type->getTypes(), >> ReflectionNamedType::class => [$type], >> }; >> >> foreach ($types as $type) { >> if (! is_subclass_of($type->getName(), ShouldBeStored::class)) { >> continue; >> } >> >> // … >> } >> ``` >> >> I wonder whether we would discuss adding a method on ReflectionType that >> accepts any given input, and tells you whether that input is valid for that >> type or not. I was thinking about `ReflectionType::accepts(string|object >> $input): bool` but we could discuss another name. With it, the above >> example could be refactored like so: >> >> ``` >> if (! $type->accepts(ShouldBeStored::class)) { >> return; >> } >> >> // … >> ``` >> >> I believe this method should accept both class names and objects to be >> consistent with the existing reflection API. It would also work with >> intersection types, if they are to be added some day. >> >> What do you think? >> >> Kind regards >> Brent >> > > There's two possible notions of what "accepts" does: > > * acceptsValue(), whether a given value satisfies the type. Probably needs > a flag to determine whether strict_types semantics should be used or not. > > * isSubTypeOf(), whether a given type satisfies the type in a subtyping > relationship. > > You seem to be mixing these two up into one concept, even though they are > quite different. Which is the one you actually need? (I am much more open > to providing the former than the latter.) > > Nikita > > >