Fix SubList.get() accepting an index equal to its own size - #12656
Open
renechoi wants to merge 1 commit into
Open
Fix SubList.get() accepting an index equal to its own size#12656renechoi wants to merge 1 commit into
renechoi wants to merge 1 commit into
Conversation
The bounds check used `index > size()`, so `get(size())` fell through to
`AbstractImmutableList.this.get(fromIndex + index)`. When the sub list stops
before the end of the backing list that resolves to a real element outside the
sub list and is returned without any error: `copy(List.of("a","b","c")).subList(0, 1).get(1)`
answers "b". Only a sub list reaching the end of its parent fails, and then with
an ArrayIndexOutOfBoundsException from the backing array rather than the
IndexOutOfBoundsException the class builds for this case.
Applied to the mdo template and to both generated copies, otherwise the next
regeneration reintroduces it.
listIterator(int) keeps `index > size()` a few lines above. That bound is
correct there, since List.listIterator(int) accepts size() as a cursor
position, and a test pins it so the two are not aligned by mistake later.
Fixes apache#12595
gnodet
approved these changes
Aug 2, 2026
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Clean, correct off-by-one bug fix in SubList.get() bounds check. The fix properly changes index > size() to index >= size() to align with the java.util.List contract, which specifies that get(size()) must throw IndexOutOfBoundsException.
Key observations:
- All three copies of the file are fixed (template + both generated copies), preventing reintroduction on code generation
- The
listIterator(int index)method correctly retainsindex > size()sinceList.listIterator(size())is a valid cursor position - The four new test cases are well-chosen: they reproduce both bug variants, verify normal iteration, and pin the listIterator contract
- This is the first test class for
ImmutableCollections, adding coverage to a previously untested area
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
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
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.
SubList.get()checkedindex > size(), so an index equal to the sub list's size passed and reachedAbstractImmutableList.this.get(fromIndex + index).The issue describes this as surfacing an
IndexOutOfBoundsExceptionfrom the parent instead of a clear one from the sub list. That is what happens when the sub list reaches the end of its parent, and the exception is anArrayIndexOutOfBoundsExceptionfrom the backing array. When the sub list stops earlier there is no exception at all: the parent resolves the index and returns an element the sub list does not contain.ListN.get()indexes the array directly and has no bounds check of its own, which is why a read past the sub list is silent.The same class is checked in three times: the mdo template plus the two copies generated from it under
api/maven-api-xmlandimpl/maven-xml. All three are fixed, since changing only the copies would let the next regeneration bring it back.listIterator(int)a few lines above keepsindex > size(). That bound is correct there becauseList.listIterator(int)acceptssize()as a cursor position, so one of the added tests pins it to prevent the two being aligned later by mistake.Tests: four cases in a new
ImmutableCollectionsTestinimpl/maven-xml, covering the equal-to-size index, the read past a sub list that ends early, negative indices, and thelistIteratorbound. Two of them fail onmaven-4.0.xand pass with this change; the module suite is green at 75 tests with checkstyle and spotless enabled.Fixes #12595