Skip to content

Fix JavaList.Remove() always-false guard condition (&& -> ||)#11237

Draft
jonathanpeppers wants to merge 2 commits intomainfrom
op-fix-javalist-remove-guard
Draft

Fix JavaList.Remove() always-false guard condition (&& -> ||)#11237
jonathanpeppers wants to merge 2 commits intomainfrom
op-fix-javalist-remove-guard

Conversation

@jonathanpeppers
Copy link
Copy Markdown
Member

Problem

JavaList.Remove(object?) had an always-false guard condition at line 395:

if (i < 0 && i >= Count)
    return;
RemoveAt (i);

A value cannot simultaneously be < 0 and >= Count (Count is always non-negative), so this guard could never fire. The practical consequence: when IndexOf returns -1 (item not found), the dead guard passes and RemoveAt(-1) is called, throwing an exception instead of silently no-op-ing as the IList.Remove contract requires.

Fix

Change && to ||:

if (i < 0 || i >= Count)
    return;
RemoveAt (i);

This correctly returns early when:

  • IndexOf returns -1 (item not in list) — the i < 0 branch
  • A race condition shrinks the list between IndexOf and RemoveAt — the i >= Count branch

Changes

  • src/Mono.Android/Android.Runtime/JavaList.cs: one-character fix (&&||)
  • tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaListTest.cs: added RemoveNonExistentItemDoesNotThrow test — adds "foo", calls Remove("bar"), asserts no exception is thrown and count remains 1

The guard condition if (i < 0 && i >= Count) was always false because
a value cannot simultaneously be negative and >= Count (which is
non-negative). This caused Remove() to call RemoveAt(-1) when an item
wasn't found, throwing an exception instead of silently no-op-ing as
IList.Remove requires.

Fix: change && to || so the guard correctly returns early when
IndexOf returns -1 (item not found) or when a race condition shrinks
the list between IndexOf and RemoveAt.

Add a test to verify Remove() with a non-existent item does not throw.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 28, 2026 14:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes JavaList.Remove(object?) to correctly no-op (per IList.Remove contract) when the item is not found, avoiding an out-of-range removal call.

Changes:

  • Corrected an always-false guard condition in JavaList.Remove (&&||) to prevent RemoveAt(-1).
  • Added an NUnit regression test verifying that removing a non-existent item does not throw and does not change the list count.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Mono.Android/Android.Runtime/JavaList.cs Fixes the guard in Remove(object?) so out-of-range indexes are safely ignored.
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaListTest.cs Adds regression coverage to ensure Remove is a no-op when the item is absent.

@jonathanpeppers jonathanpeppers marked this pull request as draft April 28, 2026 20:52
The original PR only fixed JavaList.Remove(object?), but the same
bug exists in JavaList.Remove(Java.Lang.Object?) and
JavaList<T>.Remove(T). The test calls list.Remove("bar") which
resolves to the Java.Lang.Object? overload via the implicit
string->Java.Lang.Object conversion operator.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

2 participants