Treat SplFileObject methods as impure/having side effects

[mind-bending-forks]

This addresses phpstan/phpstan#11200

• functionMetadata_original has been updated to mark relevant SplFileObject methods as having side effects.
• bin/generate-function-metadata.php has been run to recreate functionMetadata.php.

[ondrejmirtes]

Do really all of these methods have side effects? I doubt methods like "valid" or "getBasename" have side effects.

[mind-bending-forks]

I haven't written any tests to demonstrate that these changes have the desired effect. Hopefully there will be a change of state of https://phpstan.org/r/f2ce4361-b9e5-4f51-a97e-d6828f618e0f from producing errors to producing none. If tests are required to be written, please point me to some existing tests for similar function signature updates (or some test docs), so I can take a look at doing that using the existing tests as a template for what's required. Thanks.

[ondrejmirtes]

Look at tests in this directory https://github.com/phpstan/phpstan-src/tree/1.11.x/tests/PHPStan/Analyser/nsrt and the docs: https://phpstan.org/developing-extensions/testing#type-inference

[mind-bending-forks]

Do really all of these methods have side effects? I doubt methods like "valid" or "getBasename" have side effects.

What I'm actually trying to do is get certain methods as being treated as impure. Having side effects means something different, I think, so I may not be doing this the right way. I'm just copying what was done for feof, etc.

The return value of SplFileObject::eof may change between invocations of SplFileObject::fetch SplFileObject::fgets, for example, so I think this needs it. SplFileObject::valid reports whether eof has been reached, so this needs to be treated the same way.

As for properties of the file object being pointed to, like getBasename, these can change on the filesystem, but you'd imagine that the values reported by SplFileObject would be cached and not dynamically updated... So I think I've been overzealous there. I'll remove the hasSideEffects declarations from those methods inherited from SplFileInfo.

[ondrejmirtes]

When SplFileObject::eof is marked as impure, the result of valid will be forgotten after calling SplFileObject::eof.

So valid can still be pure.

[mind-bending-forks]

When SplFileObject::eof is marked as impure, the result of valid will be forgotten after calling SplFileObject::eof.

So valid can still be pure.

If I'm understanding correctly, you're saying that if a method marked as having side effects is called, all other methods on the object are treated as potentially providing a different return value? If that's the case, then maybe only the following methods need to be marked as having side effects:

	'SplFileObject::fgets' => ['hasSideEffects' => true],
	'SplFileObject::fflush' => ['hasSideEffects' => true],
	'SplFileObject::fgetc' => ['hasSideEffects' => true],
	'SplFileObject::fgetcsv' => ['hasSideEffects' => true],
	'SplFileObject::fgets' => ['hasSideEffects' => true],
	'SplFileObject::fgetss' => ['hasSideEffects' => true],
	'SplFileObject::flock' => ['hasSideEffects' => true],
	'SplFileObject::fpassthru' => ['hasSideEffects' => true],
	'SplFileObject::fputcsv' => ['hasSideEffects' => true],
	'SplFileObject::fread' => ['hasSideEffects' => true],
	'SplFileObject::fscanf' => ['hasSideEffects' => true],
	'SplFileObject::fseek' => ['hasSideEffects' => true],
	'SplFileObject::ftruncate' => ['hasSideEffects' => true],
	'SplFileObject::fwrite' => ['hasSideEffects' => true],

[mind-bending-forks]

I think the above can all potentially move the pointer to the current position in the file, and cause other methods to return different values.

[ondrejmirtes]

Watch me talking about this for about 10 minutes: https://youtu.be/AFjr3RlDOZQ?si=x5msNPD-mJPZz20T&t=1729

You can also read about it here: https://phpstan.org/blog/remembering-and-forgetting-returned-values

[mind-bending-forks]

Thank you. (I put the video on 1.5x speed, but then got hooked and watched much more than 10 mins anyway!) I believe that I understand @phpstan-pure and @phpstan-impure now. The part of the jigsaw that is missing is the relationship between 'hasSideEffects' => true in functionMetadata(_original).php and @phpstan-(im)pure. In any case, I'll see if I can write some tests now, which will show whether I really understood or not!

[mind-bending-forks]

@ondrejmirtes I've written some tests that assert the expected behaviour of PHPStan and confirmed that they fail when running the pipeline. I've then introduced a change to mark the relevant methods has having side effects. The assertions in the new test should now be valid. One is still failing though. This relates to fgetss. The reason is that this particular method was removed from the language in PHP 8.0.0. To account for this, I wrapped the code for that test in a version check to ensure that it only gets executed for PHP < 8.0.0:

  public function fgetss() : void
  {
    // fgetss has been removed as of PHP 8.0.0
    if ( \version_compare(\PHP_VERSION,'8.0.0','<') )
    {
      $file = new \SplFileObject('php://memory', 'r');
      assertType('bool', $file->eof());
      if ( $file->eof() )
      {
        return;
      }
      assertType('false', $file->eof());
      // call method that has side effects
      $file->fgetss();
      // the value of eof may have changed
      assertType('bool', $file->eof());
    }
  }

phpstan is not recognising the that code is not being executed for PHP >= 8.0.0 though, and so this particular test is failing for all PHP >= 8.0.0, but passing for PHP < 8.0.0.

How should I handle this case? Is there anything I can do to instruct PHPStan that the code should be ignored for PHP >= 8.0.0?

Thanks.

[ondrejmirtes]

@mind-bending-forks You can put this assertion in a separate file and mark it with // lint < 8.0.

[mind-bending-forks]

@ondrejmirtes All tests are passing now. There appear to be some failing integration and extension checks, but I don't think they are anything to do with my changes. So, please consider this pull request finalised and ready for review/merging by you.

Note of caution: The assertions in the tests I have written are based on how I anticipate the corresponding SplFileObject methods may behave and, in particular, the (types of) values the associated methods may output in certain circumstances. As I understand it though (and I may be wrong) the PHP test code I have written is not being executed - simply statically-analysed by PHP. To prove that the hasSideEffects methods can result in the asserted behaviour, I would have to write additional unit test code demonstrating that those methods can actually behave that way (i.e. causing a method to return a different value or type of value) when executed. That would require opening specific files with known contents, not 'php://memory', for example.

I'm hoping that you agree that what I have done looks sensible and is sufficient!

[ondrejmirtes]

Absolutely, this is more than sufficient to test this, thank you!