-
Notifications
You must be signed in to change notification settings - Fork 584
Report constant-string arguments to #[JetBrains\PhpStorm\FileReference] parameters that do not resolve to an existing path
#6062
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
base: 2.2.x
Are you sure you want to change the base?
Changes from 1 commit
d049933
5dd79a6
c4b7539
dcc6592
4cd51e5
aab2b28
ac312c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\File; | ||
|
|
||
| use PHPStan\DependencyInjection\AutowiredParameter; | ||
| use PHPStan\DependencyInjection\AutowiredService; | ||
| use function array_merge; | ||
| use function explode; | ||
| use function get_include_path; | ||
| use function is_dir; | ||
| use function is_file; | ||
| use const PATH_SEPARATOR; | ||
|
|
||
| /** | ||
| * Resolves a possibly-relative path the same way PHP would at runtime and checks whether it exists. | ||
| * | ||
| * We cannot use stream_resolve_include_path() because it works based on the calling script. | ||
| * This mirrors its behavior but for an arbitrary script directory. The priority order is: | ||
| * 1. The base directories (e.g. from an attribute argument). | ||
| * 2. The current working directory. | ||
| * 3. The include path. | ||
| * 4. The directory of the script that is being analysed. | ||
| */ | ||
| #[AutowiredService] | ||
| final class FileExistenceChecker | ||
| { | ||
|
|
||
| public function __construct( | ||
| #[AutowiredParameter] | ||
| private string $currentWorkingDirectory, | ||
| ) | ||
| { | ||
| } | ||
|
|
||
| /** | ||
| * @param list<string> $baseDirectories | ||
| */ | ||
| public function fileExists(string $path, string $scriptDirectory, array $baseDirectories = []): bool | ||
| { | ||
| return $this->exists($path, $scriptDirectory, $baseDirectories, false); | ||
| } | ||
|
|
||
| /** | ||
| * Like fileExists(), but a directory at the resolved path also counts as existing. | ||
| * | ||
| * @param list<string> $baseDirectories | ||
| */ | ||
| public function pathExists(string $path, string $scriptDirectory, array $baseDirectories = []): bool | ||
| { | ||
| return $this->exists($path, $scriptDirectory, $baseDirectories, true); | ||
| } | ||
|
|
||
| /** | ||
| * @param list<string> $baseDirectories | ||
| */ | ||
| private function exists(string $path, string $scriptDirectory, array $baseDirectories, bool $allowDirectory): bool | ||
| { | ||
| $scriptHelper = new FileHelper($scriptDirectory); | ||
| $resolvedBaseDirectories = []; | ||
| foreach ($baseDirectories as $baseDirectory) { | ||
| // a relative base directory is resolved against the directory of the analysed script | ||
| $resolvedBaseDirectories[] = $scriptHelper->absolutizePath($baseDirectory); | ||
| } | ||
|
|
||
| $directories = array_merge( | ||
| $resolvedBaseDirectories, | ||
| [$this->currentWorkingDirectory], | ||
| explode(PATH_SEPARATOR, get_include_path()), | ||
| [$scriptDirectory], | ||
| ); | ||
|
|
||
| foreach ($directories as $directory) { | ||
| $absolutePath = (new FileHelper($directory))->absolutizePath($path); | ||
|
|
||
| if (is_file($absolutePath)) { | ||
| return true; | ||
| } | ||
|
|
||
| if ($allowDirectory && is_dir($absolutePath)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,20 +11,16 @@ | |
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\DependencyInjection\AutowiredParameter; | ||
| use PHPStan\DependencyInjection\RegisteredRule; | ||
| use PHPStan\File\FileExistenceChecker; | ||
| use PHPStan\File\FileHelper; | ||
| use PHPStan\Node\Printer\ExprPrinter; | ||
| use PHPStan\Rules\IdentifierRuleError; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
| use PHPStan\ShouldNotHappenException; | ||
| use PHPStan\Type\Constant\ConstantStringType; | ||
| use function array_merge; | ||
| use function dirname; | ||
| use function explode; | ||
| use function get_include_path; | ||
| use function is_file; | ||
| use function sprintf; | ||
| use const PATH_SEPARATOR; | ||
|
|
||
| /** | ||
| * @implements Rule<Include_> | ||
|
|
@@ -47,12 +43,11 @@ final class RequireFileExistsRule implements Rule | |
| ]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. memo to me: in case the above file-existence functions also get the https://youtrack.jetbrains.com/issue/WI-85516/FileReference-and-builtin-functions-like-isfile
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Here's what I did to address the reviewer's memo. The concernstaabm left a memo warning that if phpstorm-stubs eventually add I confirmed this is a real, reachable regression by redeclaring The fix (one logical commit)
Verification
Committed as One note for the reviewer: this covers the false-positive risk proactively. The separate memo about dropping the in-fixture |
||
|
|
||
| public function __construct( | ||
| #[AutowiredParameter] | ||
| private string $currentWorkingDirectory, | ||
| private ExprPrinter $exprPrinter, | ||
| #[AutowiredParameter(ref: '%featureToggles.magicDirInInclude%')] | ||
| private bool $checkMagicDirInInclude, | ||
| private FileHelper $fileHelper, | ||
| private FileExistenceChecker $fileExistenceChecker, | ||
| ) | ||
| { | ||
| } | ||
|
|
@@ -91,37 +86,9 @@ public function processNode(Node $node, Scope $scope): array | |
| return $errors; | ||
| } | ||
|
|
||
| /** | ||
| * We cannot use `stream_resolve_include_path` as it works based on the calling script. | ||
| * This method simulates the behavior of `stream_resolve_include_path` but for the given scope. | ||
| * The priority order is the following: | ||
| * 1. The current working directory. | ||
| * 2. The include path. | ||
| * 3. The path of the script that is being executed. | ||
| */ | ||
| private function doesFileExist(string $path, Scope $scope): bool | ||
| { | ||
| $directories = array_merge( | ||
| [$this->currentWorkingDirectory], | ||
| explode(PATH_SEPARATOR, get_include_path()), | ||
| [dirname($scope->getFile())], | ||
| ); | ||
|
|
||
| foreach ($directories as $directory) { | ||
| if ($this->doesFileExistForDirectory($path, $directory)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private function doesFileExistForDirectory(string $path, string $workingDirectory): bool | ||
| { | ||
| $fileHelper = new FileHelper($workingDirectory); | ||
| $absolutePath = $fileHelper->absolutizePath($path); | ||
|
|
||
| return is_file($absolutePath); | ||
| return $this->fileExistenceChecker->fileExists($path, dirname($scope->getFile())); | ||
| } | ||
|
|
||
| private function getErrorMessage(Include_ $node, string $filePath): IdentifierRuleError | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1128,6 +1128,14 @@ public static function getAdditionalConfigFiles(): array | |
| ]; | ||
| } | ||
|
|
||
| public function testFileReferenceAttribute(): void | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I addressed the review: ChangeRemoved Specifically:
The feature remains fully covered by the four rule tests ( Verification
Committed as |
||
| { | ||
| $errors = $this->runAnalyse(__DIR__ . '/data/file-reference-attribute.php'); | ||
| $this->assertCount(1, $errors); | ||
| $this->assertSame('Path "file-reference-attribute-missing.php" passed to parameter #1 $path does not exist.', $errors[0]->getMessage()); | ||
| $this->assertSame(26, $errors[0]->getLine()); | ||
| } | ||
|
|
||
| public function testBug8004(): void | ||
| { | ||
| // false positive | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| <?php declare(strict_types = 1); // lint >= 8.0 | ||
|
|
||
| namespace JetBrains\PhpStorm { | ||
|
|
||
| #[\Attribute(\Attribute::TARGET_PARAMETER)] | ||
| class FileReference | ||
| { | ||
|
|
||
| public function __construct(string $basePath = '') | ||
| { | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| namespace FileReferenceIntegration { | ||
|
|
||
| use JetBrains\PhpStorm\FileReference; | ||
|
|
||
| function loadFile(#[FileReference] string $path): void | ||
| { | ||
| } | ||
|
|
||
| loadFile('file-reference-attribute.php'); | ||
| loadFile('file-reference-attribute-missing.php'); | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.