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

Allow for loop increment expression to use variable defined inside loop #262

Merged
merged 9 commits into from
Aug 10, 2022

Conversation

sirbrillig
Copy link
Owner

@sirbrillig sirbrillig commented Jul 4, 2022

In most cases, for loops do not require special processing by this sniff because they share the variable scope of their container. However, the third expression of the loop (what I'm calling the "increment expression") is technically not evaluated until after the loop runs at least once. This means that you could define a variable inside the loop that is then used in the increment expression, despite the definition coming lexically after the use.

In this PR, we modify the sniff to set aside variables in the increment expression until after the loop has been parsed. Variables found in this way are then parsed in their own operation (but in their original position).

Fixes #261

@jrfnl
Copy link
Collaborator

jrfnl commented Jul 4, 2022

Little heads-up for when you continue working on this: each "expression" segment in a for can contain multiple comma-separated expressions.
Example: https://3v4l.org/X3R8U

@sirbrillig sirbrillig force-pushed the allow-for-loop-increment-def-inside-loop branch 2 times, most recently from 75a0803 to 10b9396 Compare August 2, 2022 00:06
@sirbrillig
Copy link
Owner Author

Ugh, trying to find the semicolons that divide a for loop's initialization, condition, and increment expressions is really hard since each expression could contain a closure that itself contains semicolons. 😩

@jrfnl
Copy link
Collaborator

jrfnl commented Aug 3, 2022

Ugh, trying to find the semicolons that divide a for loop's initialization, condition, and increment expressions is really hard since each expression could contain a closure that itself contains semicolons. 😩

Haven't looked at your current code, but in case you aren't already, you may want to use something along the lines of this code to jump over closed structures: https://github.com/PHPCSStandards/PHPCSUtils/blob/b65fbd47c38202a667ea3930a4a51c5c8b9ca434/PHPCSUtils/Utils/PassedParameters.php#L243-L280 (That's the code I use to loop through and split out the parameters in a function call, where parameter values may contain the comma delimiter as well)

@jrfnl
Copy link
Collaborator

jrfnl commented Aug 3, 2022

You may also find this code useful to see: https://github.com/PHPCSStandards/PHPCSUtils/blob/b65fbd47c38202a667ea3930a4a51c5c8b9ca434/PHPCSUtils/Utils/Context.php#L152-L204 (utility which splits a for() into the three expressions)

@sirbrillig sirbrillig force-pushed the allow-for-loop-increment-def-inside-loop branch from d561cb7 to f06152f Compare August 8, 2022 19:32
@sirbrillig sirbrillig marked this pull request as ready for review August 8, 2022 19:40
@sirbrillig
Copy link
Owner Author

Thank you, @jrfnl! That was the key; I needed to check for the level of the tokens I was scanning.

@sirbrillig sirbrillig merged commit 1087f11 into 2.x Aug 10, 2022
@sirbrillig sirbrillig deleted the allow-for-loop-increment-def-inside-loop branch August 10, 2022 19:17
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.

UndefinedVariable first assigned in for loop, used in loop expression
2 participants