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

mark readline* function having side-effects #1153

Merged
merged 3 commits into from Apr 1, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 31, 2022

@staabm staabm marked this pull request as ready for review March 31, 2022 14:03
@staabm
Copy link
Contributor Author

staabm commented Mar 31, 2022

btw: I am wondering why phpstan does not report errors atm https://phpstan.org/r/91377db4-0029-494f-935b-83c619fa5f06
but psalm does https://psalm.dev/r/eb33a31483

@staabm
Copy link
Contributor Author

staabm commented Mar 31, 2022

This needs to be adjusted like described by ondrey in #698 (comment)

@staabm staabm marked this pull request as draft March 31, 2022 18:49
@staabm
Copy link
Contributor Author

staabm commented Apr 1, 2022

added the instructions from #698 (comment) into the generated file, to hopefully ease the contributor experience

@staabm staabm marked this pull request as ready for review April 1, 2022 07:15
@ondrejmirtes
Copy link
Member

Hi, can you please first verify that these records actually need to be added by some tests? Ideally NodeScopeResolverTest + CallToFunctionStatementWithoutSideEffectsRule. It's possible that PHPStan already knows exactly what's going on.

Also the "generated" message in the generated file should be in a separate PR if we decide the readline changes in this one aren't needed :)

@staabm
Copy link
Contributor Author

staabm commented Apr 1, 2022

It's possible that PHPStan already knows exactly what's going on.

hmm ok.. this might explain, why I don't see errors in the background, even this entries are missing.

btw: I am wondering why phpstan does not report errors atm phpstan.org/r/91377db4-0029-494f-935b-83c619fa5f06


Also the "generated" message in the generated file should be in a separate PR if we decide the readline changes in this one aren't needed :)

I will adjust this PR to only include the generated-file-comment (I will not change the PR title accordingly)

@ondrejmirtes ondrejmirtes merged commit 6e1b71a into phpstan:1.5.x Apr 1, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the readline-sideeffects branch April 1, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants