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 file resource functions as having side effects #698

Merged

Conversation

jlherren
Copy link
Contributor

@jlherren jlherren commented Oct 4, 2021

'func_get_arg' => ['hasSideEffects' => false],
'func_get_args' => ['hasSideEffects' => false],
'func_num_args' => ['hasSideEffects' => false],
'function_exists' => ['hasSideEffects' => false],
'fwrite' => ['hasSideEffects' => true],
Copy link
Contributor

Choose a reason for hiding this comment

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

this reminds me of, whether file_put_contents and file_get_contents is missing in this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only included the functions that work on file handles. But I guess all filesystem operations probably should be added. In that case it should also include unlink(), file(), mkdir(), and tons of others. I'll try to do that and see if anything breaks.

@jlherren jlherren force-pushed the resource-function-side-effects branch from 86d0fd8 to 5350992 Compare October 5, 2021 08:54
@jlherren
Copy link
Contributor Author

jlherren commented Oct 5, 2021

Okay, here's a revised version, which adds most filesystem functions. file_get_content() is now marked to not have side-effects, which kinda makes sense. However, I'm still a bit unsure about how useful that is. It makes this work though:

if (file_get_contents($path) === 'data') {
	assertType('\'data\'', file_get_contents($path));
	file_put_contents($path, 'other');
	assertType('string|false', file_get_contents($path));
}

Maybe in the future there should be more precise metadata than just "hasSideEffects". Some of these functions would be better described as "volatile". Or perhaps even an interface FunctionSideEffectExtension or something like that.

@ondrejmirtes
Copy link
Member

Hi, thank you! The file you modified is actually generated automatically when running ./bin/generate-function-metadata.php and the result is merged from ./bin/functionMetadata_original.php and by looking at jetbrains/phpstorm-stubs methods and functions with the #[Pure] attribute.

I'd like this course of action instead:

  1. Contribute the functions that have 'hasSideEffects' => true as a modification to functionMetadata_original.php.
  2. Contribute the #[Pure] functions without side effects to jetbrains/phpstorm-stubs.
  3. Once the PR from 2) is merged, please update the package here and run ./bin/generate-function-metadata.php.

Thank you!

@jlherren
Copy link
Contributor Author

jlherren commented Oct 6, 2021

This got a bit complicated unfortunately.

The phpstorm-stubs update has been merged. However, updating phpstorm-stubs to the newest master causes a few tests to fail. This is due to a change in phpstorm-stubs, which implements PHP RFC: Add return type declarations for internal methods. That RFC is about adding tentative return type declarations, which as I understand is about return type declarations that only trigger a warning, but aren't fully enforced yet (not until PHP 9.0).

As an example, Countable::count() has now a tentative return type int, causing PHPStan to complain about all implementations that don't have a compatible return type declaration.

I'm not sure what the best course of action here. In the long term, PHPStan should probably parse #[TentativeType] and act on it. For now I'll reduce this PR to only update ./bin/functionMetadata_original.php, it should be enough to fix phpstan/phpstan#5461.

@jlherren jlherren force-pushed the resource-function-side-effects branch from 5350992 to 1f30247 Compare October 17, 2021 10:29
@jlherren jlherren force-pushed the resource-function-side-effects branch from 1f30247 to 8b5f94d Compare November 8, 2021 18:27
@jlherren
Copy link
Contributor Author

jlherren commented Nov 8, 2021

I see that tentative return types have been implemented and the stubs have been updated. I rebased this to fix conflicts.

@ondrejmirtes ondrejmirtes merged commit 96a90cc into phpstan:master Nov 8, 2021
@ondrejmirtes
Copy link
Member

Thank you!

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