Reduce duplication between PropertyReadExprNode and ArrayIndexReadExp…

…rNode by introducing ObjectPropertyReadNode.

part-10/ReadMe.md

@@ -15,7 +15,8 @@ These new expressions are added to the
 The Nodes implementing those expressions are
 [`ArrayLiteralExprNode`](src/main/java/com/endoflineblog/truffle/part_10/nodes/exprs/arrays/ArrayLiteralExprNode.java),
 [`ArrayIndexReadExprNode`](src/main/java/com/endoflineblog/truffle/part_10/nodes/exprs/arrays/ArrayIndexReadExprNode.java)
-and [`ArrayIndexWriteExprNode`](src/main/java/com/endoflineblog/truffle/part_10/nodes/exprs/arrays/ArrayIndexWriteExprNode.java).
+and [`ArrayIndexWriteExprNode`](src/main/java/com/endoflineblog/truffle/part_10/nodes/exprs/arrays/ArrayIndexWriteExprNode.java),
+respectively.
 Accessing of the array elements is performed through a
 [Truffle library](https://www.graalvm.org/latest/graalvm-as-a-platform/language-implementation-framework/TruffleLibraries),
 [`InteropLibrary`](https://www.graalvm.org/truffle/javadoc/com/oracle/truffle/api/interop/InteropLibrary.html),

part-10/src/main/java/com/endoflineblog/truffle/part_10/nodes/exprs/arrays/ArrayIndexReadExprNode.java

@@ -18,8 +18,12 @@
 @NodeChild("arrayExpr")
 @NodeChild("indexExpr")
 public abstract class ArrayIndexReadExprNode extends EasyScriptExprNode {
+    /**
+     * A specialization for reading an integer index of an array,
+     * in code like {@code [1, 2][1]}.
+     */
     @Specialization(guards = "arrayInteropLibrary.isArrayElementReadable(array, index)", limit = "1")
-    protected Object readIntIndex(Object array, int index,
+    protected Object readIntIndexOfArray(Object array, int index,
             @CachedLibrary("array") InteropLibrary arrayInteropLibrary) {
         try {
             return arrayInteropLibrary.readArrayElement(array, index);
@@ -28,13 +32,22 @@ protected Object readIntIndex(Object array, int index,
         }
     }
 
+    /**
+     * Reading any property of {@code undefined}
+     * results in an error in JavaScript.
+     */
     @Specialization(guards = "interopLibrary.isNull(target)", limit = "1")
     protected Object indexUndefined(@SuppressWarnings("unused") Object target,
             Object index,
             @SuppressWarnings("unused") @CachedLibrary("target") InteropLibrary interopLibrary) {
         throw new EasyScriptException("Cannot read properties of undefined (reading '" + index + "')");
     }
 
+    /**
+     * Accessing a property of anything that is not {@code undefined}
+     * but doesn't have any members returns simply {@code undefined}
+     * in JavaScript.
+     */
     @Fallback
     protected Object readNonArrayOrNonIntIndex(@SuppressWarnings("unused") Object array,
             @SuppressWarnings("unused") Object index) {

part-11/ReadMe.md

@@ -118,33 +118,35 @@ we abandon caching, and instead switch to always creating a new `FunctionObject`
 
 With `ReadTruffleStringPropertyNode` now in place,
 we can use it in the existing property access Nodes.
-For direct access, in code like `"abc".length`,
-we add a new specialization to the
-[`PropertyReadExprNode` class](src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/properties/PropertyReadExprNode.java)
-for when the first argument is a `TruffleString`,
-which simply delegates to `ReadTruffleStringPropertyNode`,
+Since introducing strings to our language now makes it possible to access an object's property in two different ways
+(with "direct" access, in code like `a.propName`,
+and with "indexed" access, in code like `a['propName']`),
+we create a new class,
+[`ObjectPropertyReadNode`](src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/properties/ObjectPropertyReadNode.java),
+that contains the common logic of reading a property of an object.
+Its first specialization covers the situation where the target of the read is a `TruffleString`,
+in which case we simply delegate to `ReadTruffleStringPropertyNode`,
 obtained through the `@Cached` annotation,
-as it's a stateless Node.
+as it's a stateless Node;
+the remaining 3 specializations were moved from the `PropertyReadExprNode` class,
+[as it was in the previous part of the series](../part-10/src/main/java/com/endoflineblog/truffle/part_10/nodes/exprs/properties/PropertyReadExprNode.java).
+
+Because of this refactoring,
+we can change the
+[`PropertyReadExprNode` class](src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/properties/PropertyReadExprNode.java)
+to simply delegate to `ObjectPropertyReadNode`.
 
 For indexed property access,
-we need to add two specializations for `TruffleString` to the
-[`ArrayIndexReadExprNode` class](src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/arrays/ArrayIndexReadExprNode.java).
-The first one covers the case where the index is a string too,
+we also use `ObjectPropertyReadNode`,
+this time from the [`ArrayIndexReadExprNode` class](src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/arrays/ArrayIndexReadExprNode.java),
+but with one additional catch:
+we introduce a specialization that handles the situation when the index expression evaluates to a `TruffleString`,
 in code like `"a"['length']` - in that case,
 we need to convert `'length'` from a `TruffleString` to a Java string,
-which is what `ReadTruffleStringPropertyNode` expects
+which is what `ObjectPropertyReadNode` expects
 (we use the [Interop library](https://www.graalvm.org/truffle/javadoc/com/oracle/truffle/api/interop/InteropLibrary.html)
 for that purpose - even though `TruffleString` is not a `TruffleObject`,
 `InteropLibrary.isString()` still returns `true` for it).
-The second specialization covers non-string indexes,
-which is mostly integers, in code like `"abc"[1]`,
-which should return `'b'` - `ReadTruffleStringPropertyNode` covers that case already,
-so we can simply delegate to it without having to do any conversions.
-
-And lastly, we need to add a third specialization to `ArrayIndexReadExprNode`
-to handle reading string properties of non-`TruffleString` objects,
-which happens in code like `[1, 2, 3]['length']`.
-In this case, we simply use the `readMember()` message from `InteropLibrary`.
 
 ## Benchmark
 
@@ -174,19 +176,20 @@ it's 20 times slower for EasyScript, and 200 times slower for JavaScript
 You can change the implementation shown here to get different performance numbers.
 
 The first possible change is to handle `TruffleString` property names,
-in addition to Java strings, in `ReadTruffleStringPropertyNode`,
-which also allows you to get rid of the first added specialization in `ArrayIndexReadExprNode`.
+in addition to Java strings, in `ReadTruffleStringPropertyNode`.
 This makes the code in `ReadTruffleStringPropertyNode`
 contain a lot of duplication, as basically every property specialization needs to be written twice -
 once for Java strings, and once for `TruffleString`s.
 However, this improves the performance of the "index access" variant of the benchmark by 10x,
 making it only 2x slower than the "direct access" variant.
 
-The second possible change is to switch completely to `TruffleString`s in `ReadTruffleStringPropertyNode`.
-This will require changes in the first specialization in `PropertyReadExprNode`
-to convert a Java string to a `TruffleString`.
-However, it has the advantage of eliminating duplication in `ReadTruffleStringPropertyNode`.
-This change makes both variants have identical performance -
+The second possible change is to switch completely to `TruffleString`s
+as property names in `ReadTruffleStringPropertyNode`.
+This will require changes in `PropertyReadExprNode`
+to convert the property name from a Java string to a `TruffleString`.
+However, it has the advantage of eliminating the duplication in `ReadTruffleStringPropertyNode` mentioned above,
+when supporting both kinds of strings as property names.
+This change makes both variants of the benchmark have identical performance -
 unfortunately, it's the same performance as for the "index access"
 variant from above when changing `ReadTruffleStringPropertyNode`
 to handle both Java strings and `TruffleString`s,

part-11/src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/arrays/ArrayIndexReadExprNode.java

@@ -2,70 +2,34 @@
 
 import com.endoflineblog.truffle.part_11.exceptions.EasyScriptException;
 import com.endoflineblog.truffle.part_11.nodes.exprs.EasyScriptExprNode;
-import com.endoflineblog.truffle.part_11.nodes.exprs.strings.ReadTruffleStringPropertyNode;
-import com.endoflineblog.truffle.part_11.runtime.Undefined;
+import com.endoflineblog.truffle.part_11.nodes.exprs.properties.ObjectPropertyReadNode;
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Fallback;
 import com.oracle.truffle.api.dsl.NodeChild;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.interop.InteropLibrary;
 import com.oracle.truffle.api.interop.InvalidArrayIndexException;
-import com.oracle.truffle.api.interop.UnknownIdentifierException;
 import com.oracle.truffle.api.interop.UnsupportedMessageException;
 import com.oracle.truffle.api.library.CachedLibrary;
-import com.oracle.truffle.api.strings.TruffleString;
 
 /**
  * The Node representing reading array indexes
  * (like {@code a[1]}).
  * Similar to the class with the same name from part 10,
- * the difference is we add extra specializations for handling strings -
- * both as targets of the indexing (where we expect {@link TruffleString}s),
- * and when strings are used as the index -
- * in code like {@code a["b"]}, which in JavaScript is equivalent to {@code a.b}.
+ * the difference is we add extra specializations for when strings are used as the index,
+ * in code like {@code a["b"]} (which, in JavaScript, is equivalent to {@code a.b}).
  *
- * @see #readStringPropertyOfString
- * @see #readPropertyOfString
- * @see #readProperty
+ * @see #readStringPropertyOfObject
  */
 @NodeChild("arrayExpr")
 @NodeChild("indexExpr")
 public abstract class ArrayIndexReadExprNode extends EasyScriptExprNode {
     /**
-     * A specialization for reading a string property of a string,
-     * in code like {@code "a"['length']}.
-     * We delegate to {@link ReadTruffleStringPropertyNode},
-     * but we first convert the index to a Java string,
-     * which is what {@link ReadTruffleStringPropertyNode} expects.
+     * A specialization for reading an integer index of an array,
+     * in code like {@code [1, 2][1]}.
      */
-    @Specialization(limit = "1", guards = "indexInteropLibrary.isString(index)")
-    protected Object readStringPropertyOfString(TruffleString target,
-            Object index,
-            @CachedLibrary("index") InteropLibrary indexInteropLibrary,
-            @Cached ReadTruffleStringPropertyNode readStringPropertyNode) {
-        try {
-            return readStringPropertyNode.executeReadTruffleStringProperty(target,
-                    indexInteropLibrary.asString(index));
-        } catch (UnsupportedMessageException e) {
-            throw new EasyScriptException(this, e.getMessage());
-        }
-    }
-
-    /**
-     * A specialization for reading a non-string property of a string.
-     * The main usecase for this is string indexing,
-     * in code like {@code "a"[1]}.
-     * We delegate the implementation to {@link ReadTruffleStringPropertyNode}.
-     */
-    @Specialization
-    protected Object readPropertyOfString(TruffleString target, Object index,
-            @Cached ReadTruffleStringPropertyNode readStringPropertyNode) {
-        return readStringPropertyNode.executeReadTruffleStringProperty(target,
-                index);
-    }
-
     @Specialization(guards = "arrayInteropLibrary.isArrayElementReadable(array, index)", limit = "1")
-    protected Object readIntIndex(Object array, int index,
+    protected Object readIntIndexOfArray(Object array, int index,
             @CachedLibrary("array") InteropLibrary arrayInteropLibrary) {
         try {
             return arrayInteropLibrary.readArrayElement(array, index);
@@ -75,37 +39,28 @@ protected Object readIntIndex(Object array, int index,
     }
 
     /**
-     * A specialization for reading a string property of a non-string target,
-     * in code like {@code [1, 2]['length']}.
-     * The implementation is identical to {@code PropertyReadExprNode}.
+     * A specialization for reading a string property of an object,
+     * in code like {@code [1, 2]['length']}, or {@code "a"['length']}.
      */
-    @Specialization(guards = {
-            "targetInteropLibrary.hasMembers(target)",
-            "propertyNameInteropLibrary.isString(propertyName)"
-    }, limit = "1")
-    protected Object readProperty(Object target, Object propertyName,
-            @CachedLibrary("target") InteropLibrary targetInteropLibrary,
-            @CachedLibrary("propertyName") InteropLibrary propertyNameInteropLibrary) {
+    @Specialization(guards = "propertyNameInteropLibrary.isString(propertyName)", limit = "1")
+    protected Object readStringPropertyOfObject(Object target, Object propertyName,
+            @CachedLibrary("propertyName") InteropLibrary propertyNameInteropLibrary,
+            @Cached ObjectPropertyReadNode objectPropertyReadNode) {
         try {
-            return targetInteropLibrary.readMember(target,
+            return objectPropertyReadNode.executePropertyRead(target,
                     propertyNameInteropLibrary.asString(propertyName));
-        } catch (UnknownIdentifierException e) {
-            return Undefined.INSTANCE;
         } catch (UnsupportedMessageException e) {
             throw new EasyScriptException(this, e.getMessage());
         }
     }
 
-    @Specialization(guards = "interopLibrary.isNull(target)", limit = "1")
-    protected Object indexUndefined(@SuppressWarnings("unused") Object target,
-            Object index,
-            @SuppressWarnings("unused") @CachedLibrary("target") InteropLibrary interopLibrary) {
-        throw new EasyScriptException("Cannot read properties of undefined (reading '" + index + "')");
-    }
-
+    /**
+     * A specialization for reading a non-string property of an object,
+     * in code like {@code "a"[0]}, or {@code [1, 2][undefined]}.
+     */
     @Fallback
-    protected Object readNonArrayOrNonIntIndex(@SuppressWarnings("unused") Object array,
-            @SuppressWarnings("unused") Object index) {
-        return Undefined.INSTANCE;
+    protected Object readNonStringPropertyOfObject(Object target, Object index,
+            @Cached ObjectPropertyReadNode objectPropertyReadNode) {
+        return objectPropertyReadNode.executePropertyRead(target, index);
     }
 }

part-11/src/main/java/com/endoflineblog/truffle/part_11/nodes/exprs/properties/ObjectPropertyReadNode.java

@@ -0,0 +1,66 @@
+package com.endoflineblog.truffle.part_11.nodes.exprs.properties;
+
+import com.endoflineblog.truffle.part_11.exceptions.EasyScriptException;
+import com.endoflineblog.truffle.part_11.nodes.exprs.strings.ReadTruffleStringPropertyNode;
+import com.endoflineblog.truffle.part_11.runtime.Undefined;
+import com.oracle.truffle.api.dsl.Cached;
+import com.oracle.truffle.api.dsl.Fallback;
+import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.interop.InteropLibrary;
+import com.oracle.truffle.api.interop.UnknownIdentifierException;
+import com.oracle.truffle.api.interop.UnsupportedMessageException;
+import com.oracle.truffle.api.library.CachedLibrary;
+import com.oracle.truffle.api.nodes.Node;
+import com.oracle.truffle.api.strings.TruffleString;
+
+/**
+ * A Node for reading a property of a JavaScript object.
+ * Used by {@link PropertyReadExprNode} and {@link com.endoflineblog.truffle.part_11.nodes.exprs.arrays.ArrayIndexReadExprNode}.
+ */
+public abstract class ObjectPropertyReadNode extends Node {
+    public abstract Object executePropertyRead(Object target, Object property);
+
+    /**
+     * The specialization for reading a property of a {@link TruffleString}.
+     * Simply delegates to {@link ReadTruffleStringPropertyNode}.
+     */
+    @Specialization
+    protected Object readPropertyOfString(TruffleString target, Object property,
+            @Cached ReadTruffleStringPropertyNode readStringPropertyNode) {
+        return readStringPropertyNode.executeReadTruffleStringProperty(
+                target, property);
+    }
+
+    @Specialization(guards = "interopLibrary.hasMembers(target)", limit = "1")
+    protected Object readProperty(Object target, String propertyName,
+            @CachedLibrary("target") InteropLibrary interopLibrary) {
+        try {
+            return interopLibrary.readMember(target, propertyName);
+        } catch (UnknownIdentifierException e) {
+            return Undefined.INSTANCE;
+        } catch (UnsupportedMessageException e) {
+            throw new EasyScriptException(this, e.getMessage());
+        }
+    }
+
+    /**
+     * Reading any property of {@code undefined}
+     * results in an error in JavaScript.
+     */
+    @Specialization(guards = "interopLibrary.isNull(target)", limit = "1")
+    protected Object readPropertyOfUndefined(@SuppressWarnings("unused") Object target, Object property,
+            @CachedLibrary("target") @SuppressWarnings("unused") InteropLibrary interopLibrary) {
+        throw new EasyScriptException("Cannot read properties of undefined (reading '" + property + "')");
+    }
+
+    /**
+     * Accessing a property of anything that is not {@code undefined}
+     * but doesn't have any members returns simply {@code undefined}
+     * in JavaScript.
+     */
+    @Fallback
+    protected Object readPropertyOfNonUndefinedWithoutMembers(@SuppressWarnings("unused") Object target,
+            @SuppressWarnings("unused") Object property) {
+        return Undefined.INSTANCE;
+    }
+}