Can you please add a comment on the PR or in the code that explains what you're trying to fix here, or what bug is being addressed? That'll help us remember why we did this. Thanks!

Can you please add a comment on the PR or in the code that explains what you're trying to fix here, or what bug is being addressed? That'll help us remember why we did this. Thanks!

I think I put a link to the detailed issue that I have opened in the commit message, but I forgot to put it in the PR 😅
It's intended to fix #1402

So in #1402 you provide 3 snippets and the last one is about stuff erroring out.

Am unsure if you're saying that that code errors out in the latest version of Rhino, or that you expect it to error out when running with a languageVersion < 1.3.

Regardless, I'm unsure why you made a change to ScriptableObject, because I haven't seen anything not working properly without that change in the current version of Rhino

So in #1402 you provide 3 snippets and the last one is about stuff erroring out.

Am unsure if you're saying that that code errors out in the latest version of Rhino, or that you expect it to error out when running with a languageVersion < 1.3.

Regardless, I'm unsure why you made a change to ScriptableObject, because I haven't seen anything not working properly without that change in the current version of Rhino

So, the first two snippets are about "weird" behavior in language version < 1.3. Unfortunately, these are things that we still need to support in ServiceNow.

The change to ScriptableObject is required because of this snippet in ScriptRuntime::toBoolean:

                if (Context.getContext().isVersionECMA1()) {
                    // pure ECMA
                    return true;
                }
                // ECMA extension
                val = ((Scriptable) val).getDefaultValue(BooleanClass);
                if ((val instanceof Scriptable) && !isSymbol(val))
                    throw errorWithClassName("msg.primitive.expected", val);

It is invoking getDefaultValue with the hint boolean. My change simply makes this return true, because we were getting the error running this (admittedly weird) JS as detailed in #1402:

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

as getDefaultValue was trying to invoke valueOf and then toString, and weird things were happening. With this change, we short-circuit that problem, and also make it faster.

The end result is that:

• for legacy version (< 1.3) we preserve the behavior that Rhino had for Boolean and arrays, but we fix the problem with the JS above
• for all other versions, ScriptRuntime already skips the call to getDefaultValue, so nothing changes.

The Boolean case actually still works because of

The [] case should be handled by your changes to NativeArray, which clearly limits its impact to those specific version levels. Though should you be using Context.getContext().isVersionECMA1() as in ScriptRuntime::toBoolean rather than testing the version numbers explicitly? Your code will affect version 0, and I don't think that it should.

Was there ever a time when the var x = new Extending() code wouldn't have thrown an error that would justify the change to ScriptableObject?

I mean, I don't see any evidence going back to the initial commit of this repo 23 years ago that the array case has ever worked as you're describing. I don't actually know how to build the project from back then to test it, though. It appears to be using Ant, but I think there is some magic that I do not know how to perform.

@andreabergia in which exact Rhino version did you see the different behavior compared to the latest version?

@tonygermano all binaries from Rhino 1.4R3 onwards are available from our docs: https://rhino.github.io/releases/

I was unable to get the shell to launch with 1.4R3.

In version 1.5R1 !![] evaluates to false for versions 100 and 110, but true for 120 and higher. This likely warrants more research.

This code

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

...behaves exactly as the current version and throws a TypeError for versions 100, 110, and 120. I think it would be incorrect to change this behavior now.

The most current snapshot behaves exactly the same as version 1.5R1. I do not think this PR is necessary.

!![] is false for versions 100 and 110 and true for all other versions.

The reason that version 120 differs from 100 and 110 is because

1. ScriptableObject::getDefaultValue tries calling valueOf on the array
2. This goes up the chain to Object.prototype.valueOf
3. This method simply returns the object on which it's called
4. ScriptableObject::getDefaultValue sees this is not a primitive value, so it tries calling toString on the array
5. Only in version 120 does Array.prototype.toString return "[]" rather than an empty string when the array is empty
6. ScriptRuntime::toBoolean then converts the empty string to false for versions 100 and 110 and "[]" to true for version 120

In addition to !![] the same behavior can be observed with Boolean([])

The [] case should be handled by your changes to NativeArray, which clearly limits its impact to those specific version levels. Though should you be using Context.getContext().isVersionECMA1() as in ScriptRuntime::toBoolean rather than testing the version numbers explicitly? Your code will affect version 0, and I don't think that it should.

Good point, I need to fix that.

In version 1.5R1 !![] evaluates to false for versions 100 and 110, but true for 120 and higher. This likely warrants more research.

Yes, this is what happens. We rely on that at ServiceNow.

This code

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

...behaves exactly as the current version and throws a TypeError for versions 100, 110, and 120. I think it would be incorrect to change this behavior now.

I disagree, without the change in ScriptableObject that does work for newer versions. This unit test passes with VERSION_ES6, but fails with VERSION_1_0:

    @Test
    public void codeCanBeRunWithoutRaisingErrorInModernMode() {
        Utils.runWithAllOptimizationLevels(
                cx -> {
                    cx.setLanguageVersion(Context.VERSION_ES6);

                    Scriptable scope = cx.initStandardObjects(null);
                    Object result =
                            cx.evaluateString(
                                    scope,
                                    "var Base = function() {};\n"
                                            + "var Extending = function() {};\n"
                                            + "Extending.prototype = Base;\n"
                                            + "var x = new Extending();\n"
                                            + "!!x\n",
                                    "test",
                                    1,
                                    null);
                    assertEquals(true, result);
                    return null;
                });
    }

I think your assumptions are wrong, and @p-bakker asked you to provide the exact version where rhino functioned the way you think it should.

When I pulled and tested on a version over 20 years old, it behaves in exactly the same way as a snapshot build from the master branch for all of your test cases.

I understand that you desire codeCanBeRunWithoutRaisingErrorInLegacyMode to pass, but it should fail or you could be introducing regression bugs for someone else.

Likewise emptyArraysCoerceToFalseInLegacyModealready passes without your changes. The key here is that if you want that behavior, you need to be using versions 100 or 110. That test should historically fail for version 120 for the reason I mentioned in my previous comment.

I think your assumptions are wrong, and @p-bakker asked you to provide the exact version where rhino functioned the way you think it should.

When I pulled and tested on a version over 20 years old, it behaves in exactly the same way as a snapshot build from the master branch for all of your test cases.

I understand that you desire codeCanBeRunWithoutRaisingErrorInLegacyMode to pass, but it should fail or you could be introducing regression bugs for someone else.

Likewise emptyArraysCoerceToFalseInLegacyModealready passes without your changes. The key here is that if you want that behavior, you need to be using versions 100 or 110. That test should historically fail for version 120 for the reason I mentioned in my previous comment.

I am doubtful there ever was a version of rhino where this worked. But, this is causing problems for us at ServiceNow, so we went and fixed it in our internal fork.
Since it looks like a generic bug that should not cause any regressions (it does not cause any problem in our big internal test suite), and since we don't really think many people other than us use version 1.0-1.2 of Rhino, we thought it might be good to get it fixed in upstream, as well.

So, which Rhino version does ServiceNow use? The 'our internal fork' link just links to documentation, not a forked repo. We'd like to know, because you seem to suggest behavior changed in the latest version compared to some earlier version of Rhino, but @tonygermano checked and wasnt able to find any Rhino version that behaved any different than the current master

Curious though: the docs you pointed to suggests your users can use ES2021 features that Rhino doesn't support. Are you cross-compiling or has ServiceNow implemented those features in their own Rhino fork?

So, which Rhino version does ServiceNow use? The 'our internal fork' link just links to documentation, not a forked repo. We'd like to know, because you seem to suggest behavior changed in the latest version compared to some earlier version of Rhino, but @tonygermano checked and wasnt able to find any Rhino version that behaved any different than the current master

Ok, I realize I have been confusing. I meant "engine version, as in 1.0 and so on", not "rhino version" as in release 1.7.11 or so.
So, I don't think that the snippet of code I put in the issue has ever worked in any release of Rhino. But we need to fix it when Rhino is executing code with the context version set to 1.0. Does that clarify?

Curious though: the docs you pointed to suggests your users can use ES2021 features that Rhino doesn't support. Are you cross-compiling or has ServiceNow implemented those features in their own Rhino fork?

I am unsure if my NDA allows me to tell you what we are doing... 😢 I'll try to find out what I can share.

I am unsure if my NDA allows me to tell you what we are doing... 😢 I'll try to find out what I can share.

K, as I said, just curious. If ServiceNow has implemented all those features in Rhino, I'd really like to know more about it, but am assuming cross-compiling is used.

Looks like !![] evaluating to false is the correct behavior, as JavaScript 1.2 is the Netscape JavaScript implementation that predates EcmaScript 1.0 and this particular behavior is something where Netscapse's JavaScript 1.2 implementation differs from EcmaScript 1.0 specification, see https://docstore.mik.ua/orelly/webprog/jscript/ch11_06.htm#jscript4-CHP-11-SECT-6

Looks like !![] evaluating to false is the correct behavior, as JavaScript 1.2 is the Netscape JavaScript implementation that predates EcmaScript 1.0 and this particular behavior is something where Netscapse's JavaScript 1.2 implementation differs from EcmaScript 1.0 specification, see https://docstore.mik.ua/orelly/webprog/jscript/ch11_06.htm#jscript4-CHP-11-SECT-6

That's an interesting document. I scoured the Netscape documentation (which was terrible) in archive.org and couldn't find anything to support the following. It was also missing from the MDN document in "what's new in 1.2." Yet we do follow this behavior, anyway, with regard to converting arrays to numbers in 1.2 (and only in 1.2.)

When an array object is used in a numeric context, it evaluates to its length. When used in a boolean context, it evaluates to false if its length is 0 and otherwise evaluates to true.

However, at least the MDN documentation did agree with both of these in "what's new in 1.2," which we do support correctly, also for only version 1.2.

The Array.toString( ) method separates array elements with a comma and a space, instead of just a comma, and returns the list of elements within square brackets. In addition, string elements of the array are quoted, so that the result is a string in legal array literal syntax.

The default Object.toString( ) method displays the values of all properties defined by the object, returning a string formatted using object literal syntax docstore.mik.ua/orelly/webprog/jscript/ch11_06.htm

If we label the boolean conversion of a zero length array to true in version 1.2 as a bug. Is it a behavior we want to change now, considering it has functioned this way since Rhino was released as open source? My understanding is that the intent is to minimize changes to the legacy versions to preserve backward compatibility. If this was truly a regression bug that was introduced in a more recent version, then surely we would address it, but I would think that the "bug" is the accepted behavior after 20+ years.

Here's more fun stuff to contemplate... I downloaded Netscape Navigator 7.2 (windows version) and installed it in Wine. For language version 1.2, !![] did evaluate to false. However, for language versions 1.0, 1.1, and 1.3 it evaluated to true, so apparently our 100 and 110 version behavior is wrong.

I am unsure if my NDA allows me to tell you what we are doing... 😢 I'll try to find out what I can share.

K, as I said, just curious. If ServiceNow has implemented all those features in Rhino, I'd really like to know more about it, but am assuming cross-compiling is used.

Ok, I got the green light - we are using a transpiler in front of Rhino. 🙂

If we label the boolean conversion of a zero length array to true in version 1.2 as a bug. Is it a behavior we want to change now, considering it has functioned this way since Rhino was released as open source? My understanding is that the intent is to minimize changes to the legacy versions to preserve backward compatibility. If this was truly a regression bug that was introduced in a more recent version, then surely we would address it, but I would think that the "bug" is the accepted behavior after 20+ years.

I don't think it's a bug - we need to preserve it, even if it is weird. But, from what you are saying, my change in NativeArray should not check for versionECMA1 but rather look something like:

    @Override
    public Object getDefaultValue(Class<?> hint) {
        if (hint == ScriptRuntime.NumberClass) {
            Context cx = Context.getContext();
            if (cx.getLanguageVersion() == Context.VERSION_1_2) return Long.valueOf(length);
        }
        if (hint == ScriptRuntime.BooleanClass) {
            Context cx = Context.getContext();
            if (cx.getLanguageVersion() == Context.VERSION_1_2) return length != 0;
        }
        return super.getDefaultValue(hint);
    }

I've updated the PR.

Rhino has not changed behavior in this regard since initial release. Now, if we compare Rhino to Netscape, Rhino differs from Netscape in 3 different language versions (1.0, 1.1, and 1.2.) However, in neither engine did all 3 of those versions evaluate to the same true or false value.

Maybe I can illustrate this with some code.

I ran this code in Netscape Navigator 7.2 (2004)

<html>
	<head></head>
	<body>
		<script language="JavaScript1.0">
			document.write('1.0: ' + !![])
		</script>
        	<br>
		<script language="JavaScript1.1">
			document.write('1.1: ' + !![])
		</script>
	        <br>
		<script language="JavaScript1.2">
			document.write('1.2: ' + !![])
		</script>
        	<br>
		<script language="JavaScript1.3">
			document.write('1.3: ' + !![])
		</script>
	</body>
</html>

and this was the output

1.0: true
1.1: true
1.2: false
1.3: true

This was in Rhino 1.5R1 (2000)

js> var v = [100,110,120,130]
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + !![]) }
100: false
110: false
120: true
130: true

Rhino 1.7 release 1 (2008)

js> var v = [100,110,120,130]
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + !![]) }
100: false
110: false
120: true
130: true

Rhino 1.7.15-SNAPSHOT (2023)

js> var v = [100,110,120,130]
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + !![]) }
100: false
110: false
120: true
130: true

Your PR changes this table to the following, and I don't think there is justification for this change being "preservation" as this state has never existed anywhere in the past as far as I can tell.

100: false
110: false
120: false
130: true

I think either we call it a "bug" and make all language versions match Netscape or we say Rhino has been constant in this behavior since initial release and therefore the current behavior is now an immutable "feature." I think most people on this project lean towards the latter.

Your PR changes this table to the following, and I don't think there is justification for this change being "preservation" as this state has never existed anywhere in the past as far as I can tell.

I absolutely didn't mean to change that behavior, it is an oversight. I've pushed a fix, now it behaves in the "old" way.

This PR was only supposed to fix the initial problem with this javascript, not to touch arrays in any way. 🙂

var Base = function() {};
var Extending = function() {};
Extending.prototype = Base;
var x = new Extending();
!!x

That code is a similar situation. I don't see a need to "fix" anything. The only different between Netscape and current Rhino is that version 1.2 throws an error in Rhino, but doesn't in Netscape. Rhino has been constant in that differing behavior since initial release.

Netscape Navigator 7.2

<html>
	<head></head>
	<body>
		<script language="JavaScript1.0">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.0: ' + test())
		</script>
        <br>
		<script language="JavaScript1.1">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.1: ' + test())
		</script>
        <br>
		<script language="JavaScript1.2">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.2: ' + test())
		</script>
        <br>
		<script language="JavaScript1.3">
		    function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r;
		    try {r = !!x} catch(e) {r = e.toString()} return r;}
			document.write('1.3: ' + test())
		</script>
	</body>
</html>

1.0: TypeError: Function.prototype.toString called on incompatible object
1.1: TypeError: Function.prototype.toString called on incompatible object
1.2: true
1.3: true 

Rhino 15R1

js> var v = [100,110,120,130]
js> function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r; try {r = !!x} catch(e) {r = e.toString()} return r;}
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + test()) }
100: TypeError: Method "toString" called on incompatible object.
110: TypeError: Method "toString" called on incompatible object.
120: TypeError: Method "toString" called on incompatible object.
130: true

Rhino 1.7 release 1 2008 03 06

js> var v = [100,110,120,130]
js> function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r; try {r = !!x} catch(e) {r = e.toString()} return r;}
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + test()) }
100: TypeError: Method "toString" called on incompatible object.
110: TypeError: Method "toString" called on incompatible object.
120: TypeError: Method "toString" called on incompatible object.
130: true

Rhino 1.7.15-SNAPSHOT

js> var v = [100,110,120,130]
js> function test() {var B = function() {}; var E = function() {}; E.prototype = B; var x = new E(), r; try {r = !!x} catch(e) {r = e.toString()} return r;}
js> for (i=0; i < v.length; i++) { version(v[i]); print(v[i] + ': ' + test()) }
100: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeObject is not an instance of org.mozilla.javascript.BaseFunction).
110: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeObject is not an instance of org.mozilla.javascript.BaseFunction).
120: TypeError: Method "toString" called on incompatible object (org.mozilla.javascript.NativeObject is not an instance of org.mozilla.javascript.BaseFunction).
130: true

I'm also quite hesitant to start changing behavior that has existed in these old language versions van 20 years...

Thoughts @gbrail @rbri?

Well, seems this won't get merged. Closing it.