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

Bug fix: false negative unused var #120

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Jan 18, 2020

As reported in #111, if a variable is assigned a value with a combined assignment operator, it would not be reported as unused.

Includes unit tests.

Fixes #111

@jrfnl jrfnl force-pushed the feature/111-bug-fix-combined-assignment-operators branch 2 times, most recently from 5cfa7c8 to 65479d5 Compare January 19, 2020 18:47
@jrfnl
Copy link
Collaborator Author

jrfnl commented Jan 19, 2020

The build for this PR won't pass until #122 and #123 have been merged. I'll rebase after that.

As reported in 111, if a variable is assigned a value with a combined assignment operator, it would not be reported as unused.

Includes unit tests.

Fixes 111
@jrfnl jrfnl force-pushed the feature/111-bug-fix-combined-assignment-operators branch 2 times, most recently from cfa309c to a95ad2c Compare January 20, 2020 18:38
Now the `PHP_CodeSniffer\Util\Tokens` file is imported, the `Constant T_\w+ not found` errors should no longer be thrown.

However, it depends on the order in which the files are analysed as that file is not (yet) `use`d in all files using the `T_` constants.

If I remove the `ignoreErrors`, [the build fails](https://circleci.com/gh/sirbrillig/phpcs-variable-analysis/281) on those errors for the `VariableAnalysisSniff` file, while if I leave the exclusion in, [the build fails](https://circleci.com/gh/sirbrillig/phpcs-variable-analysis/283) on a "Ignored error not matched".

So, for now, I'm leaving the ignore pattern in place and turning `reportUnmatchedIgnoredErrors` off to allow the build to pass.
@jrfnl
Copy link
Collaborator Author

jrfnl commented Jan 20, 2020

Rebased.

I've added an additional commit to allow the build to pass:

PHPStan config: don't report unmatched errors

Now the PHP_CodeSniffer\Util\Tokens file is imported, the Constant T_\w+ not found errors should no longer be thrown.

However, it depends on the order in which the files are analysed as that file is not (yet) used in all files using the T_ constants.

If I remove the ignoreErrors, the build fails on those errors for the VariableAnalysisSniff file, while if I leave the exclusion in, the build fails on a "Ignored error not matched".

So, for now, I'm leaving the ignore pattern in place and turning reportUnmatchedIgnoredErrors off to allow the build to pass.

Copy link
Owner

@sirbrillig sirbrillig left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation. I love that there's a way to just check for assignment tokens.

@sirbrillig sirbrillig merged commit 4f8e440 into sirbrillig:master Jan 21, 2020
@jrfnl jrfnl deleted the feature/111-bug-fix-combined-assignment-operators branch January 21, 2020 00:30
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.

False negative: unused variable which has been changed
2 participants