[GR-26443] Ordering of field/getter/setter access in Nashorn-compat m…

…ode.

PullRequest: js/1708

CHANGELOG.md

@@ -5,6 +5,7 @@ The main focus is on user-observable behavior of the engine.
 
 ## Version 20.3.0
 * Updated Node.js to version 12.18.4.
+* Fixed field/getter/setter access order in nashorn-compat mode, see [issue #343](https://github.com/graalvm/graaljs/issues/343).
 
 ## Version 20.2.0
 * Implemented the [Intl.NumberFormat Unified API](https://github.com/tc39/proposal-unified-intl-numberformat) proposal.

docs/user/NashornMigrationGuide.md

@@ -204,14 +204,12 @@ var myYear = date.year; // calls date.getYear()
 date.year = myYear + 1; // calls date.setYear(myYear + 1);
 ```
 
-GraalVM JavaScript defines an ordering in which it searches for the field or getters.
-It will always first try to read or write the field with the name as provided in the property.
-If the field cannot be read or written, it will try to call a getter or setter:
-* In case of a read operation, GraalVM JavaScript will first try to call a getter with the name `get` and the property name in camel case. If that is not available, a getter with the name `is` and the property name in camel case is called. In the second case, the value is returned even if it is not of type boolean.
-* In case of a write operation, GraalVM JavaScript will try to call a setter with the name `set` and the property name in camel case, providing the value as argument to that function.
-
-Nashorn can expose random behavior when both `getFieldName` and `isFieldName` are available.
-Nashorn also gives precedence to getters, even when a public field of the exact name is available.
+GraalVM JavaScript mimics the behavior of Nashorn regarding the ordering of the access:
+* In case of a read operation, GraalVM JavaScript will first try to call a getter with the name `get` and the property name in camel case. If that is not available, a getter with the name `is` and the property name in camel case is called. In the second case, unlike Nashorn, the resulting value is returned even if it is not of type boolean. Only if both methods are not available, the property itself will be read.
+* In case of a write operation, GraalVM JavaScript will try to call a setter with the name `set` and the property name in camel case, providing the value as argument to that function. If the setter is not available, the property itself will be written.
+
+Note that Nashorn (and thus, GraalVM JavaScript) makes a clear distinction between property read/writes and function calls.
+When the Java class has both a field and a method of the same name publically available, `obj.property` will always read the field (or the getter as discussed above), while `obj.property()` will always call the respective method.
 
 ## Additional Aspects to Consider
 

graal-js/src/com.oracle.truffle.js.test/src/com/oracle/truffle/js/test/nashorn/PropertyAccessorOrder.java

@@ -0,0 +1,151 @@
+/*
+ * Copyright (c) 2020, 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * The Universal Permissive License (UPL), Version 1.0
+ *
+ * Subject to the condition set forth below, permission is hereby granted to any
+ * person obtaining a copy of this software, associated documentation and/or
+ * data (collectively the "Software"), free of charge and under any and all
+ * copyright rights in the Software, and any and all patent rights owned or
+ * freely licensable by each licensor hereunder covering either (i) the
+ * unmodified Software as contributed to or provided by such licensor, or (ii)
+ * the Larger Works (as defined below), to deal in both
+ *
+ * (a) the Software, and
+ *
+ * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if
+ * one is included with the Software each a "Larger Work" to which the Software
+ * is contributed by such licensors),
+ *
+ * without restriction, including without limitation the rights to copy, create
+ * derivative works of, display, perform, and distribute the Software and make,
+ * use, sell, offer for sale, import, export, have made, and have sold the
+ * Software and the Larger Work(s), and to sublicense the foregoing rights on
+ * either these or other terms.
+ *
+ * This license is subject to the following condition:
+ *
+ * The above copyright notice and either this complete permission notice or at a
+ * minimum a reference to the UPL must be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+package com.oracle.truffle.js.test.nashorn;
+
+import org.graalvm.polyglot.Context;
+import org.graalvm.polyglot.Source;
+import org.graalvm.polyglot.Value;
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.oracle.truffle.js.lang.JavaScriptLanguage;
+import com.oracle.truffle.js.runtime.JSContextOptions;
+import com.oracle.truffle.js.test.JSTest;
+
+//see e.g. GR-26443
+public class PropertyAccessorOrder {
+
+    private static void testIntl(String sourceText, Object obj) {
+        try (Context context = JSTest.newContextBuilder().option(JSContextOptions.NASHORN_COMPATIBILITY_MODE_NAME, "true").allowAllAccess(true).build()) {
+            context.getBindings("js").putMember("obj", obj);
+            Value result = context.eval(Source.newBuilder(JavaScriptLanguage.ID, sourceText, "property-accessor-test").buildLiteral());
+            Assert.assertTrue(result.isBoolean());
+            Assert.assertTrue(result.asBoolean());
+        }
+    }
+
+    @Test
+    public void testGetter() {
+        // expected order in nashorn-compat mode is: `getId`>`isId`>`id`
+
+        // check property access
+        testIntl("obj.id === 'field'", new TestField());
+        testIntl("obj.id === 'getId'", new TestGetter());
+        testIntl("obj.id === 'isId'", new TestChecker());
+        testIntl("obj.id === 'isId'", new TestFieldAndChecker());
+        testIntl("obj.id === 'getId'", new TestAll());
+
+        // check calls
+        testIntl("obj.id === 'field';", new TestFieldAndFunction());
+        testIntl("obj.id() === 'function';", new TestFieldAndFunction());
+        testIntl("obj.id() === 'function'", new TestAll());
+    }
+
+    @Test
+    public void testSetter() {
+        // expected order in nashorn-compat mode is: `setId`>`id`
+        testIntl("obj.id = 'test'; obj.countSet===0 && obj.id === 'test';", new TestField());
+        testIntl("obj.id = 'test'; obj.countSet===1 && obj.id === undefined;", new TestSetter());
+        testIntl("obj.id = 'test'; obj.countSet===1 && obj.id === 'getId';", new TestAll());
+    }
+
+    public static class TestGetSet {
+        public int countSet = 0;
+    }
+
+    public static class TestField extends TestGetSet {
+        public String id = "field";
+    }
+
+    public static class TestFieldAndFunction extends TestGetSet {
+        public String id = "field";
+
+        public String id() {
+            return "function";
+        }
+    }
+
+    public static class TestGetter extends TestGetSet {
+        public String getId() {
+            return "getId";
+        }
+    }
+
+    public static class TestSetter extends TestGetSet {
+        public void setId(@SuppressWarnings("unused") Object value) {
+            countSet++;
+        }
+    }
+
+    public static class TestChecker extends TestGetSet {
+        public String isId() {
+            return "isId";
+        }
+    }
+
+    public static class TestFieldAndChecker extends TestGetSet {
+        public String id = "field";
+
+        public String isId() {
+            return "isId";
+        }
+    }
+
+    public static class TestAll extends TestGetSet {
+        public String id = "field";
+
+        public String isId() {
+            return "isId";
+        }
+
+        public String getId() {
+            return "getId";
+        }
+
+        public void setId(@SuppressWarnings("unused") Object value) {
+            countSet++;
+        }
+
+        public String id() {
+            return "function";
+        }
+    }
+}

graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/PropertyGetNode.java

@@ -941,14 +941,20 @@ private Object foreignGet(Object thisObj, PropertyGetNode root) {
                 return Undefined.instance;
             }
             String stringKey = (String) key;
+            if (context.isOptionNashornCompatibilityMode()) {
+                Object result = tryGetters(thisObj, root);
+                if (result != null) {
+                    return result;
+                }
+            }
             Object foreignResult;
             if (optimistic) {
                 try {
                     foreignResult = interop.readMember(thisObj, stringKey);
                 } catch (UnknownIdentifierException | UnsupportedMessageException e) {
                     CompilerDirectives.transferToInterpreterAndInvalidate();
                     optimistic = false;
-                    foreignResult = fallback(thisObj, key, root, e instanceof UnknownIdentifierException);
+                    foreignResult = maybeGetFromPrototype(thisObj, key);
                 }
             } else {
                 if (interop.isMemberReadable(thisObj, stringKey)) {
@@ -958,20 +964,12 @@ private Object foreignGet(Object thisObj, PropertyGetNode root) {
                         return Undefined.instance;
                     }
                 } else {
-                    foreignResult = fallback(thisObj, key, root, true);
+                    foreignResult = maybeGetFromPrototype(thisObj, key);
                 }
             }
             return importValueNode.executeWithTarget(foreignResult);
         }
 
-        private Object fallback(Object thisObj, Object key, PropertyGetNode root, boolean mayHaveMembers) {
-            if (mayHaveMembers && context.isOptionNashornCompatibilityMode()) {
-                return tryInvokeGetter(thisObj, root);
-            } else {
-                return maybeGetFromPrototype(thisObj, key);
-            }
-        }
-
         private Object maybeGetFromPrototype(Object thisObj, Object key) {
             if (context.getContextOptions().hasForeignObjectPrototype()) {
                 if (getFromPrototypeNode == null || foreignObjectPrototypeNode == null) {
@@ -986,24 +984,26 @@ private Object maybeGetFromPrototype(Object thisObj, Object key) {
             return Undefined.instance;
         }
 
-        // in nashorn-compat mode, `javaObj.xyz` can mean `javaObj.getXyz()`.
-        private Object tryInvokeGetter(Object thisObj, PropertyGetNode root) {
+        // in nashorn-compat mode, `javaObj.xyz` can mean `javaObj.getXyz()` or `javaObj.isXyz()`.
+        private Object tryGetters(Object thisObj, PropertyGetNode root) {
             assert context.isOptionNashornCompatibilityMode();
             TruffleLanguage.Env env = context.getRealm().getEnv();
             if (env.isHostObject(thisObj)) {
-                Object result = tryGetResult(thisObj, "get", root);
+                Object result = tryInvokeGetter(thisObj, "get", root);
                 if (result != null) {
                     return result;
                 }
-                result = tryGetResult(thisObj, "is", root);
+                result = tryInvokeGetter(thisObj, "is", root);
+                // Nashorn would only accept `isXyz` of type boolean. We cannot check upfront!
                 if (result != null) {
                     return result;
                 }
             }
-            return maybeGetFromPrototype(thisObj, root.getKey());
+            return null;
         }
 
-        private Object tryGetResult(Object thisObj, String prefix, PropertyGetNode root) {
+        private Object tryInvokeGetter(Object thisObj, String prefix, PropertyGetNode root) {
+            assert context.isOptionNashornCompatibilityMode();
             String getterKey = root.getAccessorKey(prefix);
             if (getterKey == null) {
                 return null;

graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/PropertySetNode.java

@@ -1109,11 +1109,17 @@ protected boolean setValue(Object thisObj, Object value, Object receiver, Proper
 
         private boolean performWriteMember(Object truffleObject, Object value, PropertySetNode root) {
             String stringKey = (String) root.getKey();
+            if (context.isOptionNashornCompatibilityMode()) {
+                if (tryInvokeSetter(truffleObject, value, root)) {
+                    return true;
+                }
+            }
             if (optimistic) {
                 try {
                     interop.writeMember(truffleObject, stringKey, value);
                 } catch (UnknownIdentifierException e) {
-                    unknownIdentifier(truffleObject, value, root);
+                    CompilerDirectives.transferToInterpreterAndInvalidate();
+                    optimistic = false;
                 } catch (UnsupportedTypeException | UnsupportedMessageException e) {
                     throw Errors.createTypeErrorInteropException(truffleObject, e, "writeMember", stringKey, this);
                 }
@@ -1124,46 +1130,35 @@ private boolean performWriteMember(Object truffleObject, Object value, PropertyS
                     } catch (UnknownIdentifierException | UnsupportedTypeException | UnsupportedMessageException e) {
                         throw Errors.createTypeErrorInteropException(truffleObject, e, "writeMember", stringKey, this);
                     }
-                } else if (context.isOptionNashornCompatibilityMode()) {
-                    tryInvokeSetter(truffleObject, value, root);
                 }
             }
             return true;
         }
 
-        private void unknownIdentifier(Object truffleObject, Object value, PropertySetNode root) {
-            CompilerDirectives.transferToInterpreterAndInvalidate();
-            optimistic = false;
-            if (context.isOptionNashornCompatibilityMode()) {
-                tryInvokeSetter(truffleObject, value, root);
-            }
-        }
-
         // in nashorn-compat mode, `javaObj.xyz = a` can mean `javaObj.setXyz(a)`.
-        private void tryInvokeSetter(Object thisObj, Object value, PropertySetNode root) {
+        private boolean tryInvokeSetter(Object thisObj, Object value, PropertySetNode root) {
             assert context.isOptionNashornCompatibilityMode();
-            if (!(root.getKey() instanceof String)) {
-                return;
-            }
             TruffleLanguage.Env env = context.getRealm().getEnv();
             if (env.isHostObject(thisObj)) {
                 String setterKey = root.getAccessorKey("set");
                 if (setterKey == null) {
-                    return;
+                    return false;
                 }
                 if (setterInterop == null) {
                     CompilerDirectives.transferToInterpreterAndInvalidate();
                     setterInterop = insert(InteropLibrary.getFactory().createDispatched(3));
                 }
                 if (!setterInterop.isMemberInvocable(thisObj, setterKey)) {
-                    return;
+                    return false;
                 }
                 try {
                     setterInterop.invokeMember(thisObj, setterKey, value);
+                    return true;
                 } catch (UnknownIdentifierException | UnsupportedMessageException | UnsupportedTypeException | ArityException e) {
                     // silently ignore
                 }
             }
+            return false;
         }
     }