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

Allow finding arrow function variables when arrow function is in file scope #347

Merged
merged 5 commits into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Tests/VariableAnalysisSniff/ArrowFunctionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public function testArrowFunctionsWithoutUnusedBeforeUsed()
102,
112,
150,
184,
];
$this->assertSame($expectedWarnings, $lines);
}
Expand Down
11 changes: 11 additions & 0 deletions Tests/VariableAnalysisSniff/fixtures/ArrowFunctionFixture.php
Original file line number Diff line number Diff line change
Expand Up @@ -175,3 +175,14 @@ function arrowFunctionWithNestedArrowFunction() {
];
$fn();
}

// Arrow function in global scope
array_map(
fn(
$dir,
string $bar,
\My\Class|bool $foo, // Unused variable $foo
\My\Class|bool $baz,
) => PREFIX_DIR . $dir . $bar . $baz,
PHP_ERROR_LOGS['ignore_dirs']
);
5 changes: 3 additions & 2 deletions VariableAnalysis/Lib/Helpers.php
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,8 @@ public static function findVariableScope(File $phpcsFile, $stackPtr, $varName =
$varName = isset($varName) ? $varName : self::normalizeVarName($token['content']);

$enclosingScopeIndex = self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr);
if ($enclosingScopeIndex) {

if (!is_null($enclosingScopeIndex)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can self::findVariableScopeExceptArrowFunctions() return false ? Would a check for is_int() instead of !is_null() be more stable ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think it can return false. It returns null|int. It has only 3 return statements:

  • this one which is always an integer (guarded by is_int()).
  • this one which is also always an integer.
  • this one which is the return value of getStartOfTokenScope() when it's not an integer or less than or equal to 0. Since that function can return null or 0 or a key from the token conditions stack, I think it is always null|int, unless there's something I don't grok about conditions (can the key of a conditions array be false?).

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fair enough. I was mostly asking because the code within the condition seems to expect an int, while the condition doesn't actually strictly safeguard that.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's a good point. I think that is_int() is probably better for that reason, but I'm going to leave it for now since I already merged and I have to trust the types in the phpdoc, even if they're only enforced by static analysis tools.

I checked, and if I change the return type of findVariableScopeExceptArrowFunctions to int|null|false, I get this error:

Parameter #3 $enclosingScopeIndex of static method VariableAnalysis\Lib\Helpers::getContainingArrowFunctionIndex() expects int, int|false given.

So everything should be safe. Hooray for strong types!

$arrowFunctionIndex = self::getContainingArrowFunctionIndex($phpcsFile, $stackPtr, $enclosingScopeIndex);
$isTokenInsideArrowFunctionBody = is_int($arrowFunctionIndex);
if ($isTokenInsideArrowFunctionBody) {
Expand All @@ -446,7 +447,7 @@ public static function findVariableScope(File $phpcsFile, $stackPtr, $varName =
}
}

return self::findVariableScopeExceptArrowFunctions($phpcsFile, $stackPtr);
return $enclosingScopeIndex;
}

/**
Expand Down
123 changes: 61 additions & 62 deletions composer.json
Original file line number Diff line number Diff line change
@@ -1,64 +1,63 @@
{
"name": "sirbrillig/phpcs-variable-analysis",
"description": "A PHPCS sniff to detect problems with variables.",
"type": "phpcodesniffer-standard",
"keywords" : [ "phpcs", "static analysis" ],
"license": "BSD-2-Clause",
"authors": [
{
"name": "Sam Graham",
"email": "[email protected]"
},
{
"name": "Payton Swick",
"email": "[email protected]"
}
],
"support" : {
"issues": "https://github.com/sirbrillig/phpcs-variable-analysis/issues",
"wiki" : "https://github.com/sirbrillig/phpcs-variable-analysis/wiki",
"source": "https://github.com/sirbrillig/phpcs-variable-analysis"
},
"config": {
"sort-order": true,
"allow-plugins": {
"dealerdirect/phpcodesniffer-composer-installer": true
},
"lock": false
},
"autoload": {
"psr-4": {
"VariableAnalysis\\": "VariableAnalysis/"
}
},
"autoload-dev": {
"psr-4": {
"VariableAnalysis\\Tests\\": "Tests/"
}
},
"minimum-stability": "dev",
"prefer-stable": true,
"scripts": {
"test": "./vendor/bin/phpunit --no-coverage",
"coverage": "./vendor/bin/phpunit",
"test-lte9": "./vendor/bin/phpunit -c phpunitlte9.xml.dist --no-coverage",
"coverage-lte9": "./vendor/bin/phpunit -c phpunitlte9.xml.dist",
"lint": "./vendor/bin/phpcs",
"fix": "./vendor/bin/phpcbf",
"phpstan": "./vendor/bin/phpstan analyse",
"psalm": "./vendor/bin/psalm --no-cache",
"static-analysis": "composer phpstan && composer psalm"
},
"require" : {
"php" : ">=5.4.0",
"squizlabs/php_codesniffer": "^3.5.6"
},
"require-dev": {
"phpunit/phpunit": "^4.8.36 || ^5.7.21 || ^6.5 || ^7.0 || ^8.0 || ^9.0 || ^10.5.32 || ^11.3.3",
"sirbrillig/phpcs-import-detection": "^1.1",
"phpcsstandards/phpcsdevcs": "^1.1",
"phpstan/phpstan": "^1.7",
"dealerdirect/phpcodesniffer-composer-installer": "^0.7 || ^1.0",
"vimeo/psalm": "^0.2 || ^0.3 || ^1.1 || ^4.24 || ^5.0"
}
"name": "sirbrillig/phpcs-variable-analysis",
"description": "A PHPCS sniff to detect problems with variables.",
"type": "phpcodesniffer-standard",
"keywords": ["phpcs", "static analysis"],
"license": "BSD-2-Clause",
"authors": [
{
"name": "Sam Graham",
"email": "[email protected]"
},
{
"name": "Payton Swick",
"email": "[email protected]"
}
],
"support": {
"issues": "https://github.com/sirbrillig/phpcs-variable-analysis/issues",
"wiki": "https://github.com/sirbrillig/phpcs-variable-analysis/wiki",
"source": "https://github.com/sirbrillig/phpcs-variable-analysis"
},
"config": {
"sort-order": true,
"allow-plugins": {
"dealerdirect/phpcodesniffer-composer-installer": true
},
"lock": false
},
"autoload": {
"psr-4": {
"VariableAnalysis\\": "VariableAnalysis/"
}
},
"autoload-dev": {
"psr-4": {
"VariableAnalysis\\Tests\\": "Tests/"
}
},
"minimum-stability": "dev",
"prefer-stable": true,
"scripts": {
"test": "./vendor/bin/phpunit --no-coverage",
"coverage": "./vendor/bin/phpunit",
"test-lte9": "./vendor/bin/phpunit -c phpunitlte9.xml.dist --no-coverage",
"coverage-lte9": "./vendor/bin/phpunit -c phpunitlte9.xml.dist",
"lint": "./vendor/bin/phpcs",
"fix": "./vendor/bin/phpcbf",
"phpstan": "./vendor/bin/phpstan analyse",
"psalm": "./vendor/bin/psalm --no-cache",
"static-analysis": "composer phpstan && composer psalm"
},
"require": {
"php": ">=5.4.0",
"squizlabs/php_codesniffer": "^3.5.6"
},
"require-dev": {
"phpunit/phpunit": "^4.8.36 || ^5.7.21 || ^6.5 || ^7.0 || ^8.0 || ^9.0 || ^10.5.32 || ^11.3.3",
"phpcsstandards/phpcsdevcs": "^1.1",
"phpstan/phpstan": "^1.7",
"dealerdirect/phpcodesniffer-composer-installer": "^0.7 || ^1.0",
"vimeo/psalm": "^0.2 || ^0.3 || ^1.1 || ^4.24 || ^5.0"
}
}
3 changes: 1 addition & 2 deletions phpcs.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

<!--
#############################################################################
USE THE PHPCSDev, ImportDetection and VariableAnalysis RULESETS
USE THE PHPCSDev, VariableAnalysis RULESETS
#############################################################################
-->

Expand Down Expand Up @@ -59,7 +59,6 @@
<exclude name="Generic.Files.LineLength.TooLong" />
</rule>

<rule ref="ImportDetection" />
<rule ref="VariableAnalysis"/>


Expand Down
Loading