Skip to content

[Mono.Android] JavaList.Add should allow duplicates. #9751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Feb 4, 2025

Fixes: #9675

Remove the check for duplicates in JavaList.Add to allow duplicates.

@jpobst jpobst force-pushed the dev/jpobst/javalist-add branch from fdc6ffe to fae7cd1 Compare February 4, 2025 20:58
@jpobst jpobst marked this pull request as ready for review February 5, 2025 18:45
@jpobst jpobst requested a review from jonpryor as a code owner February 5, 2025 18:45
@jonpryor
Copy link
Contributor

jonpryor commented Feb 5, 2025

Draft commit message:

Fixes: https://github.com/dotnet/android/issues/9675

Context: https://github.com/xamarin/monodroid/commit/0e782afc16fe4b7197c800864d6c10520905fe28
Context: https://github.com/xamarin/monodroid/commit/8d38265ba00678ff3f3dd6edd8ceb10a97b55abd
Context: https://github.com/xamarin/monodroid/commit/be4087f038957a36a0bb3b9f9dc401c65ef379ca

Binding is a historically hairy business, and sometimes we live with
the results of those choices decades later.

Case in point: what should be done about `java.util.List<E>`?
Some time around 2010-Sep-7 in xamarin/monodroid@0e782afc was a
"Generic and Collections restructuring", which resulted in the
*removal* of `java.util.List` and other interfaces from
`Mono.Android.dll`.  The idea was that bindings would instead use
`Android.Runtime.JavaList` and other types instead.

This was the continued state of affairs when [Bugzilla 8112][0] was
filed, which was an issue binding DroidText, which was simplified
into [Bugzilla 8134][1].  The underlying problem was that some type
in DroidText implemented `java.util.List`, which wasn't bound and
instead was replaced with `JavaList`, but the generated bindings
would attempt to override and implement `java.util.List` methods
such as `Add()` on `JavaList`, which didn't exist, resulting in
compilation errors.

The fix in 2012-Nov-2 in xamarin/monodroid@8d38265b was to add all
`java.util.List` methods onto `JavaList`, and as part of this was an
overlooked bug within `JavaList.Add(int, Object)`: it had a
`Contains()` check before trying to add!

	partial class JavaList {
	  public virtual bool Add (int index, Java.Lang.Object item)
	  {
	    if (Contains (item))
	      return false;
	    Add ((object) item);
	    return true;
	  }
	}

This in turn meant that duplicate values couldn't be added, even
though that is something that Java would allow.

The underlying problem of "`java.util.List` isn't bound!" was later
resolved in 2013-Mar-11 by xamarin/monodroid@be4087f0, which stopped
removing `java.util.List` from `Mono.Android.dll`.

There is another issue with the above `JavaList.Add()` method: it
ignores the `index` parameter!  Fix that as well.

[0]: https://web.archive.org/web/20210922182648/https://bugzilla.xamarin.com/81/8112/bug.html
[1]: https://web.archive.org/web/20210925214551/https://bugzilla.xamarin.com/81/8134/bug.html

@jonpryor
Copy link
Contributor

jonpryor commented Feb 5, 2025

@jpobst: there is another issue issue here: we have an Add(int index, Java.Lang.Object? item) method, but index isn't actually used!

Which means if someone tried:

list.Add ("foo");
list.Add ("baz");
list.Add (1, "bar");

the list contents would be { "foo", "baz", "bar" } not { "foo", "bar", "baz" }.

list.Add (2, "Blueberry");
list.Add (4, "Fig");

Assert.AreEqual ("Apple", list [0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This new test is failing on CI:

Expected is <System.String>, actual is <Java.Lang.ICharSequenceInvoker>
Values differ at index [0]
Expected: 'A'
But was:  'C'

The SubList() test is also failing:

Expected is <System.String>, actual is <Java.Lang.ICharSequenceInvoker>
Values differ at index [0]
Expected: 'f'
But was:  'b'

Very odd. :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jonpryor jonpryor merged commit a9299a2 into main Feb 7, 2025
58 checks passed
@jonpryor jonpryor deleted the dev/jpobst/javalist-add branch February 7, 2025 16:32
grendello added a commit that referenced this pull request Feb 10, 2025
* main:
  [Mono.Android] `JNIEnv.FindClass()` uses `JniEnvironment.Types.FindClass()` (#9769)
  [Mono.Android] `JavaList.Add` should allow duplicates. (#9751)
  Bump to dotnet/java-interop@6bc87e8b (#9761)
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate values cannot be added to JavaList via Add(index, item)
2 participants