Moved ADefLink to be a marker interface instead.

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EChain.java

@@ -215,12 +215,6 @@ public final class EChain extends AExpression {
         there = AnalyzerCaster.getLegalCast(definition, location, last.after, promote, false);
         back = AnalyzerCaster.getLegalCast(definition, location, promote, last.after, true);
 
-        if (last instanceof ADefLink) {
-            final ADefLink lastDef = (ADefLink) last;
-            // Unfortunately, we don't know the real type because we load from DEF and store to DEF!
-            lastDef.storeValueType = last.after;
-        }
-
         this.statement = true;
         this.actual = read ? last.after : definition.voidType;
     }
@@ -230,28 +224,26 @@ public final class EChain extends AExpression {
 
         // If the store node is a DEF node, we remove the cast to DEF from the expression
         // and promote the real type to it:
-        if (last instanceof ADefLink) {
+        if (last instanceof IDefLink) {
-            final ADefLink lastDef = (ADefLink) last;
             expression.analyze(settings, definition, variables);
-            // TODO: does it make more sense to just re-use last.after instead of using storeValueType?
+            last.after = expression.expected = expression.actual;
-            lastDef.storeValueType = expression.expected = expression.actual;
-            this.actual = read ? lastDef.storeValueType : definition.voidType;
         } else {
             // otherwise we adapt the type of the expression to the store type
             expression.expected = last.after;
             expression.analyze(settings, definition, variables);
-            this.actual = read ? last.after : definition.voidType;
         }
 
         expression = expression.cast(settings, definition, variables);
+
         this.statement = true;
+        this.actual = read ? last.after : definition.voidType;
     }
 
     private void analyzeRead() {
         final ALink last = links.get(links.size() - 1);
 
         // If the load node is a DEF node, we adapt its after type to use _this_ expected output type:
-        if (last instanceof ADefLink && this.expected != null) {
+        if (last instanceof IDefLink && this.expected != null) {
             last.after = this.expected;
         }
 
@@ -317,12 +309,7 @@ public final class EChain extends AExpression {
                     expression.write(settings, definition, adapter);
 
                     if (link.load) {
-                        // storeValueType may be different from after, so use storeValueType if last is an ADefLink
+                        WriterUtility.writeDup(adapter, link.after.sort.size, link.size);
-                        if (last instanceof ADefLink) {
-                            WriterUtility.writeDup(adapter, ((ADefLink)last).storeValueType.sort.size, link.size);
-                        } else {
-                            WriterUtility.writeDup(adapter, link.after.sort.size, link.size);
-                        }
                     }
 
                     link.store(settings, definition, adapter);

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/IDefLink.java

@@ -19,23 +19,9 @@
 
 package org.elasticsearch.painless.node;
 
-import org.elasticsearch.painless.Definition.Type;
-
 /**
- * The superclass for all LDef* (link) nodes that store or return a DEF. (Internal only.)
+ * A marker interface applied to LDef* nodes allowing changes to {@link ALink#after} from outside,
- * For this node it is allowed to change {@link ALink#after} from outside, by default
+ * by default {@code after} is {@code DEF}.
- * {@code after} is {@code DEF}.
  */
-abstract class ADefLink extends ALink {
+interface IDefLink {
-    
-    /**
-     * The type of the original type that was pushed on stack, set by {@link EChain} during analyze.
-     * This value is only used for writing the 'store' bytecode, otherwise ignored.
-     */
-    Type storeValueType = null;
-
-    ADefLink(final int line, final String location, final int size) {
-        super(line, location, size);
-    }
-
 }

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefArray.java

@@ -31,7 +31,7 @@ import static org.elasticsearch.painless.WriterConstants.DEF_BOOTSTRAP_HANDLE;
 /**
  * Represents an array load/store or shortcut on a def type.  (Internal only.)
  */
-final class LDefArray extends ADefLink {
+final class LDefArray extends ALink implements IDefLink {
 
     AExpression index;
 
@@ -65,12 +65,8 @@ final class LDefArray extends ADefLink {
 
     @Override
     void store(final CompilerSettings settings, final Definition definition, final GeneratorAdapter adapter) {
-        if (storeValueType == null) {
-            throw new IllegalStateException(error("Illegal tree structure."));
-        }
-
         final String desc = Type.getMethodDescriptor(definition.voidType.type, definition.defType.type,
-            index.actual.type, storeValueType.type);
+            index.actual.type, after.type);
         adapter.invokeDynamic("arrayStore", desc, DEF_BOOTSTRAP_HANDLE, DefBootstrap.ARRAY_STORE);
     }
 }

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefCall.java

@@ -32,7 +32,7 @@ import static org.elasticsearch.painless.WriterConstants.DEF_BOOTSTRAP_HANDLE;
 /**
  * Represents a method call made on a def type. (Internal only.)
  */
-final class LDefCall extends ADefLink {
+final class LDefCall extends ALink implements IDefLink {
 
     final String name;
     final List<AExpression> arguments;

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/LDefField.java

@@ -31,7 +31,7 @@ import static org.elasticsearch.painless.WriterConstants.DEF_BOOTSTRAP_HANDLE;
 /**
  * Represents a field load/store or shortcut on a def type.  (Internal only.)
  */
-final class LDefField extends ADefLink {
+final class LDefField extends ALink implements IDefLink {
 
     final String value;
 
@@ -62,10 +62,7 @@ final class LDefField extends ADefLink {
 
     @Override
     void store(final CompilerSettings settings, final Definition definition, final GeneratorAdapter adapter) {
-        if (storeValueType == null) {
+        final String desc = Type.getMethodDescriptor(definition.voidType.type, definition.defType.type, after.type);
-            throw new IllegalStateException(error("Illegal tree structure."));
-        }
-        final String desc = Type.getMethodDescriptor(definition.voidType.type, definition.defType.type, storeValueType.type);
         adapter.invokeDynamic(value, desc, DEF_BOOTSTRAP_HANDLE, DefBootstrap.STORE);
     }
 }

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/package-info.java

@@ -28,7 +28,6 @@
  * <p>
  * The following is a brief description of each node:
  * {@link org.elasticsearch.painless.node.AExpression} - The superclass for all E* (expression) nodes.
- * {@link org.elasticsearch.painless.node.ADefLink} - The superclass for all LDef* (link) nodes.
  * {@link org.elasticsearch.painless.node.ALink} - The superclass for all L* (link) nodes.
  * {@link org.elasticsearch.painless.node.ANode} - The superclass for all other nodes.
  * {@link org.elasticsearch.painless.node.AStatement} - The superclass for all S* (statement) nodes.
@@ -45,6 +44,7 @@
  * {@link org.elasticsearch.painless.node.ENull} - Represents a null constant.
  * {@link org.elasticsearch.painless.node.ENumeric} - Respresents a non-decimal numeric constant.
  * {@link org.elasticsearch.painless.node.EUnary} - Represents a unary math expression.
+ * {@link org.elasticsearch.painless.node.IDefLink} - A marker interface for all LDef* (link) nodes.
  * {@link org.elasticsearch.painless.node.LArrayLength} - Represents an array length field load.
  * {@link org.elasticsearch.painless.node.LBrace} - Represents an array load/store or defers to possible shortcuts.
  * {@link org.elasticsearch.painless.node.LCall} - Represents a method call or deferes to a def call.

modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOptimizationTests.java

@@ -22,205 +22,160 @@ package org.elasticsearch.painless;
 public class DefOptimizationTests extends ScriptTestCase {
     public void testIntBraceArrayOptiLoad() {
         final String script = "int x = 0; def y = new int[1]; y[0] = 5; x = y[0]; return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)I"));
         assertEquals(5, exec(script));
     }
 
     public void testIntBraceArrayOptiStore() {
         final String script = "int x = 1; def y = new int[1]; y[0] = x; return y[0];";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayStore(Ljava/lang/Object;II)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayStore(Ljava/lang/Object;II)"));
         assertEquals(1, exec(script));
     }
 
     public void testIntBraceListOptiLoad() {
         final String script = "int x = 0; def y = new ArrayList(); y.add(5); x = y[0]; return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)I"));
         assertEquals(5, exec(script));
     }
 
     public void testIntBraceListOptiStore() {
         final String script = "int x = 1; def y = new ArrayList(); y.add(0); y[0] = x; return y[0];";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayStore(Ljava/lang/Object;II)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayStore(Ljava/lang/Object;II)"));
         assertEquals(1, exec(script));
     }
 
     public void testIntBraceMapOptiLoad() {
         final String script = "int x = 0; def y = new HashMap(); y.put(0, 5); x = y[0];";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)I"));
         assertEquals(5, exec(script));
     }
 
     public void testIntBraceMapOptiStore() {
         final String script = "int x = 1; def y = new HashMap(); y.put(0, 1); y[0] = x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayStore(Ljava/lang/Object;II)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayStore(Ljava/lang/Object;II)"));
         assertEquals(1, exec(script));
     }
 
     public void testIntFieldListOptiLoad() {
         final String script = "int x = 0; def y = new ArrayList(); y.add(5); x = y.0;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;)I"));
         assertEquals(5, exec(script));
     }
 
     public void testIntFieldListOptiStore() {
         final String script = "int x = 1; def y = new ArrayList(); y.add(0); y.0 = x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;I)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;I)"));
         assertEquals(1, exec(script));
     }
 
     public void testIntFieldMapOptiLoad() {
         final String script = "int x = 0; def y = new HashMap(); y.put('0', 5); x = y.0; return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;)I"));
         assertEquals(5, exec(script));
     }
 
     public void testIntFieldMapOptiStore() {
         final String script = "int x = 1; def y = new HashMap(); y.put('0', 1); y.0 = x; return y.0;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;I)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;I)"));
         assertEquals(1, exec(script));
     }
 
     public void testIntCall0Opti() {
         final String script = "int x; def y = new HashMap(); y['int'] = 1; x = y.get('int'); return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I"));
         assertEquals(1, exec(script));
     }
 
     public void testIntCall1Opti() {
         final String script = "int x; def y = new HashMap(); y['int'] = 1; x = y.get('int');";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I"));
         assertEquals(1, exec(script));
     }
 
     public void testDoubleBraceArrayOptiLoad() {
         final String script = "double x = 0; def y = new double[1]; y[0] = 5.0; x = y[0]; return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)D");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)D"));
         assertEquals(5.0, exec(script));
     }
 
     public void testDoubleBraceArrayOptiStore() {
         final String script = "double x = 1; def y = new double[1]; y[0] = x; return y[0];";
-        final String asm = Debugger.toString(script);
 
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayStore(Ljava/lang/Object;ID)"));
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayStore(Ljava/lang/Object;ID)");
         assertEquals(1.0, exec(script));
     }
 
     public void testDoubleBraceListOptiLoad() {
         final String script = "double x = 0.0; def y = new ArrayList(); y.add(5.0); x = y[0]; return x;";
-        final String asm = Debugger.toString(script);
 
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)D"));
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)D");
         assertEquals(5.0, exec(script));
     }
 
     public void testDoubleBraceListOptiStore() {
         final String script = "double x = 1.0; def y = new ArrayList(); y.add(0.0); y[0] = x; return y[0];";
-        final String asm = Debugger.toString(script);
 
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayStore(Ljava/lang/Object;ID)"));
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayStore(Ljava/lang/Object;ID)");
         assertEquals(1.0, exec(script));
     }
 
     public void testDoubleBraceMapOptiLoad() {
         final String script = "double x = 0.0; def y = new HashMap(); y.put(0, 5.0); x = y[0];";
-        final String asm = Debugger.toString(script);
 
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)D"));
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayLoad(Ljava/lang/Object;I)D");
         assertEquals(5.0, exec(script));
     }
 
     public void testDoubleBraceMapOptiStore() {
         final String script = "double x = 1.0; def y = new HashMap(); y.put(0, 2.0); y[0] = x;";
-        final String asm = Debugger.toString(script);
 
-        assertTrue(asm.contains("INVOKEDYNAMIC arrayStore(Ljava/lang/Object;ID)"));
+        assertBytecodeExists(script, "INVOKEDYNAMIC arrayStore(Ljava/lang/Object;ID)");
         assertEquals(1.0, exec(script));
     }
 
     public void testDoubleFieldListOptiLoad() {
         final String script = "double x = 0; def y = new ArrayList(); y.add(5.0); x = y.0;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;)D");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;)D"));
         assertEquals(5.0, exec(script));
     }
 
     public void testDoubleFieldListOptiStore() {
         final String script = "double x = 1.0; def y = new ArrayList(); y.add(0); y.0 = x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;D)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;D)"));
         assertEquals(1.0, exec(script));
     }
 
     public void testDoubleFieldMapOptiLoad() {
         final String script = "double x = 0; def y = new HashMap(); y.put('0', 5.0); x = y.0; return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;)D");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;)D"));
         assertEquals(5.0, exec(script));
     }
 
     public void testDoubleFieldMapOptiStore() {
         final String script = "double x = 1.0; def y = new HashMap(); y.put('0', 1.0); y.0 = x; return y.0;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC 0(Ljava/lang/Object;D)");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC 0(Ljava/lang/Object;D)"));
         assertEquals(1.0, exec(script));
     }
 
     public void testDoubleCall0Opti() {
         final String script = "double x; def y = new HashMap(); y['double'] = 1.0; x = y.get('double'); return x;";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)D");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)D"));
         assertEquals(1.0, exec(script));
     }
 
     public void testDoubleCall1Opti() {
         final String script = "double x; def y = new HashMap(); y['double'] = 1.0; x = y.get('double');";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)D");
