Skip to content

Support constructor promotion with namespaced and union typehints #333

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

Merged
merged 8 commits into from
Nov 17, 2024
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
11 changes: 11 additions & 0 deletions Tests/VariableAnalysisSniff/fixtures/ClassWithMembersFixture.php
Original file line number Diff line number Diff line change
Expand Up @@ -222,3 +222,14 @@ public function getMessage(): string {
return $this->message;
}
}

class ClassWithNamespacedConstructorPropertyPromotion
{
public function __construct(
public \App\Models\User $user,
public readonly \App\Models\Blog $blog,
private \App\Models\Game $game,
protected ?\App\Models\Flag $flag,
protected true|false|int|string|null|\App\Models\Favorite $favorite,
) {}
}
113 changes: 78 additions & 35 deletions VariableAnalysis/Lib/Helpers.php
Original file line number Diff line number Diff line change
Expand Up @@ -1535,62 +1535,105 @@ public static function getForLoopForIncrementVariable($stackPtr, $forLoops)
*/
public static function isConstructorPromotion(File $phpcsFile, $stackPtr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sirbrillig Just out of curiousity: why are you not using the File::getMethodParameters() to parse the function signature ? That would allow for just checking if the parameter has a 'property_visibility' index to know whether it is a promoted property...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because I didn't know about File::getMethodParameters() 😅 That does sound much easier! TIL there's helpers like this inside the source code.

Related: I really need to get around to adding phpcsutils to this package; you've done a fantastic job with its documentation!

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you didn't know about this one... you might also be interested in the File::getMemberProperties() (for declared, non constructor promoted, properties in a class)...

And yes, PHPCSUtils has those too and improved on them further + much more.

Happy to talk things through with you at some point if you think that would help. Unfortunately won't have any time in the foreseeable future to actually help you with the code changes.

{
// If we are not in a function's parameters, this is not promotion.
$functionIndex = self::getFunctionIndexForFunctionParameter($phpcsFile, $stackPtr);
if (! $functionIndex) {
return false;
}

$tokens = $phpcsFile->getTokens();

// If the previous token is a visibility keyword, this is constructor
// promotion. eg: `public $foobar`.
$prevIndex = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($stackPtr - 1), $functionIndex, true);
if (! is_int($prevIndex)) {
// Move backwards from the token, ignoring whitespace, typehints, and the
// 'readonly' keyword, and return true if the previous token is a
// visibility keyword (eg: `public`).
for ($i = $stackPtr - 1; $i > $functionIndex; $i--) {
if (in_array($tokens[$i]['code'], Tokens::$scopeModifiers, true)) {
return true;
}
if (in_array($tokens[$i]['code'], Tokens::$emptyTokens, true)) {
continue;
}
if ($tokens[$i]['content'] === 'readonly') {
continue;
}
if (self::isTokenPartOfTypehint($phpcsFile, $i)) {
continue;
}
return false;
}
$prevToken = $tokens[$prevIndex];
if (in_array($prevToken['code'], Tokens::$scopeModifiers, true)) {
return false;
}

/**
* Return false if the token is definitely not part of a typehint
*
* @param File $phpcsFile
* @param int $stackPtr
*
* @return bool
*/
private static function isTokenPossiblyPartOfTypehint(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$token = $tokens[$stackPtr];
if ($token['code'] === 'PHPCS_T_NULLABLE') {
return true;
}

// If the previous token is not a visibility keyword, but the one before it
// is, the previous token was probably a typehint and this is constructor
// promotion. eg: `public boolean $foobar`.
$prev2Index = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevIndex - 1), $functionIndex, true);
if (! is_int($prev2Index)) {
return false;
if ($token['code'] === T_NS_SEPARATOR) {
return true;
}
$prev2Token = $tokens[$prev2Index];
// If the token that might be a visibility keyword is a nullable typehint,
// ignore it and move back one token further eg: `public ?boolean $foobar`.
if ($prev2Token['code'] === 'PHPCS_T_NULLABLE') {
$prev2Index = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev2Index - 1), $functionIndex, true);
if (! is_int($prev2Index)) {
return false;
}
if ($token['code'] === T_STRING) {
return true;
}
$prev2Token = $tokens[$prev2Index];
if (in_array($prev2Token['code'], Tokens::$scopeModifiers, true)) {
if ($token['code'] === T_TRUE) {
return true;
}

// If the previous token is not a visibility keyword, but the one two
// before it is, and one of the tokens is `readonly`, the previous token
// was probably a typehint and this is constructor promotion. eg: `public
// readonly boolean $foobar`.
$prev3Index = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prev2Index - 1), $functionIndex, true);
if (! is_int($prev3Index)) {
return false;
if ($token['code'] === T_FALSE) {
return true;
}
$prev3Token = $tokens[$prev3Index];
$wasPreviousReadonly = $prevToken['content'] === 'readonly' || $prev2Token['content'] === 'readonly';
if (in_array($prev3Token['code'], Tokens::$scopeModifiers, true) && $wasPreviousReadonly) {
if ($token['code'] === T_NULL) {
return true;
}
if ($token['content'] === '|') {
return true;
}
if (in_array($token['code'], Tokens::$emptyTokens)) {
return true;
}

return false;
}

/**
* Return true if the token is inside a typehint
*
* @param File $phpcsFile
* @param int $stackPtr
*
* @return bool
*/
public static function isTokenPartOfTypehint(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();

if (! self::isTokenPossiblyPartOfTypehint($phpcsFile, $stackPtr)) {
return false;
}

// Examine every following token, ignoring everything that might be part of
// a typehint. If we find a variable at the end, this is part of a
// typehint.
$i = $stackPtr;
while (true) {
$i += 1;
if (! isset($tokens[$i])) {
return false;
}
if (! self::isTokenPossiblyPartOfTypehint($phpcsFile, $i)) {
return ($tokens[$i]['code'] === T_VARIABLE);
}
}
}

/**
* Return true if the token is inside an abstract class.
*
Expand Down
Loading