Scalar Type Declaration Syntax Weirdness

9 years ago by Ryan Pallas — view source

unread

On Tue, Nov 24, 2015 at 8:10 AM, Sebastian Bergmann sebastian@php.net
wrote:

The following is currently valid PHP 7 code
 &lt;?php
 function a(\int $i) {}
Is it intentional that the \ in front of the "int" is allowed? IMHO, this
confusing notation must not be allowed.

Why not? Its a root level type, you can prefix a \ on any other root level
type that's valid for use in type hinting.

function a(\DateTime $d) {}
function b(\SplFileObject $f) {}

Also, this is the only way to get some IDEs to recognize the type when in a
namespace (at least currently).

9 years ago by Sebastian Bergmann — view source

unread

Also, this is the only way to get some IDEs to recognize the type when in a
namespace (at least currently).

Then the IDEs have to be fixed.

9 years ago by Andrea Faulds — view source

unread

Hi Sebastian,

Sebastian Bergmann wrote:

The following is currently valid PHP 7 code
  &lt;?php
  function a(\int $i) {}
Is it intentional that the \ in front of the "int" is allowed? IMHO, this
confusing notation must not be allowed.

This is weird and I'd consider it a bug. You can't do \array or
\callable, and if I saw \int, I'd think it meant a class of that name
rather than a scalar type.

Can this be fixed for 7.0.0?

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Stanislav Malyshev — view source

unread

Hi!

  &lt;?php
  function a(\int $i) {}
Is it intentional that the \ in front of the "int" is allowed? IMHO,
this
confusing notation must not be allowed.
This is weird and I'd consider it a bug. You can't do \array or
\callable, and if I saw \int, I'd think it meant a class of that name
rather than a scalar type.

I would assume \int means class named "int", as opposed to "int" type.
But banning it may be ok too.

Can this be fixed for 7.0.0?

I don't think this would be a good idea. We're in the final stretch of
release cycle, and should not do any non-urgent fixes. This does not
look urgent. It can wait for 7.0.1.

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Andrea Faulds — view source

unread

Hi Stas,

Stanislav Malyshev wrote:

Hi!
   &lt;?php
   function a(\int $i) {}
Is it intentional that the \ in front of the "int" is allowed? IMHO,
this
confusing notation must not be allowed.
This is weird and I'd consider it a bug. You can't do \array or
\callable, and if I saw \int, I'd think it meant a class of that name
rather than a scalar type.
I would assume \int means class named "int", as opposed to "int" type.

That's also what I'd expect. However, "int" is not allowed as a class
name in PHP 7. And unfortunately what the code Sebastian posted sctually
does is act as an integer type hint, not as a class type hint.

Can this be fixed for 7.0.0?

I don't think this would be a good idea. We're in the final stretch of
release cycle, and should not do any non-urgent fixes. This does not
look urgent. It can wait for 7.0.1.

It can't wait for 7.0.1, because banning this would be a
backwards-compatibility break with 7.0.0. We have to fix it in 7.0.0 or
not fix it ever.

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Stanislav Malyshev — view source

unread

Hi!

It can't wait for 7.0.1, because banning this would be a
backwards-compatibility break with 7.0.0. We have to fix it in 7.0.0 or
not fix it ever.

In theory, yes. In practice, if somebody starts using 7.0.0 and
immediately jumps to using \int, I don't feel too bad for breaking that
code. We can put a note in release notes for this is needed. But the
risk of changing syntax parts on the brink of GA IMHO is much larger
than the risk of somebody using \int in 7.0.0 and getting breakage in
7.0.1. Especially if it's clearly described as a bug we intend to fix.

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Andrea Faulds — view source

unread

Hi Stas,

Stanislav Malyshev wrote:

Hi!

It can't wait for 7.0.1, because banning this would be a
backwards-compatibility break with 7.0.0. We have to fix it in 7.0.0 or
not fix it ever.

In theory, yes. In practice, if somebody starts using 7.0.0 and
immediately jumps to using \int, I don't feel too bad for breaking that
code. We can put a note in release notes for this is needed. But the
risk of changing syntax parts on the brink of GA IMHO is much larger
than the risk of somebody using \int in 7.0.0 and getting breakage in
7.0.1. Especially if it's clearly described as a bug we intend to fix.

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Matteo Beccati — view source

unread

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

I agree and we should be still in time for RC8.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

9 years ago by Bob Weinand — view source

unread

