Skip to content

exit statuses should be in the range 0 to 254#6648

Closed
sasezaki wants to merge 2 commits intovimeo:masterfrom
sasezaki:callmap_exit
Closed

exit statuses should be in the range 0 to 254#6648
sasezaki wants to merge 2 commits intovimeo:masterfrom
sasezaki:callmap_exit

Conversation

@sasezaki
Copy link
Copy Markdown
Contributor

@sasezaki sasezaki commented Oct 11, 2021

from manual.

If status is an int, that value will be used as the exit status and not printed. Exit statuses should be in the range 0 to 254, the exit status 255 is reserved by PHP and shall not be used. The status 0 is used to terminate the program successfully.
https://www.php.net/manual/en/function.exit.php

actual PHP behaviors is like this.

sasezaki@z:~$ php -r "exit(0);"; echo $?
0
sasezaki@z:~$ php -r "exit(254);"; echo $?
254
sasezaki@z:~$ php -r "exit(255);"; echo $?
255
sasezaki@z:~$ php -r "exit(256);"; echo $?
0
sasezaki@z:~$ php -r "exit(-1);"; echo $?
255
sasezaki@z:~$ php -r "exit(-2);"; echo $?
254

Note: This is my first change for CallMap, so if anything missing, please let me know.

@sasezaki sasezaki changed the title exit statuses should be in the range 0 to 254 [WIP] exit statuses should be in the range 0 to 254 Oct 11, 2021
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Oct 12, 2021

Seems correct :)

I'm always a bit cautious about changing parameter types to non-standard types because it has the potential to bother a lot of people but I don't expect exit to be used a lot with dynamic params

@weirdan
Copy link
Copy Markdown
Collaborator

weirdan commented Oct 12, 2021

Since exit() is not really a function, I'd suggest to add a test highlighting new behavior. Callmap could be ignored in this case, and you may need to change src/Psalm/Internal/Analyzer/Statements/Expression/ExitAnalyzer.php instead.

Looks good to me otherwise.

@sasezaki
Copy link
Copy Markdown
Contributor Author

sasezaki commented Oct 13, 2021

Great thanks for review and feedback !

Callmap could be ignored in this case, and you may need to change src/Psalm/Internal/Analyzer/Statements/Expression/ExitAnalyzer.php instead.

I got it! I just understand why my locally test was not reported as an error.

Then, in this PR, I would to try update ExitAnalyzer...

@staabm staabm mentioned this pull request Oct 13, 2021
Merged
@sasezaki sasezaki changed the title [WIP] exit statuses should be in the range 0 to 254 exit statuses should be in the range 0 to 254 Oct 14, 2021
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Oct 14, 2021 •
edited
Loading

Interesting, it seems that PHPUnit uses 255 as an error code.

It doesn't seem to cause problems and it returns 255 to the caller: https://3v4l.org/b1tnm

I'd be inclined to allow it, even if the doc says it's supposed to be reserved...

@weirdan
Copy link
Copy Markdown
Collaborator

weirdan commented Oct 14, 2021

I'd be inclined to allow it,

👍

@sasezaki
Copy link
Copy Markdown
Contributor Author

sasezaki commented Oct 14, 2021 •
edited
Loading

PHPUnit uses 255 as an error code.

yep, PHPUnit seems introduced 255 code just 2 month ago.
sebastianbergmann/phpunit@5604ae9

I'd be inclined to allow it, even if the doc says it's supposed to be reserved...

Agreed.

I would like to investigate php-src if I have time, ... currently, it seems better to forgo this change.

@sasezaki sasezaki closed this Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sasezaki @orklah @weirdan