Inconsistent behaviour findPrevious() vs findNext()

[jrfnl]

I've been tripped up by this a number of times already, so putting this out here for consideration.

If the $start and $end token positions you pass to findPrevious() are the same, the function will work as expected. However, the same can not be said about the findNext() method as in that case it will always return false.

This is caused by the findPrevious() method treating the end position as inclusive for ($i = $start; $i >= $end; $i--), while the findNext() method treats it as exclusive for ($i = $start; $i < $end; $i++).

There are two possible solutions for this:

• Either check for $start and $end being the same and adding 1 to the $end in that case before entering the loop. This will lead to the least difference with current behaviour. - PR Issue #1319: findnext inconsistency, option 1 #1320
• Or make the loop in findNext() inclusive as well. This might lead to more breakage, but would make the functions more consistent in use. - PR Issue #1319: findnext inconsistency, option 2 #1321

[jrfnl]

FYI: I've opened PRs for both options. Only one of the two should be merged.

[aik099]

Either would be considered a BC break and would require all sniffs (core and custom) to be changed make use of it.

I'm for inclusive option (the option 2 if I'm not mistaken), because at least code sniffs are doing:

• ($stackPtr + 1) when specifying start position for findNext
• ($stackPtr - 1) when specifying start position for findPrevious

[jrfnl]

would require all sniffs (core and custom) to be changed make use of it.

Not necessarily - for the core sniffs, I've already made the pertinent changes needed and included them with the PRs for this issue. Turned out only a few sniffs really needed changing to account for the change (at least based on the unit test results).

To be very specific:

For both options, usage of the findNext() function would need to be reviewed, but only when a non-null $end pointer is being passed.

For option 1:

Any usage of findNext() with an $end pointer in combination with $exclude= true which run the risk of the $start and $end pointer being the same, should be reviewed.
Those are the only ones which are affected by the change and only if the return value is not being evaluated further.

For example:

// Test code snippet
function foobar(){}

$next = $phpcsFile->findNext(T_WHITESPACE, ($tokens[$stackPtr]['close_parenthesis'] + 1), $tokens[$stackPtr]['scope_opener'], true);

The above returns false with the current code as the $start and $end pointer are the same. With patch 1 applied, it would return the pointer to the { scope opener.

if ( $next === false) { // <- this would be problematic
	return;
}

if ( $next !== false && $tokens[$next]['code'] === T_SOMETHING) { // <- this would be fine
	// do something;
}

For option 2:

Any changes which option 1 necessitated would need to be applied.
Additionally any usage of findNext() with an $end pointer in combination with a value for $types which includes the token type of the $end pointer and $exclude = false and which runs the risk of the $start and $end pointer being the same, should be reviewed.
Again, those are the only ones which are affected by the change and only if the return value is not being evaluated further.

Oh and of course, any work-arounds for this issue can be removed ($end + 1), though as long as the findNext() call does not pass $exclude = true or the token type of the $end pointer in $types with $exclude = false, that's not even really necessary.

[gsherwood]

My intention was actually to have these methods not be inclusive. If you are looking for a token before/after a specific one, and you want to stop early, I didn't think the stop token should be included. Obviously, I've accidentally coded these differently.

So a bigger question is: should findPrevious() actually be changed to match findNext()?

I could be swayed either way - it's just that findNext() was written first and I tend to think of them both working in that way. I'd like to know what others think.

Whatever happens, they should be made consistent. But it is a BC break and I'd like to keep it in 3.0 if possible. I'm trying (trying) to keep the BC breaks down for sniff developers in 2.x because it's getting towards the end, and 3.0 is a big BC break in itself.

[jrfnl]

In my opinion they should (obviously) both be inclusive.
To make a case for this: I've regularly come across sniffs which use a findNext() in a while loop and had to debug why these were halting PHPCS / going into an infinite loop. The answer always turned out to be a case of the $start and $end token being the same.

For sniff devs to always have to remember to add +1 to the end token to prevent that is far less intuitive than for the function to sort it out.

Regarding the BC break - I'd like to suggest the following: what about including option 1 in the last 2.x release and option 2 in the 3.x branch ? Option 2 is the real BC break, option 1 is more a "plaster to stop the bleeding" and will more than anything just prevent those infinite loops.

The branch against which the PRs are pulled can be changed easily, so let me know if there's anything I can do in that regards and/or regarding a more in-depth review of the potential BC-break cases in the sniffs which are natively included.

[aik099]

Regarding the BC break - I'd like to suggest the following: what about including option 1 in the last 2.x release and option 2 in the 3.x branch ? Option 2 is the real BC break, option 1 is more a "plaster to stop the bleeding" and will more than anything just prevent those infinite loops.

👎 for this, because:

1. no way to determine if given 2.x version would be the last one
2. custom sniff developers needs to add +1 conditionally based on phpcs version and since there can be many findNext calls in single sniff the sniff code would become ugly

Custom standard developers like myself can't guarantee that everyone using the standard would have specific phpcs version installed.

[jrfnl]

no way to determine if given 2.x version would be the last one

Sorry, should have read "one of the last"

custom sniff developers needs to add +1 conditionally based on phpcs version and since there can be many findNext calls in single sniff the sniff code would become ugly

As I outlined, that is only for Option2 which wouldn't go in the 2.x branch

Custom standard developers like myself can't guarantee that everyone using the standard would have specific phpcs version installed.

I completely concur, that's why I proposed the two step approach with the real BC break only in v3

[aik099]

If no BC break (even minor one), then I'm OK with merging to 2.x.

[jrfnl]

If no BC break (even minor one), then I'm OK with merging to 2.x.

@aik099 There is only a very very very minor BC break with Option 1 as outlined in more detail in #1319 (comment)

Conditions in which Option 1 would constitute a break:

• Using findNext()
• Passing same stack pointer to $start + $end pointer
• With exclude set to true
• And $types not including the type of the $end pointer
• And limited checking on the output of the function