Rule to check if required file exists

[Bellangelo]

Closes phpstan/phpstan#3397

Originally created at: https://github.com/Bellangelo/phpstan-require-file-exists

[phpstan-bot]

You've opened the pull request against the latest branch 1.12.x. If your code is relevant on 1.11.x and you want it to be released sooner, please rebase your pull request and change its target to 1.11.x.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

I just realized one big thing we need to implement here.

See the documentation: https://www.php.net/manual/en/function.include.php

Files are included based on the file path given or, if none is given, the include_path specified. If the file isn't found in the include_path, include will finally check in the calling script's own directory and the current working directory before failing. The include construct will emit an E_WARNING if it cannot find a file; this is different behavior from require, which will emit an E_ERROR.

So - for absolute paths we can just check is_file before throwing an error, but for relative paths, we need to implement the logic described in the PHP documentation.

Also - we don't need these many tests, the tests you added simply test the logic that is already present in $scope->getType(), not the code that's in the rule itself.

I'd be fine if just tested a few cases where general string is passed into require_once (no error reported) and a union of a couple of constant strings (which are checked against existing files).

Instead of these tests, we should thoroughly test the logic I quoted above.

Thanks.

[Bellangelo]

I just realized one big thing we need to implement here.

See the documentation: https://www.php.net/manual/en/function.include.php

Files are included based on the file path given or, if none is given, the include_path specified. If the file isn't found in the include_path, include will finally check in the calling script's own directory and the current working directory before failing. The include construct will emit an E_WARNING if it cannot find a file; this is different behavior from require, which will emit an E_ERROR.

So - for absolute paths we can just check is_file before throwing an error, but for relative paths, we need to implement the logic described in the PHP documentation.

Fixed in e1469de.

Also - we don't need these many tests, the tests you added simply test the logic that is already present in $scope->getType(), not the code that's in the rule itself.

I'd be fine if just tested a few cases where general string is passed into require_once (no error reported) and a union of a couple of constant strings (which are checked against existing files).

Instead of these tests, we should thoroughly test the logic I quoted above.

Thanks.

Tests decluttered here 4ae5df5 and then a new test was added for the include paths bec70b8

[ondrejmirtes]

I appreciate your effort and your patience here! I bet you didn't expect this to be so much work but the bar of what can go into PHPStan is really high.

I'm pointing out all the issues I can think of, because from my experience it's really important. The users will report even more issues, issues we didn't even think of.

So please bear with me so we can bring this over the finish line. Thank you.

[VincentLanglet]

Shouldn't a test be added for the case

<?php declare(strict_types=1);

$path = __DIR__ . '/include-me-to-prove-you-work.txt';

if (file_exists('a-file-that-does-not-exist.php')) {
	$path = 'a-file-that-does-not-exist.php';
}

include $path;
include_once $path;
require $path;
require_once $path;

and

<?php declare(strict_types=1);

$path = __DIR__ . '/include-me-to-prove-you-work.txt';

if (rand(0,1)) {
	$path = 'a-file-that-does-not-exist.php';
}

if (file_exists($path)) {
    include $path;
    include_once $path;
    require $path;
    require_once $path;
}

because this shouldn't report any error ?

[Bellangelo]

Hi @VincentLanglet, I think what you are proposing is outside of the scope of the PR. It seems that PHPStan doesn't evaluate correctly if file_exists always returns true/false and as such it doesn't work.

Here is an example: https://phpstan.org/r/3e79db4e-40ab-46d7-9e6f-9dde7a850fe3 where you would expect the variable to always exist.

[VincentLanglet]

Hi @VincentLanglet, I think what you are proposing is outside of the scope of the PR. It seems that PHPStan doesn't evaluate correctly if file_exists always returns true/false and as such it doesn't work.

Here is an example: phpstan.org/r/3e79db4e-40ab-46d7-9e6f-9dde7a850fe3 where you would expect the variable to always exist.

In both of my example, the variable $path is always defined. That's very different from your example.

What you're trying to introduce is a rule which add a PHPStan error when include/require is used on the path if the path doesn't exist, regardless the path has been checked as existing !

Phpstan does the difference between
https://phpstan.org/r/a082ea0f-619e-4294-b909-eefa6ff7c43c
and
https://phpstan.org/r/16b7bed5-1a95-4534-a43b-3a2c02da11c3

it must the be same for the existence of files.

$path = __DIR__ . '/include-me-to-prove-you-work.txt';

if (file_exists('a-file-that-does-not-exist.php')) {
	$path = 'a-file-that-does-not-exist.php';
}

include $path; // Here I assume you have a phpstan error reported ; it shouldn't because path can only be `'a-file-that-does-not-exist.php'` if the file exists.

$path = __DIR__ . '/include-me-to-prove-you-work.txt';

if (rand(0,1)) {
	$path = 'a-file-that-does-not-exist.php';
}

if (file_exists($path)) {
    include $path; // Here I assume you have a PHPStan error reported, you shouldn't since `file_exists` check is done before.
}

[Bellangelo]

In both of my example, the variable $path is always defined. That's very different from your example.

My example showcases that PHPStan currently does not understand that file_exists(__FILE__) is always true and its variations. So, how can we know the type of the variable if PHPStan itself doesn't report it back to us correctly? Here is another example where PHPStan identifies correctly that if(1===1) is always true and as such it identifies the variable's type.
I found another example without the usage of file_exists which shows that it cannot identify correctly the conditions. It could be that we need to enable another option but I could be missing more than this.

[VincentLanglet]

I dunno why you quoting totally out of debate example with always true condition. (Especially the "isAlwaysTrue" method which is a misconception of how PHPStan works since it rely on the signature and not the implementation of the method).

Currently PHPStan has no issue with the include/require method since there is no rule about it. You are trying to include one ; all I am saying is the fact that you rule introduces false-positive. If you want to prove me wrong, just add the example I gave you in your tests.

You are throwing a PHPStan error when a constantString fileName does not exists ; but I can use some code on different environment (and the file might exist sometimes, and might not exist the other time). If I check for file_exist before include/require the file, PHPStan should not warn me with a false positive error.

[Bellangelo]

I dunno why you quoting totally out of debate example with always true condition. (Especially the "isAlwaysTrue" method which is a misconception of how PHPStan works since it rely on the signature and not the implementation of the method).

Currently PHPStan has no issue with the include/require method since there is no rule about it. You are trying to include one ; all I am saying is the fact that you rule introduces false-positive. If you want to prove me wrong, just add the example I gave you in your tests.

You are throwing a PHPStan error when a constantString fileName does not exists ; but I can use some code on different environment (and the file might exist sometimes, and might not exist the other time). If I check for file_exist before include/require the file, PHPStan should not warn me with a false positive error.

@VincentLanglet The examples are not out of debate. They show that the problem you are mentioning pre-exists of my new rule. The rule does not parse by itself the variables used in the include, it just uses PHPStan to get their values. This is what I have been trying to show you with my examples. My examples reproduce the problem you are mentioning but without the usage of the new rule.
And what is the problem? The problem is that PHPStan in the current setup does not handles correctly the variable types when used inside a if(callToFunction()).
You can check again the first example that I send you which uses the file_exists. As you can see, even if in my example the file_exists returns true, it still doesn't recognize that the variable exists. Here is also an updated version of it. As you can see, its dumped type is mixed and not string. All of these prove that the behaviour you are trying to test is in the PHPStan itself and not in the rule per se.
Should my rule do something differently to retrieve the real value of the variable?

[VincentLanglet]

@VincentLanglet The examples are not out of debate. They show that the problem you are mentioning pre-exists of my new rule. The rule does not parse by itself the variables used in the include, it just uses PHPStan to get their values. This is what I have been trying to show you with my examples. My examples reproduce the problem you are mentioning but without the usage of the new rule.

We definitely don't understand each others.

Can you please focus use the example I gave you ? I'm not concerned about the fact that some file_exist($path) condition are always true or always false (like you said, it's already the case). I'm concerned about the fact that your new rule about require/include introduce false positive.
Sure the current analyse from PHPStan about require, include, file_exist method (and certainly more) is not perfect, but it's better to have no error reported rather than introducing false positive. (Especially if this rule is enabled by default in bleeding edge / PHPStan 2.0)

As I understand your rule

require 'path-which-exists';

is OK and

require 'path-which-not-exists';

is reported as an error.

According to your test testFileDoesNotExistConditionally https://github.com/phpstan/phpstan-src/pull/3294/files#diff-c86e2fa4121a5853c7f1d13452c7599ef1e871bd07ddc9ccd93ee4e21a7617fdR67,

require 'path-which-exists'|'path-which-not-exists';

is supposed to be reported as an error, which would be https://phpstan.org/r/9a199041-399e-4254-8642-a92bdf12ca65

But with your rule, https://phpstan.org/r/c4083b8a-a572-4dc2-b48e-4e95e247aab7 will be thrown as an error which is wrong. It's better to have no rule about include/require rather than file positive because PHPStan doesn't understand file_exist file. Or it require to improve some TypeSpecifier/StringType (with maybe an accessory filename-string) things.

[Bellangelo]

@VincentLanglet Ok, I think we pretty much saying the same thing but we disagree on what should happen next. Your last comment makes it clear that you don't want this rule to be merged since it doesn't catch all cases. On the other hand, all I am saying is that this problem exists in all rules with similar cases, since it isn't the rule's fault, and as such you can create new rules with similar issues as this "bug" is expected.

[Bellangelo]

Hi @ondrejmirtes, how shall we proceed with this one? Is what VincentLanglet saying a valid concern or we can live without it? Should we schedule perhaps some next steps? For example, add support for file_exists?

[ondrejmirtes]

@Bellangelo I say we continue. Real-world user feedback after releasing this will tell if this is a real problem or not.

The way to address this problem would be to introduce a new string accessory type like file-string. A new AccessoryFileStringType could be attached to StringType or ConstantStringType in an IntersectionType. After asking for is_file() we could narrow the type of the argument, and skip reporting that in the rule.

[ondrejmirtes]

One last simple change: I worry the identifier cannot be statically inferred here because of str_replace. We need that for https://github.com/phpstan/phpstan/actions/workflows/extract-identifiers.yml which updates https://phpstan.org/error-identifiers.

Please assign $identifier = ... in the switch in each case, and use that in RuleErrorBuilder.

Then I'll merge this. Thank you for the effort!

[Bellangelo]

Fixed in e861017.

Shouldn't this also run as a quality check before merging a PR?

[ondrejmirtes]

Yes, feel free to contribute this rule 😊

[ondrejmirtes]

Thank you!

[ondrejmirtes]

More about the file-string type. I discussed it with @jbafford and he thinks that this concept can't be expressed with the typesystem. Because the fact about file existence can change between the calls.

if (is_file($path)) {
    require_once $path; // can fail, file might have been deleted
}

So reporting an error in such cases is actually a feature, not a bug.