Implement Iterator.remove() on OptimizedTagMap view iterators#11326
Closed
Implement Iterator.remove() on OptimizedTagMap view iterators#11326
Conversation
The view iterators returned by OptimizedTagMap.entrySet().iterator(), keySet().iterator(), values().iterator(), and the TagMap.iterator() overload all extend IteratorBase, which inherited the default Iterator.remove() that throws UnsupportedOperationException. As a result, AbstractSet/AbstractCollection bulk operations that rely on iterator-remove (removeAll, removeIf, retainAll) failed mid-iteration on optimized TagMaps. Track the last-returned entry on IteratorBase and remove by tag through OptimizedTagMap.getAndRemove, which enforces the frozen check. LegacyTagMap is unaffected; its view iterators are inherited from HashMap and already support remove. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Experiment - based on a Claude review, not quite what I had in mind |
Contributor
Author
|
Decided this isn't worth pursuing, would be better to just eliminate LegacyTagMap |
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.
Summary
The view iterators returned by
OptimizedTagMap.entrySet().iterator(),keySet().iterator(),values().iterator(), and theTagMap.iterator()overload all extendIteratorBase, which inherited the defaultIterator.remove()that throwsUnsupportedOperationException. As a result,AbstractSet/AbstractCollectionbulk operations that rely on iterator-remove (removeAll,removeIf,retainAll) silently failed mid-iteration on optimized maps — aMapcontract gap.This patch tracks the last-returned entry on
IteratorBaseand removes by tag throughOptimizedTagMap.getAndRemove, which enforces the frozen check. The fix is contained entirely inIteratorBase, so all three subclasses (EntryReaderIterator,KeysIterator,ValuesIterator) inheritremove()automatically.LegacyTagMapis unaffected: its view iterators are inherited fromHashMapand already support remove. (Its TagMap-leveliterator()does not, but that's a separate divergence — see follow-up work below.)Test plan
TagMapTest:entrySet_removeIf(parameterized over OPTIMIZED + LEGACY)keySet_removeAll(parameterized over OPTIMIZED + LEGACY)values_removeIf(parameterized over OPTIMIZED + LEGACY)optimizedIterator_removeoptimizedIterator_remove_acrossBucketCollisions(forces BucketGroup chains, then removes all entries via iterator)optimizedIterator_remove_beforeNext_throwsoptimizedIterator_remove_twice_throwsoptimizedIterator_remove_onFrozenMap_throws./gradlew :internal-api:testpasses./gradlew :internal-api:spotlessCheckpassesFollow-up not in scope
LegacyTagMap.IteratorImpl.remove()is also unsupported (would need to delegate to the wrapped HashMap iterator). Different mechanism, different PR.@EnumSource(TagMapType.class)test in isolation (e.g.--tests TagMapTest.numericZeroToBooleanCoercion) fails with NPE inTagMap.<clinit>becauseTagMapTypeloadsOptimizedTagMapFactory.INSTANCEbeforeTagMap. Reproduces on master without these changes; orthogonal to this PR.🤖 Generated with Claude Code