Fix get_index_from_list returning -1 for negative start when value is found#5654
Open
sudheerr937-ai wants to merge 1 commit intorobotframework:masterfrom
Open
Conversation
pekkaklarck
requested changes
Apr 20, 2026
Member
pekkaklarck
left a comment
There was a problem hiding this comment.
The overall approach how to fix the issue looks good, but there are few things to be fixed.
| Get Index From List ${LONG} ${2} start=-1 expected=8 type=int | ||
| Get Index From List ${LONG} 43 start=-4 expected=5 type=int | ||
| Get Index From List ${LONG} nonex start=-3 expected=-1 type=int | ||
|
|
Member
There was a problem hiding this comment.
Adding new validation steps to this existing test is fine, but you could have also created a new test Get Index From List with negative indices or something like that. It would then need to be added to the matching file on the atest/robot side as well.
Please remove the unnecessary empty line.
| ) | ||
| start = 0 | ||
| if isinstance(start, int) and start < 0: | ||
| start = max(len(list_) + start, 0) |
Member
There was a problem hiding this comment.
startis known to beintat this point so theisinstancecheck is unnecessary.- Using
max(len(list_) + start), 0)looks a bit odd to me. Wouldn'tlen(list_) + start)be enough? I can see that if you use a negative index that's out-of-the-range (e.g. list has length 5 and index is -7) the latter can map the index to a value that is incorrect but valid (e.g. -2 with the earlier values), but usingmax(..., 0)just changes that to an other incorrect value. - I believe it would be better to map the
startindex to a positive value only after doingself.get_slice_from_list(...). In that case possible invalid indices (e.g. -7 when list has length 5) cause an error already then and afterwards we can safely dostart = len(list_) + startwith negative indices.
Member
|
Should we add a note to keyword documentation about negative start indices yielding invalid results prior to RF 7.5? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
get_index_from_list was returning -1 (not found) even when the value existed in the list, but only when a negative start like -1 or -2 was used.
Root cause: the math start + list_.index(value) accidentally produces -1 when start is negative and the value is at the end of the slice.
Fix: convert negative start to its real position in the list before doing the math
Added test cases in atest/testdata/standard_libraries/collections/list.robot covering negative start scenarios.
Fixes #5649