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

[Basic functions] Added return type for arginfo #1492

Closed
wants to merge 2 commits into from

Conversation

ovr
Copy link
Contributor

@ovr ovr commented Aug 26, 2015

Hey!
I love PHP7 ideas about return type hints and it's very usefull in static analysis
I am working https://github.com/ovr/phpsa
Now I am using https://github.com/ovr/phpreflection but I would like to see return type for all functions in PHP7 on FunctionReflection

Now
I am going to add return types for basic functions

Dear mainterns:
If idea is rly good please write smthgs in comments :)
Because i don't know is it okey or not

Main questions:
1 http://php.net/manual/en/function.abs.php
Maybe it's time to add fake type IS_NUMBER?

Thanks

@ovr ovr force-pushed the better-reflection branch from 06bf4db to dac78e3 Compare August 26, 2015 06:30
@ovr
Copy link
Contributor Author

ovr commented Aug 30, 2015

@nikic @laruence @dstogov

1 http://php.net/manual/en/function.abs.php
Maybe it's time to add fake type IS_NUMBER?

@dstogov
Copy link
Member

dstogov commented Aug 31, 2015

Hi Dmitry,

PHP-7 is in RC stage. We can't add any new features now.
We may add something like this in 7.1, and this will require RFC,
discussion, voting...

Thanks. Dmitry.

On Sun, Aug 30, 2015 at 7:22 PM, Dmitry Patsura notifications@github.com
wrote:

@nikic https://github.com/nikic @laruence https://github.com/laruence
@dstogov https://github.com/dstogov

1 http://php.net/manual/en/function.abs.php
Maybe it's time to add fake type IS_NUMBER?


Reply to this email directly or view it on GitHub
#1492 (comment).

@laruence laruence added the RFC label Aug 31, 2015
@krakjoe
Copy link
Member

krakjoe commented Jan 2, 2017

Since no RFC has materialized, and since the work seems to have been abandoned by the author, I'm closing this PR.

Take this as encouragement to create a clean, complete PR, targeting an appropriate branch (too late for 7.1), and start an accompanying RFC and internals discussion at the same time.

@krakjoe krakjoe closed this Jan 2, 2017
@dstogov
Copy link
Member

dstogov commented Jan 9, 2017

BTW: I don't see a problem merging this PR into master.

@carusogabriel
Copy link
Contributor

@morrisonlevi Was this you were talking about #3026 (comment)?

@krakjoe
Copy link
Member

krakjoe commented Feb 14, 2018

@dstogov quote:

PHP-7 is in RC stage. We can't add any new features now.
We may add something like this in 7.1, and this will require RFC,
discussion, voting...

Was this in reference to the suggestion to add IS_NUMBER ?

@dstogov
Copy link
Member

dstogov commented Feb 14, 2018

@krakjoe yes

@krakjoe krakjoe reopened this Feb 14, 2018
@krakjoe krakjoe removed the RFC label Feb 14, 2018
@krakjoe
Copy link
Member

krakjoe commented Feb 14, 2018

Merged b7d2e04

Thanks.

Apologies for the confusion ...

@krakjoe krakjoe closed this Feb 14, 2018
@krakjoe krakjoe reopened this Feb 14, 2018
@krakjoe
Copy link
Member

krakjoe commented Feb 14, 2018

Reverted because the use of macro is wrong since we changed to use zend_type ...

@ovr please update for current master ...

If @ovr doesn't respond in the next week or so, I'll do it again myself ...

@ovr ovr force-pushed the better-reflection branch 2 times, most recently from 687e2e7 to d3908b1 Compare February 14, 2018 12:06
@ovr ovr force-pushed the better-reflection branch from d3908b1 to 7933411 Compare February 14, 2018 12:07
@ovr ovr changed the title [WIP] [Basic functions] Added return type for arginfo [Basic functions] Added return type for arginfo Feb 14, 2018
@ovr
Copy link
Contributor Author

ovr commented Feb 14, 2018

Rebased commits, drop unneeded argument on ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO & ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX

@@ -1619,71 +1619,71 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_round, 0, 0, 1)
ZEND_ARG_INFO(0, mode)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO(arginfo_sin, 0)
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO(arginfo_sin, IS_DOUBLE, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these will need to be declared nullable due to zpp failures :(

Copy link
Contributor

@morrisonlevi morrisonlevi Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic I think it would be better for the future if we distinguish at the macro level the functions returning nullable types because of ZPP failures and those which may return null even if ZPP failure is changed to TypeError. Unless we don't have any of those; I know returning false is incredibly common. Agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic @krakjoe bump on this.

Copy link
Contributor

@carusogabriel carusogabriel Feb 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morrisonlevi @nikic @krakjoe Is functions return type something that might be happening in PHP 7.x? Is so, I've started to write a script (it is very simple yet), so we can automate that. Let me know if you want me to PR it to the core, so we can start adding them 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes much sense to add return type arginfo, if we're not also adding parameter type arginfo. Furthermore, adding return type arginfo is a general problem as pointed out above, since ZPP failures are handled manually (most functions return NULL in this case, but I've seen functions returning FALSE). It might be most reasonable to postpone this to PHP 8, where we hopefully can throw exceptions on ZPP failures, and maybe even unify arginfo and ZPP.

@krakjoe
Copy link
Member

krakjoe commented Feb 22, 2018

@ovr bump, see nikita's comment ...

@php-pulls
Copy link

Comment on behalf of kalle at php.net:

Closing due to inactivity

@php-pulls php-pulls closed this Sep 26, 2018
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.

9 participants