Skip to content
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

Improve NativeJavaList to behave more like NativeArray 2 #1083

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Nov 8, 2021

This PR replaces #830
(most of #830 is already meged)
This adds some fixes in sameZero and shallowEx that were discovered in testArrayIndexOf/lastIndexOf and testArrayIncludes

Also seems to make #561 obsolete, see #561 (comment)

@rPraml
Copy link
Contributor Author

rPraml commented Nov 9, 2021

@gbrail (or anyone else) can you re-run the CI builds. They failed, because https://twitter.com/toongeens/status/1458053129677819904
(or should I push an empty commit)

@tonygermano
Copy link
Contributor

I don't think this is correct. A java object should not be strictly equal to a javascript primitive.

The following behavior is observed with your changes

js> new String('foo') === 'foo' // should be false
false
js> new java.lang.String('foo') === 'foo' // should be false
true

@gbrail
Copy link
Collaborator

gbrail commented Nov 18, 2021

Thanks for working on this -- sounds like we need to have a bit more discussion on this.

I am going to hold off on merging this until after we have all had a chance to test 1.7.14 RC1 and I've been able to publish a non-RC version of 1.7.14. (I think that this project is too small for us to have branches for old releases that diverge from the trunk, and that kind of thing...)

@rPraml
Copy link
Contributor Author

rPraml commented Nov 19, 2021

@tonygermano thanks for feedback, I'm also unsure, which is correct or expected behaviour. I've made some tests at current master branch (1.7.14-RC1)

// Test 1:
var list = []
list[0] = 'foo'
list[1] = 'foo'
list[0] === list[1]; // returns true
list[2] = 3.1
list[3] = 3.1
list[2] === list[3]; // returns true


// Test 2:
var list = new java.util.ArrayList();
list[0] = 'averylongstring'
list[1] = 'averylongstring'
list[0] === list[1]; // returns true
list[2] = 3.1
list[3] = 3.1
list[2] === list[3]; // returns false in master branch
list[2] = list[3]
list[2] === list[3]; // returns true

// Test 3:
var list = []
list[0] = 'foo'
list[1] = 'f'
list[1] += 'oo'
list[0] === list[1]; // returns true

// Test 4:
var list = new java.util.ArrayList();
list[0] = 'foo'
list[1] = 'f'
list[1] += 'oo'
list[0] === list[1]; // returns false in master branch

The problem with the 'foo' string is, that the same object instance is put into the java.list, so list.get(0) == list.get(1) on the java side. This is not true if the string is a result of a computation or if you use a double value.
Then we run into the == check in shallowEq

            if (x instanceof Wrapper && y instanceof Wrapper) {
                return ((Wrapper) x).unwrap() == ((Wrapper) y).unwrap();
            }

What do you think is the best way to solve this issue - or should we accept it as a problem of liveConnect

@p-bakker p-bakker added the Java Interop Issues related to the interaction between Java and JavaScript label Nov 29, 2021
@p-bakker p-bakker added the Community input needed Issues requiring community input label May 9, 2022
@tonygermano
Copy link
Contributor

tonygermano commented Aug 22, 2024

If you set the javaPrimitiveWrap to false on the Context's WrapFactory, all of your tests in #1083 (comment) do what you expect without any code changes or breaking object comparisons.

It will end up returning the java Double and String objects as primitive numbers and strings instead so that the comparisons work as expected.

js> org.mozilla.javascript.Context.currentContext.wrapFactory.javaPrimitiveWrap = false
false
js> var list = new java.util.ArrayList();
js> list[0] = 'foo'
foo
js> list[1] = 'f'
f
js> list[1] += 'oo'
foo
js> list[0] === list[1]; // returns false in master branch
true
js> typeof list[1]
string
js> org.mozilla.javascript.Context.currentContext.wrapFactory.javaPrimitiveWrap = true // default setting
true
js> typeof list[1]
object

@p-bakker p-bakker marked this pull request as draft September 8, 2024 07:42
@p-bakker
Copy link
Collaborator

p-bakker commented Sep 8, 2024

Converted to draft, as currently failing the CI pipeline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community input needed Issues requiring community input Java Interop Issues related to the interaction between Java and JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants