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

Implement MagicClassConstantRule #2741

Merged
merged 18 commits into from
Nov 17, 2023
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 15, 2023

@@ -56,6 +56,7 @@ rules:
- PHPStan\Rules\Classes\NonClassAttributeClassRule
- PHPStan\Rules\Classes\TraitAttributeClassRule
- PHPStan\Rules\Constants\FinalConstantRule
- PHPStan\Rules\Constants\MagicClassConstantRule
Copy link
Member

Choose a reason for hiding this comment

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

This should be enabled only with bleeding edge. The code does not error, it just returns an empty string AFAIK.

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 have used a feature flag with a more general name, so I can use it for similar rules for __FUNCTION__ and __METHOD__


return [
RuleErrorBuilder::message(
sprintf('Magic constant %s cannot be used outside a class.', $node->getName()),
Copy link
Member

Choose a reason for hiding this comment

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

The message should be different. It just returns an empty string.

@ondrejmirtes
Copy link
Member

Can you also please go through other magic constants and report them when used in a wrong way? Maybe it can all be done inside this single rule.

@clxmstaab clxmstaab force-pushed the magicclass branch 2 times, most recently from 5ef5af0 to 92c94d4 Compare November 15, 2023 15:22
@staabm
Copy link
Contributor Author

staabm commented Nov 15, 2023

there is one interessting leftover case:

// test.php
function doFooParams(
	string $file = __FILE__,
	int $line = __LINE__,
	string $class = __CLASS__,
	string $dir = __DIR__,
	string $namespace = __NAMESPACE__,
	string $method = __METHOD__,
	string $function = __FUNCTION__,
	string $trait = __TRAIT__
): void
{
}

since used in the parameter-list and not the function body it false-positives with

https://3v4l.org/BbZWn

$ php bin/phpstan analyze test.php --debug
Note: Using configuration file C:\dvl\Workspace\phpstan-src\phpstan.neon.dist.
C:\dvl\Workspace\phpstan-src\test.php
 ------ -----------------------------------------------------------------------------
  Line   test.php
 ------ -----------------------------------------------------------------------------
  6      Magic constant __CLASS__ is always empty when used outside a class.
  8      Magic constant __NAMESPACE__ is always empty when used in global namespace.
  9      Magic constant __METHOD__ is always empty when used outside a function.
  10     Magic constant __FUNCTION__ is always empty when used outside a function.
 ------ -----------------------------------------------------------------------------

@staabm
Copy link
Contributor Author

staabm commented Nov 15, 2023

we could fix the above case by moving enterFunction before the processParamNode call in

foreach ($stmt->params as $param) {
$this->processParamNode($param, $scope, $nodeCallback);
}
if ($stmt->returnType !== null) {
$nodeCallback($stmt->returnType, $scope);
}
$functionScope = $scope->enterFunction(

wdyt?

Copy link
Contributor Author

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Instead of changing NodeScopeResolver I went with adding a dedicated new NodeVisitor to prevent the false positives

if ($scope->getNamespace() === null) {
return [
RuleErrorBuilder::message(
sprintf('Magic constant %s is always empty when used in global namespace.', $node->getName()),
Copy link
Member

Choose a reason for hiding this comment

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

Please deleted "when used" from all messages. They'll still make sense.


return [
RuleErrorBuilder::message(
sprintf('Magic constant %s is always empty when used outside a trait-using-class.', $node->getName()),
Copy link
Member

Choose a reason for hiding this comment

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

"outside a trait"

@clxmstaab clxmstaab force-pushed the magicclass branch 2 times, most recently from c1172f6 to 9191a31 Compare November 17, 2023 09:23
@staabm
Copy link
Contributor Author

staabm commented Nov 17, 2023

(btw: interessting that issuebot did not realize that the snippet from issue phpstan/phpstan#10099 gets a new error with this PR - locally it works as expected though)

)->build(),
];
} elseif ($node instanceof MagicConst\Method || $node instanceof MagicConst\Function_) {
// https://3v4l.org/3CAHm
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. What about private $foo = __METHOD__;?

Also a false positive here - you can have an anonymous function outside a class/function and it's going to report '{closure}'.

@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the magicclass branch November 17, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants