Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] instanceof survives non-object variables, but crashes on non-object constants.

Posted by Andreas Hennings 
The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:

$x = 1;
$x instanceof \stdClass;


The following (https://3v4l.org/IdSBu) gives a fatal error:

1 instanceof \stdclass;

t think this behavior is inconsistent, and we should consider changing it.

There are two options, but only one is BC.

- Let 1 instanceof \stdClass return false, instead of crashing. -> seems BC
- Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.

So it seems the first would the option we should take.
This is also what hhvm does, according to https://3v4l.org/IdSBu.
Hi

We should just add a warning to the first example, it seems like an
oversight that it was left silent

On 9 Dec 2017 07.29, "Andreas Hennings" <[email protected]> wrote:

> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>
> $x = 1;
> $x instanceof \stdClass;
>
>
> The following (https://3v4l.org/IdSBu) gives a fatal error:
>
> 1 instanceof \stdclass;
>
> t think this behavior is inconsistent, and we should consider changing it.
>
> There are two options, but only one is BC.
>
> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems BC
> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>
> So it seems the first would the option we should take.
> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>
Adding a warning to something that used to be ok would break existing code.
I remember a number of cases where I used instanceof on possibly
non-objects.

if ($x instanceof C) {
return $x;
}
elseif ($x === NULL) {
...
}

All such code would then produce warnings.


On 9 December 2017 at 07:35, Kalle Sommer Nielsen <[email protected]> wrote:

> Hi
>
> We should just add a warning to the first example, it seems like an
> oversight that it was left silent
>
> On 9 Dec 2017 07.29, "Andreas Hennings" <[email protected]> wrote:
>
>> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>>
>> $x = 1;
>> $x instanceof \stdClass;
>>
>>
>> The following (https://3v4l.org/IdSBu) gives a fatal error:
>>
>> 1 instanceof \stdclass;
>>
>> t think this behavior is inconsistent, and we should consider changing it.
>>
>> There are two options, but only one is BC.
>>
>> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems
>> BC
>> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>>
>> So it seems the first would the option we should take.
>> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>>
>
Hi

That is fine for code that is broken in the first place. Similarly we added
a warning some years back about array to string conversions.

If your code is like the example, it should be updated and warned about as
you are doing something illogical by first checking the type and or value
of the operand you are passing to instanceof after.

The impact should be minimal as is, so persevering bc for broken usage is a
poor argument imo



On 9 Dec 2017 07.38, "Andreas Hennings" <[email protected]> wrote:

> Adding a warning to something that used to be ok would break existing code.
> I remember a number of cases where I used instanceof on possibly
> non-objects.
>
> if ($x instanceof C) {
> return $x;
> }
> elseif ($x === NULL) {
> ...
> }
>
> All such code would then produce warnings.
>
>
> On 9 December 2017 at 07:35, Kalle Sommer Nielsen <[email protected]> wrote:
>
>> Hi
>>
>> We should just add a warning to the first example, it seems like an
>> oversight that it was left silent
>>
>> On 9 Dec 2017 07.29 <20%2017%2007%2029>, "Andreas Hennings" <
>> [email protected]> wrote:
>>
>>> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>>>
>>> $x = 1;
>>> $x instanceof \stdClass;
>>>
>>>
>>> The following (https://3v4l.org/IdSBu) gives a fatal error:
>>>
>>> 1 instanceof \stdclass;
>>>
>>> t think this behavior is inconsistent, and we should consider changing
>>> it.
>>>
>>> There are two options, but only one is BC.
>>>
>>> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems
>>> BC
>>> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>>>
>>> So it seems the first would the option we should take.
>>> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>>>
>>
>
Why is the usage broken? It works :)
In practice, the current version of instanceof has an is_object() built-in.
This is what people could assume from trying.

Of course I don't know how it is implemented internally. Maybe a kitten
dies every time I use instanceof on non-object variables?

If we need an additional is_object() it can make some code more expensive..




On 9 December 2017 at 07:46, Kalle Sommer Nielsen <[email protected]> wrote:

> Hi
>
> That is fine for code that is broken in the first place. Similarly we
> added a warning some years back about array to string conversions.
>
> If your code is like the example, it should be updated and warned about as
> you are doing something illogical by first checking the type and or value
> of the operand you are passing to instanceof after.
>
> The impact should be minimal as is, so persevering bc for broken usage is
> a poor argument imo
>
>
>
> On 9 Dec 2017 07.38, "Andreas Hennings" <[email protected]> wrote:
>
>> Adding a warning to something that used to be ok would break existing
>> code.
>> I remember a number of cases where I used instanceof on possibly
>> non-objects.
>>
>> if ($x instanceof C) {
>> return $x;
>> }
>> elseif ($x === NULL) {
>> ...
>> }
>>
>> All such code would then produce warnings.
>>
>>
>> On 9 December 2017 at 07:35, Kalle Sommer Nielsen <[email protected]> wrote:
>>
>>> Hi
>>>
>>> We should just add a warning to the first example, it seems like an
>>> oversight that it was left silent
>>>
>>> On 9 Dec 2017 07.29 <20%2017%2007%2029>, "Andreas Hennings" <
>>> [email protected]> wrote:
>>>
>>>> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>>>>
>>>> $x = 1;
>>>> $x instanceof \stdClass;
>>>>
>>>>
>>>> The following (https://3v4l.org/IdSBu) gives a fatal error:
>>>>
>>>> 1 instanceof \stdclass;
>>>>
>>>> t think this behavior is inconsistent, and we should consider changing
>>>> it.
>>>>
>>>> There are two options, but only one is BC.
>>>>
>>>> - Let 1 instanceof \stdClass return false, instead of crashing. ->
>>>> seems BC
>>>> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC
>>>> break.
>>>>
>>>> So it seems the first would the option we should take.
>>>> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>>>>
>>>
>>
The typical usage of `instanceof` is checking a `null|object` API that
built an object of a (presumably known) type. Adding a crash there seems
silly and overcomplicated.

On 9 Dec 2017 07:57, "Andreas Hennings" <[email protected]> wrote:

> Why is the usage broken? It works :)
> In practice, the current version of instanceof has an is_object() built-in.
> This is what people could assume from trying.
>
> Of course I don't know how it is implemented internally. Maybe a kitten
> dies every time I use instanceof on non-object variables?
>
> If we need an additional is_object() it can make some code more expensive..
>
>
>
>
> On 9 December 2017 at 07:46, Kalle Sommer Nielsen <[email protected]> wrote:
>
> > Hi
> >
> > That is fine for code that is broken in the first place. Similarly we
> > added a warning some years back about array to string conversions.
> >
> > If your code is like the example, it should be updated and warned about
> as
> > you are doing something illogical by first checking the type and or value
> > of the operand you are passing to instanceof after.
> >
> > The impact should be minimal as is, so persevering bc for broken usage is
> > a poor argument imo
> >
> >
> >
> > On 9 Dec 2017 07.38, "Andreas Hennings" <[email protected]> wrote:
> >
> >> Adding a warning to something that used to be ok would break existing
> >> code.
> >> I remember a number of cases where I used instanceof on possibly
> >> non-objects.
> >>
> >> if ($x instanceof C) {
> >> return $x;
> >> }
> >> elseif ($x === NULL) {
> >> ...
> >> }
> >>
> >> All such code would then produce warnings.
> >>
> >>
> >> On 9 December 2017 at 07:35, Kalle Sommer Nielsen <[email protected]>
> wrote:
> >>
> >>> Hi
> >>>
> >>> We should just add a warning to the first example, it seems like an
> >>> oversight that it was left silent
> >>>
> >>> On 9 Dec 2017 07.29 <20%2017%2007%2029>, "Andreas Hennings" <
> >>> [email protected]> wrote:
> >>>
> >>>> The following (https://3v4l.org/A2Tp6) is ok, it simply returns
> false:
> >>>>
> >>>> $x = 1;
> >>>> $x instanceof \stdClass;
> >>>>
> >>>>
> >>>> The following (https://3v4l.org/IdSBu) gives a fatal error:
> >>>>
> >>>> 1 instanceof \stdclass;
> >>>>
> >>>> t think this behavior is inconsistent, and we should consider changing
> >>>> it.
> >>>>
> >>>> There are two options, but only one is BC.
> >>>>
> >>>> - Let 1 instanceof \stdClass return false, instead of crashing. ->
> >>>> seems BC
> >>>> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC
> >>>> break.
> >>>>
> >>>> So it seems the first would the option we should take.
> >>>> This is also what hhvm does, according to https://3v4l.org/IdSBu.
> >>>>
> >>>
> >>
>
>
> That is fine for code that is broken in the first place. Similarly we added
> a warning some years back about array to string conversions.
>

Code using instanceof on possible non-objects isn't broken. instanceof
simply does an implicit is_object() check without needing an extra function
call.

I know that a change there would break Amp in quite a few places (
https://github.com/amphp/amp/search?utf8=%E2%9C%93&q=instanceof&type=) and
I'm very sure that a lot of other applications would break, too.


> The impact should be minimal as is, so persevering bc for broken usage is a
> poor argument imo
>

Why is it broken? What's wrong with the implicit is_object() check?

1 instanceof XXX *is clearly broken* on the other hand, which can be
statically verified to fail in all cases.

Regards, Niklas
I prefer the inconsistency between `$x instanceof \stdClass` and `1
instanceof \stdClass`, than adding a warning (or throwing) to `$x
instanceof \stdClass` when `$x = 1`.
I think `1 instanceof \stdClass === false` would be reasonable.


1. PHP has no means of locking a variable to a type (i.e. the variable
could be reassigned to a scalar at any point), so lack of an implicit
`is_object` check in `instanceof` would basically move to require an
explicit one in many cases it is used, if you want to be *sure* a warning
won't be thrown that is (baring in mind just using the variable earlier
might change its type non-visibly, since we don't have explicit caller
opt-in to pass-by-reference).

This to say that IMO `$x instanceof stdClass` when `$x = 1` perhaps
shouldn't be considered broken usage because it is useful for it to perform
this check. Also consider that `is_object($x) === false` implies `$x` is
not an instance of any object (and in-particular not of the object you
might ask about with `instanceof`), so there is no reason `instanceof`
wouldn't be able to do this check.

There is also a certain amount of irony about making a type-checking
operator start complaining when you give it the wrong type ;-)

2. There is already precedent for variables to act differently than
literals, and even constants in PHP when used in object operators, for
example
```
$x = new \stdClass;
const y = 'stdClass';
$y = y;

var_dump($x instanceof $x);
var_dump($x instanceof $y);
var_dump($x instanceof y);
var_dump($x instanceof 'stdClass');
var_dump($x instanceof new \stdClass);
var_dump($x instanceof (new \stdClass));
```

where the three lines return `false`, the third as (the const isn't
looked at, and no warning is thrown when checking against non-existent `y`
class). The last three lines throw errors.

Point here is that, "should variables be treated like their literal
values?" is perhaps a bigger question, to which the answer at present seems
to be "no" of object operators.

I'm not opposed to moving towards constancy, but I think there being
inconsistency isn't worth throwing in `$x instanceof \stdClass` when `$x =
1` (since inconsistency between variables and their values shouldn't be
unexpected when using object operators).

---
@Kalle

> We should just add a warning to the first example, it seems like an oversight
that it was left silent

&
> That is fine for code that is broken in the first place. Similarly we
added a warning some years back about array to string conversions.

As a data-point, usage with non-objects is documented behaviour
http://php.net/manual/en/language.operators.type.php#example-115 so would
this be spec change to say this is incorrect, as opposed to being a bugfix?

Kind regards,
Aidan

On 9 December 2017 at 06:28, Andreas Hennings <[email protected]> wrote:

> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>
> $x = 1;
> $x instanceof \stdClass;
>
>
> The following (https://3v4l.org/IdSBu) gives a fatal error:
>
> 1 instanceof \stdclass;
>
> t think this behavior is inconsistent, and we should consider changing it.
>
> There are two options, but only one is BC.
>
> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems BC
> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>
> So it seems the first would the option we should take.
> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>
> where the three lines return `false`, the third [...]

Oops, that should say "the first two lines return `true`, the third
`false`" (the point here being that they return something, as opposed to
the later three, which throw).

On 9 December 2017 at 15:13, Aidan Woods <[email protected]> wrote:

> I prefer the inconsistency between `$x instanceof \stdClass` and `1
> instanceof \stdClass`, than adding a warning (or throwing) to `$x
> instanceof \stdClass` when `$x = 1`.
> I think `1 instanceof \stdClass === false` would be reasonable.
>
>
> 1. PHP has no means of locking a variable to a type (i.e. the variable
> could be reassigned to a scalar at any point), so lack of an implicit
> `is_object` check in `instanceof` would basically move to require an
> explicit one in many cases it is used, if you want to be *sure* a warning
> won't be thrown that is (baring in mind just using the variable earlier
> might change its type non-visibly, since we don't have explicit caller
> opt-in to pass-by-reference).
>
> This to say that IMO `$x instanceof stdClass` when `$x = 1` perhaps
> shouldn't be considered broken usage because it is useful for it to perform
> this check. Also consider that `is_object($x) === false` implies `$x` is
> not an instance of any object (and in-particular not of the object you
> might ask about with `instanceof`), so there is no reason `instanceof`
> wouldn't be able to do this check.
>
> There is also a certain amount of irony about making a type-checking
> operator start complaining when you give it the wrong type ;-)
>
> 2. There is already precedent for variables to act differently than
> literals, and even constants in PHP when used in object operators, for
> example
> ```
> $x = new \stdClass;
> const y = 'stdClass';
> $y = y;
>
> var_dump($x instanceof $x);
> var_dump($x instanceof $y);
> var_dump($x instanceof y);
> var_dump($x instanceof 'stdClass');
> var_dump($x instanceof new \stdClass);
> var_dump($x instanceof (new \stdClass));
> ```
>
> where the three lines return `false`, the third as (the const isn't
> looked at, and no warning is thrown when checking against non-existent `y`
> class). The last three lines throw errors.
>
> Point here is that, "should variables be treated like their literal
> values?" is perhaps a bigger question, to which the answer at present seems
> to be "no" of object operators.
>
> I'm not opposed to moving towards constancy, but I think there being
> inconsistency isn't worth throwing in `$x instanceof \stdClass` when `$x =
> 1` (since inconsistency between variables and their values shouldn't be
> unexpected when using object operators).
>
> ---
> @Kalle
>
> > We should just add a warning to the first example, it seems like an oversight
> that it was left silent
>
> &
> > That is fine for code that is broken in the first place. Similarly we
> added a warning some years back about array to string conversions.
>
> As a data-point, usage with non-objects is documented behaviour
> http://php.net/manual/en/language.operators.type.php#example-115 so would
> this be spec change to say this is incorrect, as opposed to being a bugfix?
>
> Kind regards,
> Aidan
>
> On 9 December 2017 at 06:28, Andreas Hennings <[email protected]> wrote:
>
>> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>>
>> $x = 1;
>> $x instanceof \stdClass;
>>
>>
>> The following (https://3v4l.org/IdSBu) gives a fatal error:
>>
>> 1 instanceof \stdclass;
>>
>> t think this behavior is inconsistent, and we should consider changing it.
>>
>> There are two options, but only one is BC.
>>
>> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems
>> BC
>> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>>
>> So it seems the first would the option we should take.
>> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>>
>
>
On Fri, Dec 8, 2017 at 11:35 PM, Kalle Sommer Nielsen <[email protected]> wrote:
> We should just add a warning to the first example, it seems like an
> oversight that it was left silent

I strongly object! We ought to consider using `instanceof` to support
more type-checks than just classes. I think `1 instanceof int` really
ought to return true for both literals and if they were variables:

$lhs = 1;
$rhs = 'int';
var_export($lhs instanceof $rhs);

The fact we do not have a single, unified way to check if some value
on the LHS is of the type on the RHS really bothers me.

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 9 December 2017 at 16:19, Aidan Woods <[email protected]> wrote:

> > where the three lines return `false`, the third [...]
>
> Oops, that should say "the first two lines return `true`, the third
> `false`" (the point here being that they return something, as opposed to
> the later three, which throw).
>
>
This is interesting, I did not know that the right side is also
inconsistent :)

I think the default mental model of a programmer is that an operator or
function only sees the value of an expression, not where this value is
coming from.
There is already an exception to this for by-reference parameters, which
already feels weird e.g. for isset(null).

The inconsistency for the right side of instanceof may be justified, I
don't know. But definitely it is not good for shaping a clear mental model.

The "1 instanceof \stdClass" is nonsensical of course, but not any more
than if(true) or if (5 + 0 === 0) or unreachable code.
I would say the main use case is for a programmer to find out how
instanceof behaves on non-object values.
In this case the experiment will give the wrong answer.

It should be the IDE's job to tell the programmer if something is
nonsensical. The language should only care if it follows the rules, and
these should be as consistent as possible.

So, imo:
- we should let "1 instanceof \stdClass" return false without complaining.
- possible inconsistencies on the right side of instanceof can be discussed
separately.
- extension of instanceof to other types, e.g. "1 instanceof int" can be
discussed separately.

I think the simple change would gradually improve consistency, without
closing any doors on future changes.
Hi!

> t think this behavior is inconsistent, and we should consider changing it.
>
> There are two options, but only one is BC.
>
> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems BC
> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.

Neither of those "crashes", one of them returns fatal error, which is a
valid option for code that is obviously broken. It would also be a valid
option to return false, but since it is a valid outcome either way, I
don't think changing anything is necessary. It does not make any real
code easier to write (I can't see any legit reason to test numerics for
being instance of anything) and does not improve anything but abstract
"consistency", which in this case is not useful for anything.

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Sun, Dec 10, 2017 at 12:09 AM, Stanislav Malyshev <[email protected]>
wrote:

> Hi!
>
> > t think this behavior is inconsistent, and we should consider changing
> it.
> >
> > There are two options, but only one is BC.
> >
> > - Let 1 instanceof \stdClass return false, instead of crashing. -> seems
> BC
> > - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>
> Neither of those "crashes", one of them returns fatal error, which is a
> valid option for code that is obviously broken. It would also be a valid
> option to return false, but since it is a valid outcome either way, I
> don't think changing anything is necessary. It does not make any real
> code easier to write (I can't see any legit reason to test numerics for
> being instance of anything) and does not improve anything but abstract
> "consistency", which in this case is not useful for anything.
>

The main issue seems to be that if people are not sure whether they have to
do an is_object() check before using instanceof, they just quickly test it
using "42 instanceof A" and then draw the entirely wrong conclusion from
the generated error. I've certainly seen this play out more than once.

Which is why I'd be in favor of changing instanceof to returning false in
this case.

Nikita
On Sat, Dec 9, 2017 at 1:28 AM, Andreas Hennings <[email protected]> wrote:
> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>
> $x = 1;
> $x instanceof \stdClass;
>
>
> The following (https://3v4l.org/IdSBu) gives a fatal error:
>
> 1 instanceof \stdclass;
>
> t think this behavior is inconsistent, and we should consider changing it.
>
> There is one option, and it is BC.
>
> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems BC
>
This. There's nothing undefinable about "1 instanceof \stdClass", the
clear and obvious answer is: "No, it's not an instance of \stdClass,
it's an integer." The runtime result of false makes sense, the
compile time error doesn't. Let's relax the compile time error (fold
it into a const false if you truly must) and call it done. There's no
need to be hostile to users just because the code *seems* silly.

-Sara

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
> -----Original Message-----
> From: php@golemon.com [mailto:[email protected]] On Behalf Of Sara
> Golemon
> Sent: Sunday, December 10, 2017 1:50 AM
> To: Andreas Hennings <[email protected]>
> Cc: PHP internals <[email protected]>
> Subject: Re: [PHP-DEV] instanceof survives non-object variables, but crashes
> on non-object constants.
>
> On Sat, Dec 9, 2017 at 1:28 AM, Andreas Hennings <[email protected]>
> wrote:
> > The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
> >
> > $x = 1;
> > $x instanceof \stdClass;
> >
> >
> > The following (https://3v4l.org/IdSBu) gives a fatal error:
> >
> > 1 instanceof \stdclass;
> >
> > t think this behavior is inconsistent, and we should consider changing it.
> >
> > There is one option, and it is BC.
> >
> > - Let 1 instanceof \stdClass return false, instead of crashing. ->
> > seems BC
> >
> This. There's nothing undefinable about "1 instanceof \stdClass", the clear
> and obvious answer is: "No, it's not an instance of \stdClass, it's an integer."
> The runtime result of false makes sense, the compile time error doesn't.
> Let's relax the compile time error (fold it into a const false if you truly must)
> and call it done. There's no need to be hostile to users just because the code
> *seems* silly.

+1

Zeev
>
> This. There's nothing undefinable about "1 instanceof \stdClass", the
> clear and obvious answer is: "No, it's not an instance of \stdClass,
> it's an integer." The runtime result of false makes sense, the
> compile time error doesn't. Let's relax the compile time error (fold
> it into a const false if you truly must) and call it done. There's no
> need to be hostile to users just because the code *seems* silly.
>

This code doesn't just *seem* silly. For everything but testing whether it
works it is totally useless.

But we might turn the fatal error into a warning like we do it for "use" in
the global namespace: https://3v4l.org/ph7KW

Regards, Niklas
On 10.12.2017 at 12:05, Niklas Keller wrote:

>> This. There's nothing undefinable about "1 instanceof \stdClass", the
>> clear and obvious answer is: "No, it's not an instance of \stdClass,
>> it's an integer." The runtime result of false makes sense, the
>> compile time error doesn't. Let's relax the compile time error (fold
>> it into a const false if you truly must) and call it done. There's no
>> need to be hostile to users just because the code *seems* silly.
>
> This code doesn't just *seem* silly. For everything but testing whether it
> works it is totally useless.
>
> But we might turn the fatal error into a warning like we do it for "use" in
> the global namespace: https://3v4l.org/ph7KW

I fail to see why

1 instanceof \stdClass

should be handled differently than

1 === (new stdClass)

for instance.

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 10 December 2017 11:05:23 GMT+00:00, Niklas Keller <[email protected]> wrote:
>>
>> This. There's nothing undefinable about "1 instanceof \stdClass",
>the
>> clear and obvious answer is: "No, it's not an instance of \stdClass,
>> it's an integer." The runtime result of false makes sense, the
>> compile time error doesn't. Let's relax the compile time error (fold
>> it into a const false if you truly must) and call it done. There's
>no
>> need to be hostile to users just because the code *seems* silly.
>>
>
>This code doesn't just *seem* silly. For everything but testing whether
>it
>works it is totally useless.

It's useless, but also harmless. Unless anyone can think of a scenario where someone would do this by mistake intending to do something else?


>But we might turn the fatal error into a warning like we do it for
>"use" in
>the global namespace: https://3v4l.org/ph7KW

I feel like both that example and the case we're discussing should be a notice at most; it's not telling me my code is broken, just that some of it's unnecessary. Maybe I've decided it makes my code more self-documenting, and don't care that the compiler thinks I'm dumb.

Adding warnings has a "boy who cried wolf" cost: if people get used to ignoring ones they disagree with, they'll miss ones that are genuine problems.

Regards,

--
Rowan Collins
[IMSoP]

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
> -----Original Message----> From: Niklas Keller [mailto:[email protected]]
> Sent: Sunday, December 10, 2017 1:05 PM
> To: Sara Golemon <[email protected]>
> Cc: Andreas Hennings <[email protected]>; PHP internals
> <[email protected]>
> Subject: Re: [PHP-DEV] instanceof survives non-object variables, but crashes on
> non-object constants.
>
> >
> > This. There's nothing undefinable about "1 instanceof \stdClass", the
> > clear and obvious answer is: "No, it's not an instance of \stdClass,
> > it's an integer." The runtime result of false makes sense, the
> > compile time error doesn't. Let's relax the compile time error (fold
> > it into a const false if you truly must) and call it done. There's no
> > need to be hostile to users just because the code *seems* silly.
> >
>
> This code doesn't just *seem* silly. For everything but testing whether it works
> it is totally useless.

Do you mean the static use case specifically or in general? Because the runtime use case can definitely be useful, even if it's not everyone's cup of tea.

I think it boils down to whether we want to:
(a) Keep things as-is.
(b) Block the runtime use case and error out in case of non-objects.
(c) Change the compile-time evaluation to allow non-objects and return false.

I definitely don't think we should do (b), as I completely agree with Sara the answer to whether $x instanceof \stdClass when $x=1 is well defined, and is a clear 'no' with no need to error out. It's also a perfectly reasonable behavior to rely on.
My vote goes to (c), because while (a) would prevent an admittedly silly piece of code (a hardcoded check on whether an obviously non-object is an object) from running, I think consistency is preferable, and I don't see how erroring out during compile time in this particular case buys us anything.

Zeev
On Sat, Dec 9, 2017 at 7:28 AM, Andreas Hennings <[email protected]>
wrote:

> The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>
> $x = 1;
> $x instanceof \stdClass;
>
>
> The following (https://3v4l.org/IdSBu) gives a fatal error:
>
> 1 instanceof \stdclass;
>
> t think this behavior is inconsistent, and we should consider changing it.
>
> There are two options, but only one is BC.
>
> - Let 1 instanceof \stdClass return false, instead of crashing. -> seems BC
> - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>
> So it seems the first would the option we should take.
> This is also what hhvm does, according to https://3v4l.org/IdSBu.
>

I've prepared a PR for this change: https://github.com/php/php-src/pull/2978

From the discussion I'm understanding that our consensus is to implement
this change, so if there are no further objection I'll merge this in a few
days.

Nikita
What would it hurt to allow instanceof to return types other than object?

$x instanceof NULL
$x instanceof string

and so on. This would be more powerful and useful than throwing an error
where one wasn't being thrown before. It also would let one operator detect
all the types, rather than the plethora of detect functions we currently
have.

On Tue, Dec 19, 2017 at 4:48 PM, Nikita Popov <[email protected]> wrote:

> On Sat, Dec 9, 2017 at 7:28 AM, Andreas Hennings <[email protected]>
> wrote:
>
> > The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
> >
> > $x = 1;
> > $x instanceof \stdClass;
> >
> >
> > The following (https://3v4l.org/IdSBu) gives a fatal error:
> >
> > 1 instanceof \stdclass;
> >
> > t think this behavior is inconsistent, and we should consider changing
> it.
> >
> > There are two options, but only one is BC.
> >
> > - Let 1 instanceof \stdClass return false, instead of crashing. -> seems
> BC
> > - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
> >
> > So it seems the first would the option we should take.
> > This is also what hhvm does, according to https://3v4l.org/IdSBu.
> >
>
> I've prepared a PR for this change: https://github.com/php/php-
> src/pull/2978
>
> From the discussion I'm understanding that our consensus is to implement
> this change, so if there are no further objection I'll merge this in a few
> days.
>
> Nikita
>
We can still do this as a follow-up.
The current fix is easy and straighforward.
What you suggest might be a good idea, but I think deserves more discussion.

On 19 December 2017 at 23:09, Michael Morris <[email protected]> wrote:
> What would it hurt to allow instanceof to return types other than object?
>
> $x instanceof NULL
> $x instanceof string
>
> and so on. This would be more powerful and useful than throwing an error
> where one wasn't being thrown before. It also would let one operator detect
> all the types, rather than the plethora of detect functions we currently
> have.
>
> On Tue, Dec 19, 2017 at 4:48 PM, Nikita Popov <[email protected]> wrote:
>
>> On Sat, Dec 9, 2017 at 7:28 AM, Andreas Hennings <[email protected]>
>> wrote:
>>
>> > The following (https://3v4l.org/A2Tp6) is ok, it simply returns false:
>> >
>> > $x = 1;
>> > $x instanceof \stdClass;
>> >
>> >
>> > The following (https://3v4l.org/IdSBu) gives a fatal error:
>> >
>> > 1 instanceof \stdclass;
>> >
>> > t think this behavior is inconsistent, and we should consider changing
>> it.
>> >
>> > There are two options, but only one is BC.
>> >
>> > - Let 1 instanceof \stdClass return false, instead of crashing. -> seems
>> BC
>> > - Let $x instanceof \stdClass crash, if $x is not an object. -> BC break.
>> >
>> > So it seems the first would the option we should take.
>> > This is also what hhvm does, according to https://3v4l.org/IdSBu.
>> >
>>
>> I've prepared a PR for this change: https://github.com/php/php-
>> src/pull/2978
>>
>> From the discussion I'm understanding that our consensus is to implement
>> this change, so if there are no further objection I'll merge this in a few
>> days.
>>
>> Nikita
>>

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login