DROOLS-1701 fix: ranges cannot be (always) promoted to fields (apache#5)

* DROOLS-1701 fix: ranges cannot be (always) promoted to fields it won't work if boundaries are non-constants! * DROOLS-1701 fix: use parenthesized expression in cast (ranges) * DROOLS-1701 Comparable: do not cast in source code, check in RangeImpl -- for now * DROOLS-1701 Use ObjectCreation for numeric constants, factory method otherwise * DROOLS-1701 move range() factory to CompiledFEELSemanticMappings
evacchi · Jun 8, 2018 · 38c2cd8 · 38c2cd8
1 parent 0fe4528
commit 38c2cd8
Show file tree

Hide file tree

Showing 4 changed files with 138 additions and 23 deletions.
diff --git a/...-dmn-feel/src/main/java/org/kie/dmn/feel/codegen/feel11/CompiledFEELSemanticMappings.java b/...-dmn-feel/src/main/java/org/kie/dmn/feel/codegen/feel11/CompiledFEELSemanticMappings.java
@@ -17,18 +17,74 @@
 package org.kie.dmn.feel.codegen.feel11;
 
 import java.math.BigDecimal;
+import java.time.Period;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.kie.dmn.api.feel.runtime.events.FEELEvent;
+import org.kie.dmn.feel.lang.EvaluationContext;
 import org.kie.dmn.feel.lang.ast.InfixOpNode;
+import org.kie.dmn.feel.lang.ast.RangeNode;
+import org.kie.dmn.feel.runtime.Range;
 import org.kie.dmn.feel.runtime.UnaryTest;
+import org.kie.dmn.feel.runtime.events.ASTEventBase;
+import org.kie.dmn.feel.runtime.impl.RangeImpl;
 import org.kie.dmn.feel.util.EvalHelper;
+import org.kie.dmn.feel.util.Msg;
 
 /**
  * The purpose of this class is to offer import .* methods to compiled FEEL classes compiling expressions.
  * Implementing DMN FEEL spec chapter 10.3.2.12 Semantic mappings
  */
 public class CompiledFEELSemanticMappings {
+
+    /**
+     * Represents a [n..m] construct
+     */
+    public static RangeImpl range(
+            EvaluationContext ctx,
+            Range.RangeBoundary lowBoundary,
+            Object lowEndPoint,
+            Object highEndPoint,
+            Range.RangeBoundary highBoundary) {
+
+        Comparable left = asComparable(lowEndPoint);
+        Comparable right = asComparable(highEndPoint);
+        if (left == null || right == null || !compatible(left, right)) {
+            ctx.notifyEvt( () -> new ASTEventBase(
+                    FEELEvent.Severity.ERROR,
+                    Msg.createMessage(
+                            Msg.INCOMPATIBLE_TYPE_FOR_RANGE,
+                            left.getClass().getSimpleName()
+                    ),
+                    null));
+            return null;
+        }
+        return new RangeImpl(
+                lowBoundary,
+                left,
+                right,
+                highBoundary);
+    }
+
+    private static boolean compatible(Comparable left, Comparable right) {
+        Class<?> leftClass = left.getClass();
+        Class<?> rightClass = right.getClass();
+        return leftClass.isAssignableFrom(rightClass)
+                || rightClass.isAssignableFrom(leftClass);
+    }
+
+    private static Comparable asComparable(Object s) {
+        if (s instanceof Comparable) {
+            return (Comparable) s;
+        } else if (s instanceof Period) {
+            // period has special semantics
+            return new RangeNode.ComparablePeriod((Period) s);
+        } else {
+            // FIXME report error
+            return null;
+        }
+    }
 
     /**
      * Represent a [e1, e2, e3] construct.

diff --git a/...dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/codegen/feel11/DirectCompilerVisitor.java b/...dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/codegen/feel11/DirectCompilerVisitor.java
@@ -22,6 +22,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Deque;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -91,6 +92,12 @@ public class DirectCompilerVisitor extends FEEL_1_1BaseVisitor<DirectCompilerRes
     private static final Expression EMPTY_LIST = JavaParser.parseExpression("java.util.Collections.emptyList()");
     private static final Expression EMPTY_MAP = JavaParser.parseExpression("java.util.Collections.emptyMap()");
     private static final Expression ANONYMOUS_STRING_LITERAL = new StringLiteralExpr("<anonymous>");
+    private static final Expression BOUNDARY_CLOSED = JavaParser.parseExpression(org.kie.dmn.feel.runtime.Range.RangeBoundary.class.getCanonicalName() + ".CLOSED");
+    private static final Expression BOUNDARY_OPEN = JavaParser.parseExpression(org.kie.dmn.feel.runtime.Range.RangeBoundary.class.getCanonicalName() + ".OPEN");
+
+    private static final org.drools.javaparser.ast.type.Type TYPE_COMPARABLE =
+            JavaParser.parseType(Comparable.class.getCanonicalName());
+
     // TODO as this is now compiled it might not be needed for this compilation strategy, just need the layer 0 of input Types, but to be checked.
     private ScopeHelper scopeHelper;
     private boolean replaceEqualForUnaryTest = false;
@@ -479,34 +486,82 @@ public DirectCompilerResult visitExpressionList(FEEL_1_1Parser.ExpressionListCon
     public DirectCompilerResult visitInterval(FEEL_1_1Parser.IntervalContext ctx) {
         DirectCompilerResult start = visit(ctx.start);
         DirectCompilerResult end = visit(ctx.end);
-        RangeNode.IntervalBoundary low = ctx.low.getText().equals("[") ? RangeNode.IntervalBoundary.CLOSED : RangeNode.IntervalBoundary.OPEN;
-        RangeNode.IntervalBoundary up = ctx.up.getText().equals("]") ? RangeNode.IntervalBoundary.CLOSED : RangeNode.IntervalBoundary.OPEN;
 
-        Expression BOUNDARY_CLOSED = JavaParser.parseExpression(org.kie.dmn.feel.runtime.Range.RangeBoundary.class.getCanonicalName() + ".CLOSED");
-        Expression BOUNDARY_OPEN = JavaParser.parseExpression(org.kie.dmn.feel.runtime.Range.RangeBoundary.class.getCanonicalName() + ".OPEN");
+        Expression lowBoundary = expressionBoundaryOf(
+                RangeNode.IntervalBoundary.fromString(
+                        ctx.low.getText()));
+        Expression lowEndPoint = start.getExpression();
+        Expression highEndPoint = end.getExpression();
+        Expression highBoundary = expressionBoundaryOf(
+                RangeNode.IntervalBoundary.fromString(
+                        ctx.up.getText()));
+
+        // if this is a range of type i..j with i,j numbers:
+        // then we make it a constant; otherwise we fallback
+        // to the general case of creating the Range object at runtime
+        if (isNumericConstant(start) && isNumericConstant(end)) {
+            ObjectCreationExpr initializer =
+                    new ObjectCreationExpr()
+                            .setType(JavaParser.parseClassOrInterfaceType(RangeImpl.class.getCanonicalName()))
+                            .addArgument(lowBoundary)
+                            .addArgument(new CastExpr(TYPE_COMPARABLE, lowEndPoint))
+                            .addArgument(new CastExpr(TYPE_COMPARABLE, highEndPoint))
+                            .addArgument(highBoundary);
+
+            FieldDeclaration rangeField =
+                    fieldDeclarationOf(
+                            "RANGE",
+                            ParserHelper.getOriginalText(ctx),
+                            initializer);
+            Set<FieldDeclaration> fieldDeclarations =
+                    DirectCompilerResult.mergeFDs(start, end);
+            fieldDeclarations.add(rangeField);
+
+            return DirectCompilerResult.of(
+                    new NameExpr(
+                            rangeField.getVariable(0)
+                                    .getName().asString()),
+                    BuiltInType.RANGE,
+                    fieldDeclarations);
+        } else {
+            MethodCallExpr initializer =
+                    new MethodCallExpr(
+                            null,
+                            "range", new NodeList<>(
+                            new NameExpr("feelExprCtx"),
+                            lowBoundary,
+                            lowEndPoint,
+                            highEndPoint,
+                            highBoundary));
+
+            return DirectCompilerResult.of(
+                    initializer,
+                    BuiltInType.RANGE,
+                    DirectCompilerResult.mergeFDs(start, end));
+        }
 
-        String originalText = ParserHelper.getOriginalText(ctx);
+    }
 
-        ObjectCreationExpr initializer = new ObjectCreationExpr();
-        initializer.setType(JavaParser.parseClassOrInterfaceType(RangeImpl.class.getCanonicalName()));
-        initializer.addArgument(low == IntervalBoundary.CLOSED ? BOUNDARY_CLOSED : BOUNDARY_OPEN);
-        initializer.addArgument(start.getExpression());
-        initializer.addArgument(end.getExpression());
-        initializer.addArgument(up == IntervalBoundary.CLOSED ? BOUNDARY_CLOSED : BOUNDARY_OPEN);
-        String constantName = "RANGE_" + CodegenStringUtil.escapeIdentifier(originalText);
-        VariableDeclarator vd = new VariableDeclarator(JavaParser.parseClassOrInterfaceType(Range.class.getCanonicalName()), constantName);
-        vd.setInitializer(initializer);
-        FieldDeclaration fd = new FieldDeclaration();
-        fd.setModifier(Modifier.PUBLIC, true);
-        fd.setModifier(Modifier.STATIC, true);
-        fd.setModifier(Modifier.FINAL, true);
-        fd.addVariable(vd);
+    private FieldDeclaration fieldDeclarationOf(String prefix, String originalText, Expression initializer) {
+        String constantName = prefix + "_" + CodegenStringUtil.escapeIdentifier(originalText);
+        return new FieldDeclaration(
+                EnumSet.of(Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL),
+                new VariableDeclarator(
+                        JavaParser.parseClassOrInterfaceType(Range.class.getCanonicalName()),
+                        constantName,
+                        initializer))
+                .setJavadocComment(" FEEL range: " + originalText + " ");
+    }
 
-        fd.setJavadocComment(" FEEL range: " + originalText + " ");
+    private Expression expressionBoundaryOf(IntervalBoundary low) {
+        return low == IntervalBoundary.CLOSED ? BOUNDARY_CLOSED : BOUNDARY_OPEN;
+    }
 
-        DirectCompilerResult directCompilerResult = DirectCompilerResult.of(new NameExpr(constantName), BuiltInType.RANGE, DirectCompilerResult.mergeFDs(start, end));
-        directCompilerResult.addFieldDesclaration(fd);
-        return directCompilerResult;
+    private boolean isNumericConstant(DirectCompilerResult r) {
+        // FIXME: this is a bit arbitrary, maybe we should turn this into a flag in `r`
+        return r.getExpression().isNameExpr() &&
+                r.resultType.equals(BuiltInType.NUMBER) &&
+                r.getFieldDeclarations().size() > 0;
     }
 
     @Override

diff --git a/kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/RangeNode.java b/kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/RangeNode.java
@@ -33,6 +33,9 @@ public class RangeNode
 
     public static enum IntervalBoundary {
         OPEN, CLOSED;
+        public static IntervalBoundary fromString(String input) {
+            return "[".equals(input) || "]".equals(input) ? CLOSED : OPEN;
+        }
     }
 
     private IntervalBoundary lowerBound;

diff --git a/kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/impl/RangeImpl.java b/kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/impl/RangeImpl.java
@@ -26,6 +26,7 @@ public class RangeImpl
     private Comparable    lowEndPoint;
     private Comparable    highEndPoint;
 
+
     public RangeImpl() {
     }
-Original file line number
+Diff line change
@@ Expand Up / @@ -26,6 +26,7 @@ public class RangeImpl @@
         private Comparable    lowEndPoint;
         private Comparable    highEndPoint;
         public RangeImpl() {
         }
@@ Expand Down @@