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

Deduplicate loop analysis #10782

Open
wants to merge 2 commits into
base: 5.x
Choose a base branch
from

Conversation

theodorejb
Copy link
Contributor

Also fixed some risky truthy falsy comparisons.

@theodorejb
Copy link
Contributor Author

@weirdan Does anything prevent merging this?

@weirdan
Copy link
Collaborator

weirdan commented Mar 20, 2024

@theodorejb I've been meaning to play with it locally, but haven't found the time yet. One thing that caught my eye is the type change for $protected_var_ids - perhaps there's a reason for that, but it's not immediately apparent.

@theodorejb
Copy link
Contributor Author

theodorejb commented Mar 20, 2024

@weirdan array<string, bool|int> seems to be the correct type for $protected_var_ids, since it's assigned to from a property with the same name in LoopScope, which has that type. The LoopScope property has that type because in one place it's assigned to from an array merge with $assigned_var_ids, which has a type of array<string, int>.

Also remove some risky truthy falsy comparisons
@theodorejb
Copy link
Contributor Author

@weirdan I'm still hoping this can be reviewed/merged. It will make future contributions easier to make without introducing bugs or discrepancies.

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.

2 participants