Merge pull request #18379 from jdconrad/types

Fix Bug in Painless Assignment

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,27 +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) {
-            final ADefLink lastDef = (ADefLink) last;
+        if (last instanceof IDefLink) {
             expression.analyze(settings, definition, variables);
-            lastDef.storeValueType = expression.expected = expression.actual;
-            this.actual = read ? lastDef.storeValueType : definition.voidType;
+            last.after = expression.expected = expression.actual;
         } 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;
         }
 

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ADefLink.java → 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.)
- * For this node it is allowed to change {@link ALink#after} from outside, by default
- * {@code after} is {@code DEF}.
+ * A marker interface applied to LDef* nodes allowing changes to {@link ALink#after} from outside,
+ * by default {@code after} is {@code DEF}.
  */
-abstract class ADefLink extends ALink {
-    
-    /**
-     * 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);
-    }
-
+interface IDefLink {
 }

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;
 
@@ -41,7 +41,6 @@ final class LDefArray extends ADefLink {
         this.index = index;
     }
 
-
     @Override
     ALink analyze(final CompilerSettings settings, final Definition definition, final Variables variables) {
         index.analyze(settings, definition, variables);
@@ -66,11 +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) {
-            throw new IllegalStateException(error("Illegal tree structure."));
-        }
-        final String desc = Type.getMethodDescriptor(definition.voidType.type, definition.defType.type, storeValueType.type);
+        final String desc = Type.getMethodDescriptor(definition.voidType.type, definition.defType.type, after.type);
         adapter.invokeDynamic(value, desc, DEF_BOOTSTRAP_HANDLE, DefBootstrap.STORE);
     }
 }

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

@@ -44,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/DefTests.java → modules/lang-painless/src/test/java/org/elasticsearch/painless/DefOperationTests.java

@@ -19,7 +19,7 @@
 
 package org.elasticsearch.painless;
 
-public class DefTests extends ScriptTestCase {
+public class DefOperationTests extends ScriptTestCase {
     public void testNot() {
         assertEquals(~1, exec("def x = (byte)1 return ~x"));
         assertEquals(~1, exec("def x = (short)1 return ~x"));

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

@@ -0,0 +1,181 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+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;";
+        assertBytecodeExists(script, "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];";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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];";
+        assertBytecodeExists(script, "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];";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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');";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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];";
+
+        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;";
+
+        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];";
+
+        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];";
+
+        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;";
+
+        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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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;";
+        assertBytecodeExists(script, "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');";
+        assertBytecodeExists(script, "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";
+        assertBytecodeExists(script, "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));
+    }
 }