From 5aa5f4ad73a9848e4e051b705fdd3bac6f6d51d2 Mon Sep 17 00:00:00 2001 From: Satish Srinivasan Date: Thu, 12 Dec 2024 06:53:51 +0530 Subject: [PATCH] Address review comments --- .../org/mozilla/javascript/BaseFunction.java | 31 +---- .../javascript/IdScriptableObject.java | 23 +--- .../mozilla/javascript/ScriptableObject.java | 22 +--- ...unctionPrototypeSymbolHasInstanceTest.java | 116 ++---------------- 4 files changed, 24 insertions(+), 168 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/BaseFunction.java b/rhino/src/main/java/org/mozilla/javascript/BaseFunction.java index 4adf30d467..65e65a077c 100644 --- a/rhino/src/main/java/org/mozilla/javascript/BaseFunction.java +++ b/rhino/src/main/java/org/mozilla/javascript/BaseFunction.java @@ -32,7 +32,9 @@ static void init(Context cx, Scriptable scope, boolean sealed) { obj.setStandardPropertyAttributes(READONLY | DONTENUM); } IdFunctionObject constructor = obj.exportAsJSClass(MAX_PROTOTYPE_ID, scope, sealed); - ScriptRuntimeES6.addSymbolHasInstance(cx, scope, constructor); + if (cx.getLanguageVersion() >= Context.VERSION_ES6) { + ScriptRuntimeES6.addSymbolHasInstance(cx, scope, constructor); + } } /** @@ -269,7 +271,7 @@ protected void fillConstructorProperties(IdFunctionObject ctor) { @Override protected void initPrototypeId(int id) { if (id == SymbolId_hasInstance) { - initPrototypeValue(id, SymbolKey.HAS_INSTANCE, makeHasInstance(), 0x0F); + initPrototypeValue(id, SymbolKey.HAS_INSTANCE, makeHasInstance(), CONST | DONTENUM); return; } @@ -359,9 +361,8 @@ public Object call( } throw ScriptRuntime.typeErrorById( "msg.instanceof.bad.prototype", getFunctionName()); - } else { - return false; // NOT_FOUND, null etc. } + return false; // NOT_FOUND, null etc. } }); } @@ -564,26 +565,6 @@ protected boolean hasPrototypeProperty() { return prototypeProperty != null || this instanceof NativeFunction; } - @Override - ScriptableObject buildDataDescriptorHelper( - int instanceIdInfo, Scriptable scope, Object value, int attr) { - if (instanceIdInfo == SymbolId_hasInstance) { - return buildDataDescriptor(scope, value, attr, SymbolKey.HAS_INSTANCE.toString(), 1); - } else { - return super.buildDataDescriptorHelper(instanceIdInfo, scope, value, attr); - } - } - - @Override - ScriptableObject buildDataDescriptorHelper( - Symbol key, Scriptable scope, Object value, int attr) { - if (key == SymbolKey.HAS_INSTANCE) { - return buildDataDescriptor(scope, value, attr, key.toString(), 1); - } else { - return super.buildDataDescriptorHelper(key, scope, value, attr); - } - } - public Object getPrototypeProperty() { Object result = prototypeProperty; if (result == null) { @@ -705,7 +686,7 @@ private Object jsConstructor(Context cx, Scriptable scope, Object[] args) { @Override protected int findPrototypeId(Symbol k) { if (SymbolKey.HAS_INSTANCE.equals(k)) return SymbolId_hasInstance; - else return 0; + return 0; } @Override diff --git a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java index 51b3a0bfc4..dc28a712e3 100644 --- a/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java @@ -205,7 +205,7 @@ final void delete(int id) { String name = null; if (valueArray[nameSlot] instanceof String) name = (String) valueArray[nameSlot]; - else if (valueArray[nameSlot] instanceof SymbolKey) { + else if (valueArray[nameSlot] instanceof Symbol) { name = valueArray[nameSlot].toString(); } throw ScriptRuntime.typeErrorById( @@ -942,21 +942,6 @@ protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) { return desc; } - /** - * Overridden in the base class for different descriptors - * - * @return ScriptableObject - */ - ScriptableObject buildDataDescriptorHelper( - Symbol key, Scriptable scope, Object value, int attr) { - return buildDataDescriptor(scope, value, attr); - } - - ScriptableObject buildDataDescriptorHelper( - int instanceIdInfo, Scriptable scope, Object value, int attr) { - return buildDataDescriptor(scope, value, attr); - } - private ScriptableObject getBuiltInDescriptor(String name) { Object value = null; int attr = EMPTY; @@ -971,14 +956,14 @@ private ScriptableObject getBuiltInDescriptor(String name) { int id = (info & 0xFFFF); value = getInstanceIdValue(id); attr = (info >>> 16); - return buildDataDescriptorHelper(info, scope, value, attr); + return buildDataDescriptor(scope, value, attr); } if (prototypeValues != null) { int id = prototypeValues.findId(name); if (id != 0) { value = prototypeValues.get(id); attr = prototypeValues.getAttributes(id); - return buildDataDescriptorHelper(info, scope, value, attr); + return buildDataDescriptor(scope, value, attr); } } return null; @@ -998,7 +983,7 @@ private ScriptableObject getBuiltInDescriptor(Symbol key) { if (id != 0) { value = prototypeValues.get(id); attr = prototypeValues.getAttributes(id); - return buildDataDescriptorHelper(key, scope, value, attr); + return buildDataDescriptor(scope, value, attr); } } return null; diff --git a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java index 48998047af..dc43271e2e 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java @@ -135,28 +135,14 @@ public abstract class ScriptableObject protected static ScriptableObject buildDataDescriptor( Scriptable scope, Object value, int attributes) { - return buildDataDescriptor(scope, value, attributes, null, -1); - } - - protected static ScriptableObject buildDataDescriptor( - Scriptable scope, Object value, int attributes, String name, int length) { ScriptableObject desc = new NativeObject(); ScriptRuntime.setBuiltinProtoAndParent(desc, scope, TopLevel.Builtins.Object); desc.defineProperty("value", value, EMPTY); - desc.setCommonDescriptorProperties(attributes, true, name, length); + desc.setCommonDescriptorProperties(attributes, true); return desc; } - protected void setCommonDescriptorProperties( - int attributes, boolean defineWritable, String name, int length) { - if (name != null) { - defineProperty("name", "[Symbol.hasInstance]", attributes); - } - - if (length != -1) { - defineProperty("length", length, attributes); - } - + protected void setCommonDescriptorProperties(int attributes, boolean defineWritable) { if (defineWritable) { defineProperty("writable", (attributes & READONLY) == 0, EMPTY); } @@ -164,10 +150,6 @@ protected void setCommonDescriptorProperties( defineProperty("configurable", (attributes & PERMANENT) == 0, EMPTY); } - protected void setCommonDescriptorProperties(int attributes, boolean defineWritable) { - setCommonDescriptorProperties(attributes, defineWritable, null, -1); - } - static void checkValidAttributes(int attributes) { final int mask = READONLY | DONTENUM | PERMANENT | UNINITIALIZED_CONST; if ((attributes & ~mask) != 0) { diff --git a/rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java b/rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java index 56e1644a4f..9f8c79fe8b 100644 --- a/rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java +++ b/rhino/src/test/java/org/mozilla/javascript/FunctionPrototypeSymbolHasInstanceTest.java @@ -16,17 +16,7 @@ public void testSymbolHasInstanceIsPresent() { + "};\n" + "var g = {};\n" + "`${f.hasOwnProperty(Symbol.hasInstance)}:${g.hasOwnProperty(Symbol.hasInstance)}`"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - String result = - (String) - cx.evaluateString( - scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("true:false", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("true:false", script); } @Test @@ -39,15 +29,7 @@ public void testSymbolHasInstanceCanBeCalledLikeAnotherMethod() { + " }" + "};\n" + "f[Symbol.hasInstance]() == 42"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals(true, result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6(true, script); } // See: https://tc39.es/ecma262/#sec-function.prototype-%symbol.hasinstance% @@ -56,15 +38,7 @@ public void testFunctionPrototypeSymbolHasInstanceHasAttributes() { String script = "var a = Object.getOwnPropertyDescriptor(Function.prototype, Symbol.hasInstance);\n" + "a.writable + ':' + a.configurable + ':' + a.enumerable"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("false:false:false", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("false:false:false", script); } // See: https://tc39.es/ecma262/#sec-function.prototype-%symbol.hasinstance% @@ -81,15 +55,7 @@ public void testFunctionPrototypeSymbolHasInstanceHasAttributesStrictMode() { + " typeErrorThrown = true \n" + "}\n" + "Object.prototype.hasOwnProperty.call(Function.prototype, Symbol.hasInstance) + ':' + typeErrorThrown + ':' + t + ':' + a.writable + ':' + a.configurable + ':' + a.enumerable; \n"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("true:true:function:false:false:false", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("true:true:function:false:false:false", script); } @Test @@ -102,18 +68,8 @@ public void testFunctionPrototypeSymbolHasInstanceHasProperties() { String script2 = "var a = Object.getOwnPropertyDescriptor(Function.prototype[Symbol.hasInstance], 'name');\n" + "a.value + ':' + a.writable + ':' + a.configurable + ':' + a.enumerable"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Object result2 = - cx.evaluateString(scope, script2, "testSymbolHasInstance", 0, null); - Assert.assertEquals("1:false:true:false", result); - Assert.assertEquals("Symbol(Symbol.hasInstance):false:true:false", result2); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("1:false:true:false", script); + Utils.assertWithAllOptimizationLevelsES6("Symbol(Symbol.hasInstance):false:true:false", script2); } @Test @@ -121,15 +77,7 @@ public void testFunctionPrototypeSymbolHasInstance() { String script = "(Function.prototype[Symbol.hasInstance] instanceof Function) + ':' + " + "Function.prototype[Symbol.hasInstance].call(Function, Object)\n"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("true:true", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("true:true", script); } @Test @@ -140,15 +88,7 @@ public void testFunctionPrototypeSymbolHasInstanceOnObjectReturnsTrue() { + "var o2 = Object.create(o);\n" + "(f[Symbol.hasInstance](o)) + ':' + " + "(f[Symbol.hasInstance](o2));\n"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("true:true", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("true:true", script); } @Test @@ -158,15 +98,7 @@ public void testFunctionPrototypeSymbolHasInstanceOnBoundTargetReturnsTrue() { + "var bc = new BC();\n" + "var bound = BC.bind();\n" + "bound[Symbol.hasInstance](bc);\n"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals(true, result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6(true, script); } @Test @@ -179,15 +111,7 @@ public void testFunctionInstanceNullVoidEtc() { + "(null instanceof f) + ':' +\n" + "(void 0 instanceof f)\n" + "a"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("false:false:false:false", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("false:false:false:false", script); } @Test @@ -195,15 +119,7 @@ public void testFunctionPrototypeSymbolHasInstanceReturnsFalseOnUndefinedOrProto String script = "Function.prototype[Symbol.hasInstance].call() + ':' +" + "Function.prototype[Symbol.hasInstance].call({});"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals("false:false", result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6("false:false", script); } @Test @@ -221,15 +137,7 @@ public void testSymbolHasInstanceIsInvokedInInstanceOf() { + "Object.setPrototypeOf(g, f);\n" + "g instanceof f;" + "globalSet == 1"; - Utils.runWithAllOptimizationLevels( - (cx) -> { - cx.setLanguageVersion(Context.VERSION_ES6); - final Scriptable scope = cx.initStandardObjects(); - Object result = - cx.evaluateString(scope, script, "testSymbolHasInstance", 0, null); - Assert.assertEquals(true, result); - return null; - }); + Utils.assertWithAllOptimizationLevelsES6(true, script); } @Test