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

Treat String, ConsString, Boolean, and Double as value types #1302

Merged
merged 3 commits into from
Apr 9, 2023

Conversation

szegedi
Copy link
Contributor

@szegedi szegedi commented Feb 7, 2023

I've been helping some folks that rely heavily in continuations comparisons in their product, and they had weird failures to compare continuations as equal. Turns out it's mostly due to differences in deserialized objects not having their constructor names etc. interned. The solution ultimately is to (correctly) consider JavaScript strings, booleans, and numbers to not have identity but treat them as purely value types, in line with the JS language semantics.

Closes #1192

return o1.toString().equals(o2.toString());
}
return false;
} else if (o1 instanceof Boolean || o1 instanceof Double) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think internally any java.lang.Number can represent a javascript primitive number, with the exception of BigInteger, which is a javascript bigint.

Copy link
Contributor Author

@szegedi szegedi Feb 8, 2023

Choose a reason for hiding this comment

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

Are you sure about that? Even otherwise integer literals will end up being represented as doubles in Rhino, e.g.

import org.mozilla.javascript.Context;

public class TestNumbers {
    public static void main(String[] args) {
        try (var cx = Context.enter()) {
            cx.setOptimizationLevel(9);
            var scope = cx.initStandardObjects();
            var result = cx.evaluateString(scope, "1", "", 1, null);
            System.out.println(result.getClass().getName());
        }
    }
}

prints:

java.lang.Double

(Notice I used the most aggressive optimization level.) I could add additional types that we certainly know the engine itself produces internally, but I wouldn't blanket treat all Number subclasses especially since there can be user-defined subclasses too.

@szegedi
Copy link
Contributor Author

szegedi commented Feb 17, 2023

Okay, I modified the code to use by-value comparison for all Java primitive type wrappers.

Comment on lines +43 to +45
Collections.unmodifiableSet(
new HashSet<>(
Arrays.asList(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the closest we can get to a Set.of in a pre-9 Java; currently Rhino still compiles for 8.

@rbri rbri merged commit add208c into mozilla:master Apr 9, 2023
@rbri
Copy link
Collaborator

rbri commented Apr 9, 2023

Thanks....

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.

Possible bug in EqualObjectGraphs
3 participants