Am 24.11.2015 um 20:30 schrieb Matteo Beccati php@beccati.com:

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

I agree and we should be still in time for RC8.

Cheers

Matteo Beccati

Hey,

I fixed the issue via http://git.php.net/?p=php-src.git;a=commitdiff;h=569763cb1ac67f56e7743062ca8b3b7c650c1254

I think too this should go into PHP 7.0.0 as it is some type of a language related change (even if not directly failing in parser...)

Bob

9 years ago by Xinchen Hui — view source

unread

Hey:

Am 24.11.2015 um 20:30 schrieb Matteo Beccati php@beccati.com:

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

I agree and we should be still in time for RC8.

Cheers

Matteo Beccati

Hey,

I fixed the issue via
http://git.php.net/?p=php-src.git;a=commitdiff;h=569763cb1ac67f56e7743062ca8b3b7c650c1254

I think too this should go into PHP 7.0.0 as it is some type of a language
related change (even if not directly failing in parser...)

I've made a improvement to the fix(
https://github.com/php/php-src/commit/00865ae22f2c5fdee9e500ce79d442467e0a0899)
,

before this, \array will result a syntax , but \int result a compiler
error, which seems a little in-consistent.

thanks

Bob

--
Xinchen Hui
@Laruence
http://www.laruence.com/

9 years ago by Nikita Popov — view source

unread

Hey:

Am 24.11.2015 um 20:30 schrieb Matteo Beccati php@beccati.com:

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around
with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

I agree and we should be still in time for RC8.

Cheers

Matteo Beccati

Hey,

I fixed the issue via

http://git.php.net/?p=php-src.git;a=commitdiff;h=569763cb1ac67f56e7743062ca8b3b7c650c1254

I think too this should go into PHP 7.0.0 as it is some type of a
language
related change (even if not directly failing in parser...)

I've made a improvement to the fix(

https://github.com/php/php-src/commit/00865ae22f2c5fdee9e500ce79d442467e0a0899
)
,

before this, \array will result a syntax , but \int result a compiler
error, which seems a little in-consistent.

Imho this additional change is not necessary, it only makes the parser more
complicated.

However something missing from the original patch is handling of relative
names like namespace\int. Instead of checking for ast->attr == ZEND_NAME_FQ
it should check for ast->attr != ZEND_NAME_NOT_FQ.

Nikita

9 years ago by Xinchen Hui — view source

unread

Hey:

Hey:

Am 24.11.2015 um 20:30 schrieb Matteo Beccati php@beccati.com:

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around
with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

I agree and we should be still in time for RC8.

Cheers

Matteo Beccati

Hey,

I fixed the issue via

http://git.php.net/?p=php-src.git;a=commitdiff;h=569763cb1ac67f56e7743062ca8b3b7c650c1254

I think too this should go into PHP 7.0.0 as it is some type of a
language
related change (even if not directly failing in parser...)

I've made a improvement to the fix(

https://github.com/php/php-src/commit/00865ae22f2c5fdee9e500ce79d442467e0a0899
)
,

before this, \array will result a syntax , but \int result a compiler
error, which seems a little in-consistent.

Imho this additional change is not necessary, it only makes the parser
more complicated.

However something missing from the original patch is handling of relative
names like namespace\int. Instead of checking for ast->attr == ZEND_NAME_FQ
it should check for ast->attr != ZEND_NAME_NOT_FQ.

yeah, you are right, namespace\array still behavior difference from
namespace\int.

I am going to revert my part.

thanks

Nikita

--
Xinchen Hui
@Laruence
http://www.laruence.com/

9 years ago by Xinchen Hui — view source

unread

Hey:

Hey:

Am 24.11.2015 um 20:30 schrieb Matteo Beccati php@beccati.com:

There's no syntax change. We'd be adding another fatal error to
zend_compile.c triggered by a flag on the token. No messing around
with
the parser.

I understand your concern about the risk, but it's the kind of change
that wouldn't break anything without it being tremendously obvious.

I agree and we should be still in time for RC8.

Cheers

Matteo Beccati

Hey,

I fixed the issue via

http://git.php.net/?p=php-src.git;a=commitdiff;h=569763cb1ac67f56e7743062ca8b3b7c650c1254

I think too this should go into PHP 7.0.0 as it is some type of a
language
related change (even if not directly failing in parser...)

I've made a improvement to the fix(

https://github.com/php/php-src/commit/00865ae22f2c5fdee9e500ce79d442467e0a0899
)
,

before this, \array will result a syntax , but \int result a compiler
error, which seems a little in-consistent.

Imho this additional change is not necessary, it only makes the parser
more complicated.

However something missing from the original patch is handling of relative
names like namespace\int. Instead of checking for ast->attr == ZEND_NAME_FQ
it should check for ast->attr != ZEND_NAME_NOT_FQ.

PS: However, namespace\int will result error while checking valid
classname, as int is reserved keywords.

so I think check for == ZEND_NAME_FQ is enough here.

thanks

Nikita

--
Xinchen Hui
@Laruence
http://www.laruence.com/

9 years ago by Andrea Faulds — view source

unread

Hi,

Xinchen Hui wrote:

Hey:

Imho this additional change is not necessary, it only makes the parser
more complicated.

However something missing from the original patch is handling of relative
names like namespace\int. Instead of checking for ast->attr == ZEND_NAME_FQ
it should check for ast->attr != ZEND_NAME_NOT_FQ.

PS: However, namespace\int will result error while checking valid
classname, as int is reserved keywords.

so I think check for == ZEND_NAME_FQ is enough here.

This is how I feel as well. You can't make a class with that name anyway
(at least for now), so we don't need to prohibit it. \int was a problem
because it was interpreted the same as 'int'.

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Joshua Holmer — view source

unread

Hi!

It can't wait for 7.0.1, because banning this would be a
backwards-compatibility break with 7.0.0. We have to fix it in 7.0.0 or
not fix it ever.

In theory, yes. In practice, if somebody starts using 7.0.0 and
immediately jumps to using \int, I don't feel too bad for breaking that
code. We can put a note in release notes for this is needed. But the
risk of changing syntax parts on the brink of GA IMHO is much larger
than the risk of somebody using \int in 7.0.0 and getting breakage in
7.0.1. Especially if it's clearly described as a bug we intend to fix.

Hi,

Isn't it quite worse to fix this in a patch release than to fix it prior to
GA? If something like this breaks prior to GA, that's tolerated. People
have been warned not to use pre-release versions of PHP7 in production.
Once 7.0.0 is out, people expect stability. If something breaks between
patch releases, even an undocumented behavior like this, it makes PHP look
bad. Even if you call this change a bug fix, there's no reason not to put
it in now, since isn't the purpose of the RCs to fix as many bugs as
possible before GA?

Josh Holmer

9 years ago by Anatol Belski — view source

unread

Hi Stas,

-----Original Message-----
From: Stanislav Malyshev [mailto:smalyshev@gmail.com]
Sent: Tuesday, November 24, 2015 6:28 PM
To: Andrea Faulds ajf@ajf.me; internals@lists.php.net
Subject: Re: [PHP-DEV] Re: Scalar Type Declaration Syntax Weirdness

Hi!

It can't wait for 7.0.1, because banning this would be a
backwards-compatibility break with 7.0.0. We have to fix it in 7.0.0
or not fix it ever.

In theory, yes. In practice, if somebody starts using 7.0.0 and immediately jumps
to using \int, I don't feel too bad for breaking that code. We can put a note in
release notes for this is needed. But the risk of changing syntax parts on the
brink of GA IMHO is much larger than the risk of somebody using \int in 7.0.0
and getting breakage in 7.0.1. Especially if it's clearly described as a bug we
intend to fix.

that's my point as well. There is a clear documentation about type hints, usage of an undocumented way is out of scope of BC. Using \int means there were a "class int{}" which is prohibited. Of course it is a bug after all, which will be addressed.

Regards

Anatol

9 years ago by Andrea Faulds — view source

unread

Hi,

Anatol Belski wrote:

that's my point as well. There is a clear documentation about type hints, usage of an undocumented way is out of scope of BC. Using \int means there were a "class int{}" which is prohibited. Of course it is a bug after all, which will be addressed.

It may not be documented, but that doesn't put it outside the scope of
BC. People will unintentionally rely on bugs.

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Anatol Belski — view source

unread

Hi Andrea,

-----Original Message-----
From: Andrea Faulds [mailto:ajf@ajf.me]
Sent: Wednesday, November 25, 2015 9:00 PM
To: internals@lists.php.net
Subject: Re: [PHP-DEV] Re: Scalar Type Declaration Syntax Weirdness

Hi,

Anatol Belski wrote:

that's my point as well. There is a clear documentation about type hints, usage
of an undocumented way is out of scope of BC. Using \int means there were a
"class int{}" which is prohibited. Of course it is a bug after all, which will be
addressed.

It may not be documented, but that doesn't put it outside the scope of BC.
People will unintentionally rely on bugs.

What is the reason to use \int if "class \int{}" is prohibited? A typo :) ?

If the patch is fine within RC8 period, it is going to be applied in 7.0.1 which is soon enough to avoid creating bad habits. Applying a couple of hours old patch to the bug with such severity seems not appropriate, anyway.

Regards

Anatol

9 years ago by Andrea Faulds — view source

unread

Hi Anatol,

Anatol Belski wrote:

It may not be documented, but that doesn't put it outside the scope of BC.
People will unintentionally rely on bugs.

What is the reason to use \int if "class \int{}" is prohibited? A typo :) ?

It might be used deliberately since some IDEs (PHPStorm in particular)
can't handle PHP 7's scalar type hints properly in namespaces yet.

Thanks.

--
Andrea Faulds
http://ajf.me/

9 years ago by Anatol Belski — view source

unread

Hi Andrea,

-----Original Message-----
From: Andrea Faulds [mailto:ajf@ajf.me]
Sent: Thursday, November 26, 2015 12:47 AM
To: internals@lists.php.net
Subject: Re: [PHP-DEV] Re: Scalar Type Declaration Syntax Weirdness

Hi Anatol,

Anatol Belski wrote:

It may not be documented, but that doesn't put it outside the scope of BC.
People will unintentionally rely on bugs.

What is the reason to use \int if "class \int{}" is prohibited? A typo :) ?

It might be used deliberately since some IDEs (PHPStorm in particular) can't
handle PHP 7's scalar type hints properly in namespaces yet.

Hm, but then it's a bug in PHPStorm for PHP7 support, not in PHP? Particularly I've found this amusing thread https://devnet.jetbrains.com/message/5507875 . Even PHPStorm would insert \int, it'll then complain that there's no such class, and it'll complain about the doc blocks, and passing int instead of \int object. So an IDE will really need to be PHP 7 compatible.

Also, the basic situation is even worse, any code ported to PHP7 and using type hints is potentially already broken. With this patch available on the day zero, those codes won't work. We know that this fix is necessary and can't be avoided, so putting it into somewhere after day zero - the broken PHP codes will work with .0 and there will be a gap to fix them till next PHP release.

Regards

Anatol

9 years ago by Stanislav Malyshev — view source

unread

Hi!

It may not be documented, but that doesn't put it outside the scope of
BC. People will unintentionally rely on bugs.

People might, but we are under no obligation to keep bugs around for
these people.

--
Stas Malyshev
smalyshev@gmail.com

9 years ago by Chris Riley — view source

unread

Hi Stas,

Stanislav Malyshev wrote:
Hi!
   &lt;?php
   function a(\int $i) {}
Is it intentional that the \ in front of the "int" is allowed? IMHO,
this
confusing notation must not be allowed.
This is weird and I'd consider it a bug. You can't do \array or
\callable, and if I saw \int, I'd think it meant a class of that name
rather than a scalar type.
I would assume \int means class named "int", as opposed to "int" type.
That's also what I'd expect. However, "int" is not allowed as a class name
in PHP 7. And unfortunately what the code Sebastian posted sctually does is
act as an integer type hint, not as a class type hint.

Can this be fixed for 7.0.0?

I don't think this would be a good idea. We're in the final stretch of
release cycle, and should not do any non-urgent fixes. This does not
look urgent. It can wait for 7.0.1.

It can't wait for 7.0.1, because banning this would be a
backwards-compatibility break with 7.0.0. We have to fix it in 7.0.0 or not
fix it ever.

Thanks.

--
Andrea Faulds
http://ajf.me/

--

Could we not just document it as \int is not allowed and throws a fatal
error. Then we'd just be fixing a bug in documented behaviour.

~C

9 years ago by Sebastian Bergmann — view source

unread

Can this be fixed for 7.0.0?

I sure hope so.

9 years ago by Anthony Ferrara — view source

unread

Sebastian,

The following is currently valid PHP 7 code
 &lt;?php
 function a(\int $i) {}
Is it intentional that the \ in front of the "int" is allowed? IMHO, this
confusing notation must not be allowed.

Yeah, that is a problem. We should fix that prior to gold. I'll take a
peak at it when I get some time, but if someone else has a few minutes
and wants to tackle it go for it.

Anthony