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

Improve handling of unsupported references (fixes #8018). #8022

Conversation

AndrolGenhald
Copy link
Collaborator

Fixes the reference analysis to report references it's unable to analyze and continue as if they weren't references, rather than aborting analysis altogether.

Comment on lines +978 to +985
if (!$stmt->var instanceof ArrayDimFetch && !$stmt->var instanceof PropertyFetch) {
// If left-hand-side is an array offset or object property, usage is too difficult to track,
// so it's not registered as an unused variable (this mirrors behavior for non-references).
$statements_analyzer->registerVariableAssignment(
$lhs_var_id,
$lhs_location
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It turns out the bug this pull request fixes actually affected this method, causing everything after $context->vars_in_scope[$lhs_var_id] = &$context->vars_in_scope[$rhs_var_id]; to be ignored.

After it was fixed, that same line was reported as an unused variable, so I finally gave up and just decided that putting a reference in an object or an array is a bad idea anyway, and it's not worth it to track usage in that case (this is basically increasing the scope of the change in #7684 from only ignoring unused references on $this to ignoring unused references on any object or array).

@AndrolGenhald AndrolGenhald added the release:fix The PR will be included in 'Fixes' section of the release notes label May 26, 2022
@orklah
Copy link
Collaborator

orklah commented May 27, 2022

Thanks!

@orklah orklah merged commit ec9a999 into vimeo:master May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants