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

Simplify detecting class properties to improve detection for anonymous classes #337

Merged
merged 3 commits into from
Nov 30, 2024

Conversation

sirbrillig
Copy link
Owner

Currently this sniff allows class properties to be unused without a warning. To determine if a variable it finds is a class property, it performs several checks; first it looks for a visibility keyword (eg: public) as the previous token. If that exists, then we assume it is a property. If that isn't found, it looks for a static keyword preceded by a visibility keyword. If there is no static, then it assumes there is no property access. If there is a static, we repeat the check for the visibility keyword. If there is a static token but no visibility keyword, we instead look to see if we are inside a class definition but not inside a function. If that is true, then we are a class property.

This logic is causing some problems for typed properties inside anonymous classes because the type keyword(s) are interfering with finding the visibility keyword. We can fix this by looking more carefully for the visibility keyword, like so:

		$startOfStatement = $phpcsFile->findPrevious([T_SEMICOLON, T_OPEN_CURLY_BRACKET], $stackPtr - 1);
		$stopAtPtr = is_bool($startOfStatement) ? 1 : $startOfStatement;
		$visibilityPtr = $phpcsFile->findPrevious($propertyDeclarationKeywords, $stackPtr - 1, $stopAtPtr > 0 ? $stopAtPtr : 0);

But that doesn't totally solve the problem for anonymous classes and I realized that we probably don't need to do it anyway. The last check that looks to see if we are inside a class definition but outside a function should be enough to find any property.

This PR removes all the logic described above and leaves only the final check.

Fixes #332

This defaults to only using the condition: if a variable is defined
inside a class and not inside a method within a class, then it's a
property.
@sirbrillig sirbrillig merged commit 04e3d23 into 2.x Nov 30, 2024
53 of 55 checks passed
@sirbrillig sirbrillig deleted the fix-anon-class-props-with-types branch November 30, 2024 17:28
@jrfnl
Copy link
Collaborator

jrfnl commented Nov 30, 2024

Won't this run foul for method parameters (which are within a class and not within a function scope) ?

@sirbrillig
Copy link
Owner Author

Won't this run foul for method parameters (which are within a class and not within a function scope) ?

I think it's safe because we look for function parameters before we look for class properties.

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.

Anonymous classes and Type declarations cause false positive for UndefinedVariable bug
2 participants