-
-        assertTrue(asm.contains("INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)D"));
         assertEquals(1.0, exec(script));
     }
 
     public void testIllegalCast() {
         final String script = "int x;\ndef y = new HashMap();\ny['double'] = 1.0;\nx = y.get('double');\n";
-        final String asm = Debugger.toString(script);
+        assertBytecodeExists(script, "INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I");
 
-        assertTrue(asm.contains("INVOKEDYNAMIC get(Ljava/lang/Object;Ljava/lang/String;)I"));
         final Exception exception = expectThrows(ClassCastException.class, () -> {
             exec(script);
         });
-
         assertTrue(exception.getMessage().contains("Cannot cast java.lang.Double to java.lang.Integer"));
     }
 }

modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java

@@ -57,4 +57,13 @@ public abstract class ScriptTestCase extends ESTestCase {
         CompiledScript compiled = new CompiledScript(ScriptService.ScriptType.INLINE, getTestName(), "painless", object);
         return scriptEngine.executable(compiled, vars).run();
     }
+
+    /**
+     * Uses the {@link Debugger} to get the bytecode output for a script and compare
+     * it against an expected bytecode passed in as a String.
+     */
+    public void assertBytecodeExists(String script, String bytecode) {
+        final String asm = Debugger.toString(script);
+        assertTrue("bytecode not found", asm.contains(bytecode));
+    }
 }