Use type specifiers from signature for function ref resolution, if pr…

…esent. Raise exception if resolution is ambiguous. (#1410)

* #1408: Resolve function refs using signatures if present

* Run spotless

* Fix tests

* Fix tests

* Fix tests

* Add tests

* Add tests

* Simplify jacocoTestReport task config (#1411)

* Trigger Codecov

Src/java/elm/src/main/java/org/cqframework/cql/elm/evaluating/SimpleElmEvaluator.java

@@ -1,6 +1,8 @@
 package org.cqframework.cql.elm.evaluating;
 
+import javax.xml.namespace.QName;
 import org.hl7.elm.r1.Expression;
+import org.hl7.elm.r1.TypeSpecifier;
 
 public class SimpleElmEvaluator {
 
@@ -41,4 +43,12 @@ public static boolean dateRangesEqual(Expression left, Expression right) {
     public static boolean codesEqual(Expression left, Expression right) {
         return engine.codesEqual(left, right);
     }
+
+    public static boolean typeSpecifiersEqual(TypeSpecifier left, TypeSpecifier right) {
+        return engine.typeSpecifiersEqual(left, right);
+    }
+
+    public static boolean qnamesEqual(QName left, QName right) {
+        return engine.qnamesEqual(left, right);
+    }
 }

Src/java/engine-fhir/build.gradle

@@ -26,17 +26,15 @@ generateSources {
 }
 
 jacocoTestReport {
-    dependsOn ':cql-to-elm:test'
-    dependsOn ':engine:test'
-    dependsOn ':engine-fhir:test'
-
     sourceDirectories.setFrom(files(
+        "${projectDir}/../elm/src/main/java",
         "${projectDir}/../cql-to-elm/src/main/java",
         "${projectDir}/../engine/src/main/java",
         "${projectDir}/../engine-fhir/src/main/java",
     ))
 
     classDirectories.setFrom(files(
+        "${projectDir}/../elm/build/classes/java/main",
         "${projectDir}/../cql-to-elm/build/classes/java/main",
         "${projectDir}/../engine/build/classes/java/main",
         "${projectDir}/../engine-fhir/build/classes/java/main",

Src/java/engine/build.gradle

@@ -12,15 +12,14 @@ dependencies {
 }
 
 jacocoTestReport {
-    dependsOn ':cql-to-elm:test'
-    dependsOn ':engine:test'
-
     sourceDirectories.setFrom(files(
+            "${projectDir}/../elm/src/main/java",
             "${projectDir}/../cql-to-elm/src/main/java",
             "${projectDir}/../engine/src/main/java",
     ))
 
     classDirectories.setFrom(files(
+            "${projectDir}/../elm/build/classes/java/main",
             "${projectDir}/../cql-to-elm/build/classes/java/main",
             "${projectDir}/../engine/build/classes/java/main",
     ))

Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluator.java

@@ -3,11 +3,10 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.cqframework.cql.elm.evaluating.SimpleElmEvaluator;
 import org.cqframework.cql.elm.visiting.ElmLibraryVisitor;
-import org.hl7.elm.r1.Expression;
-import org.hl7.elm.r1.FunctionDef;
-import org.hl7.elm.r1.FunctionRef;
-import org.hl7.elm.r1.TypeSpecifier;
+import org.hl7.elm.r1.*;
 import org.opencds.cqf.cql.engine.exception.CqlException;
 import org.opencds.cqf.cql.engine.execution.Libraries;
 import org.opencds.cqf.cql.engine.execution.State;
@@ -65,7 +64,7 @@ protected static FunctionDef resolveOrCacheFunctionDef(
             }
         }
 
-        FunctionDef functionDef = resolveFunctionDef(state, functionRef, arguments);
+        FunctionDef functionDef = resolveFunctionRef(state, functionRef, arguments);
 
         if (eligibleForCaching) {
             state.getCache().getFunctionCache().put(functionRef, functionDef);
@@ -74,58 +73,97 @@ protected static FunctionDef resolveOrCacheFunctionDef(
         return functionDef;
     }
 
-    protected static FunctionDef resolveFunctionDef(State state, FunctionRef functionRef, ArrayList<Object> arguments) {
-        return resolveFunctionRef(state, functionRef.getName(), arguments, functionRef.getSignature());
-    }
+    protected static FunctionDef resolveFunctionRef(State state, FunctionRef functionRef, List<Object> arguments) {
+        var name = functionRef.getName();
+        var signature = functionRef.getSignature();
 
-    public static FunctionDef resolveFunctionRef(
-            State state, final String name, final List<Object> arguments, final List<TypeSpecifier> signature) {
-        FunctionDef ret;
+        var functionDefs = resolveFunctionRef(state, name, arguments, signature);
 
-        final List<? extends Object> types = signature.isEmpty() ? arguments : signature;
+        return pickFunctionDef(state, name, arguments, signature, functionDefs);
+    }
 
-        ret = getResolvedFunctionDef(state, name, types, !signature.isEmpty());
+    static List<FunctionDef> resolveFunctionRef(
+            State state, String name, List<Object> arguments, List<TypeSpecifier> signature) {
+        var namedDefs = Libraries.getFunctionDefs(name, state.getCurrentLibrary());
 
-        if (ret != null) {
-            return ret;
+        // If the function ref includes a signature, use the signature to find the matching function defs
+        if (!signature.isEmpty()) {
+            return namedDefs.stream()
+                    .filter(x -> functionDefOperandsSignatureEqual(x, signature))
+                    .collect(Collectors.toList());
         }
 
-        throw new CqlException(String.format(
-                "Could not resolve call to operator '%s(%s)' in library '%s'.",
-                name,
-                getUnresolvedMessage(state, types, name),
-                state.getCurrentLibrary().getIdentifier().getId()));
+        logger.debug(
+                "Using runtime function resolution for '{}'. It's recommended to always include signatures in ELM",
+                name);
+
+        return namedDefs.stream()
+                .filter(x -> state.getEnvironment().matchesTypes(x, arguments))
+                .collect(Collectors.toList());
     }
 
-    private static FunctionDef getResolvedFunctionDef(
-            State state, final String name, final List<? extends Object> types, final boolean hasSignature) {
-        var namedDefs = Libraries.getFunctionDefs(name, state.getCurrentLibrary());
+    static boolean functionDefOperandsSignatureEqual(FunctionDef functionDef, List<TypeSpecifier> signature) {
+        var operands = functionDef.getOperand();
 
-        var candidateDefs = namedDefs.stream()
-                .filter(x -> x.getOperand().size() == types.size())
-                .collect(Collectors.toList());
+        // Check if the number of operands match and if the type specifiers match
+        return operands.size() == signature.size()
+                && IntStream.range(0, operands.size())
+                        .allMatch(i -> operandDefTypeSpecifierEqual(operands.get(i), signature.get(i)));
+    }
 
-        if (candidateDefs.size() == 1) {
-            return candidateDefs.get(0);
+    static boolean operandDefTypeSpecifierEqual(OperandDef operandDef, TypeSpecifier typeSpecifier) {
+        // An operand def can have an operandTypeSpecifier or operandType
+
+        var operandDefOperandTypeSpecifier = operandDef.getOperandTypeSpecifier();
+        if (operandDefOperandTypeSpecifier != null) {
+            return SimpleElmEvaluator.typeSpecifiersEqual(operandDefOperandTypeSpecifier, typeSpecifier);
         }
 
-        if (candidateDefs.size() > 1 && !hasSignature) {
-            logger.debug(
-                    "Using runtime function resolution for '{}'. It's recommended to always include signatures in ELM",
-                    name);
+        if (typeSpecifier instanceof NamedTypeSpecifier) {
+            return SimpleElmEvaluator.qnamesEqual(
+                    operandDef.getOperandType(), ((NamedTypeSpecifier) typeSpecifier).getName());
         }
 
-        return candidateDefs.stream()
-                .filter(x -> state.getEnvironment().matchesTypes(x, types))
-                .findFirst()
-                .orElse(null);
+        return false;
     }
 
-    private static String getUnresolvedMessage(State state, List<? extends Object> arguments, String name) {
+    static FunctionDef pickFunctionDef(
+            State state,
+            String name,
+            List<Object> arguments,
+            List<TypeSpecifier> signature,
+            List<FunctionDef> functionDefs) {
+        var types = signature.isEmpty() ? arguments : signature;
+
+        if (functionDefs.isEmpty()) {
+            throw new CqlException(String.format(
+                    "Could not resolve call to operator '%s(%s)' in library '%s'.",
+                    name,
+                    typesToString(state, types),
+                    state.getCurrentLibrary().getIdentifier().getId()));
+        }
+
+        if (functionDefs.size() == 1) {
+            // Normal case
+            return functionDefs.get(0);
+        }
+
+        throw new CqlException(String.format(
+                "Ambiguous call to operator '%s(%s)' in library '%s'.",
+                name,
+                typesToString(state, types),
+                state.getCurrentLibrary().getIdentifier().getId()));
+    }
+
+    static String typesToString(State state, List<? extends Object> arguments) {
         StringBuilder argStr = new StringBuilder();
         if (arguments != null) {
-            arguments.forEach(a -> argStr.append((argStr.length() > 0) ? ", " : "")
-                    .append(state.getEnvironment().resolveType(a).getTypeName()));
+            arguments.forEach(a -> {
+                argStr.append((argStr.length() > 0) ? ", " : "");
+
+                Class<?> type = state.getEnvironment().resolveType(a);
+                argStr.append(type == null ? "null" : type.getTypeName());
+            });
         }
 
         return argStr.toString();

Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluatorTest.java

@@ -0,0 +1,66 @@
+package org.opencds.cqf.cql.engine.elm.executing;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.Arrays;
+import java.util.List;
+import javax.xml.namespace.QName;
+import org.hl7.elm.r1.*;
+import org.junit.jupiter.api.Test;
+import org.opencds.cqf.cql.engine.exception.CqlException;
+import org.opencds.cqf.cql.engine.execution.Environment;
+import org.opencds.cqf.cql.engine.execution.State;
+
+class FunctionRefEvaluatorTest {
+    @Test
+    void pickFunctionDef() {
+        var env = new Environment(null);
+        var state = new State(env);
+        state.init(new Library().withIdentifier(new VersionedIdentifier().withId("lib")));
+
+        var cqlException = assertThrows(
+                CqlException.class,
+                () -> FunctionRefEvaluator.pickFunctionDef(state, "func", List.of(1, 2, 3), List.of(), List.of()));
+        assertEquals(
+                "Could not resolve call to operator 'func(java.lang.Integer, java.lang.Integer, java.lang.Integer)' in library 'lib'.",
+                cqlException.getMessage());
+    }
+
+    @Test
+    void functionDefOperandsSignatureEqual() {
+        var functionDefWithOneOperand = new FunctionDef().withOperand(new OperandDef());
+        List<TypeSpecifier> signatureWithTwoOperands = List.of(new NamedTypeSpecifier(), new NamedTypeSpecifier());
+
+        assertFalse(FunctionRefEvaluator.functionDefOperandsSignatureEqual(
+                functionDefWithOneOperand, signatureWithTwoOperands));
+    }
+
+    @Test
+    void operandDefTypeSpecifierEqual() {
+        var integerTypeName = new QName("urn:hl7-org:elm-types:r1", "Integer");
+        var integerNamedTypeSpecifier = new NamedTypeSpecifier().withName(integerTypeName);
+        var listTypeSpecifier = new ListTypeSpecifier().withElementType(integerNamedTypeSpecifier);
+
+        var listOperandDef = new OperandDef().withOperandTypeSpecifier(listTypeSpecifier);
+        var integerOperandDef = new OperandDef().withOperandType(integerTypeName);
+
+        assertTrue(FunctionRefEvaluator.operandDefTypeSpecifierEqual(listOperandDef, listTypeSpecifier));
+        assertTrue(FunctionRefEvaluator.operandDefTypeSpecifierEqual(integerOperandDef, integerNamedTypeSpecifier));
+        assertFalse(FunctionRefEvaluator.operandDefTypeSpecifierEqual(integerOperandDef, null));
+    }
+
+    @Test
+    void typesToString() {
+        var env = new Environment(null);
+        var state = new State(env);
+
+        var actual = FunctionRefEvaluator.typesToString(state, List.of("a", "b", "c"));
+        assertEquals("java.lang.String, java.lang.String, java.lang.String", actual);
+
+        actual = FunctionRefEvaluator.typesToString(state, Arrays.asList(1, 2, null));
+        assertEquals("java.lang.Integer, java.lang.Integer, null", actual);
+
+        actual = FunctionRefEvaluator.typesToString(state, null);
+        assertEquals("", actual);
+    }
+}

Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlFunctionTest.java

@@ -4,12 +4,18 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 
+import org.cqframework.cql.cql2elm.CqlCompilerOptions;
+import org.cqframework.cql.cql2elm.LibraryBuilder;
 import org.junit.jupiter.api.Test;
 
 class CqlFunctionTest extends CqlTestBase {
 
     @Test
     void all_function_tests() {
+        var compilerOptions =
+                CqlCompilerOptions.defaultOptions().withSignatureLevel(LibraryBuilder.SignatureLevel.Overloads);
+        var engine = getEngine(compilerOptions);
+
         var results = engine.evaluate(toElmIdentifier("CqlFunctionTests"));
         var value = results.forExpression("FunctionTestStringArg").value();
         assertThat(value, is("hello"));

Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloadsTest.java

@@ -1,10 +1,13 @@
 package org.opencds.cqf.cql.engine.execution;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import org.cqframework.cql.cql2elm.CqlCompilerOptions;
+import org.cqframework.cql.cql2elm.LibraryBuilder;
 import org.hl7.elm.r1.VersionedIdentifier;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
+import org.opencds.cqf.cql.engine.exception.CqlException;
 
 @SuppressWarnings("removal")
 class CqlListDistinguishedOverloadsTest extends CqlTestBase {
@@ -13,9 +16,17 @@ class CqlListDistinguishedOverloadsTest extends CqlTestBase {
             new VersionedIdentifier().withId("CqlListDistinguishedOverloads");
 
     @Test
-    @Disabled("There's a bug in the cql engine that is causing it to select the wrong function overload at runtime")
     void list_overload() {
-        var value = engine.expression(library, "Test").value();
+        var compilerOptions = CqlCompilerOptions.defaultOptions();
+
+        var engine1 = getEngine(compilerOptions.withSignatureLevel(LibraryBuilder.SignatureLevel.Overloads));
+        var value = engine1.expression(library, "Test").value();
         assertEquals("1, 2, 3, 4, 5", value);
+
+        var engine2 = getEngine(compilerOptions.withSignatureLevel(LibraryBuilder.SignatureLevel.None));
+        var cqlException = assertThrows(CqlException.class, () -> engine2.expression(library, "Test"));
+        assertEquals(
+                "Ambiguous call to operator 'toString(java.util.List)' in library 'CqlListDistinguishedOverloads'.",
+                cqlException.getMessage());
     }
 }

Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlMainSuiteTest.java

@@ -7,6 +7,7 @@
 import java.util.*;
 import org.cqframework.cql.cql2elm.CqlCompilerException;
 import org.cqframework.cql.cql2elm.CqlCompilerOptions;
+import org.cqframework.cql.cql2elm.LibraryBuilder;
 import org.junit.jupiter.api.Test;
 
 class CqlMainSuiteTest extends CqlTestBase {
@@ -69,6 +70,11 @@ protected CqlCompilerOptions testCompilerOptions() {
         options.getOptions().remove(CqlCompilerOptions.Options.DisableListDemotion);
         options.getOptions().remove(CqlCompilerOptions.Options.DisableListPromotion);
 
+        // When called with the null argument, the toString function in the CqlTestSuite
+        // library can only be unambiguously resolved at runtime if the library is
+        // compiled with signature level set to Overloads or All.
+        options.withSignatureLevel(LibraryBuilder.SignatureLevel.Overloads);
+
         return options;
     }
 

Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlPerformanceIT.java

@@ -8,6 +8,7 @@
 import java.time.ZonedDateTime;
 import java.util.TimeZone;
 import org.cqframework.cql.cql2elm.CqlCompilerOptions;
+import org.cqframework.cql.cql2elm.LibraryBuilder;
 import org.fhir.ucum.UcumException;
 import org.hl7.elm.r1.VersionedIdentifier;
 import org.junit.jupiter.api.Test;
@@ -98,6 +99,10 @@ protected CqlCompilerOptions testCompilerOptions() {
         options.getOptions().remove(CqlCompilerOptions.Options.DisableListDemotion);
         options.getOptions().remove(CqlCompilerOptions.Options.DisableListPromotion);
 
+        // When called with the null argument, the toString function in the CqlPerformanceTest
+        // library can only be unambiguously resolved at runtime if the library is
+        // compiled with signature level set to Overloads or All.
+        options.withSignatureLevel(LibraryBuilder.SignatureLevel.Overloads);
         return options;
     }
 }

Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlFunctionTests.cql

@@ -22,4 +22,7 @@ define FunctionTestNullTupleArg: f3(Tuple { y: null as Integer })
 
 define function f3(x Quantity): x.unit
 define FunctionTestQuantityArg: f3(12'cm')
+
+// Here the call to f3 can only be unambiguously resolved at runtime
+// if the library is compiled with signature level set to Overloads or All
 define FunctionTestNullQuantityArg: f3(null as Quantity)

Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloads.cql

@@ -17,8 +17,12 @@ define function toString(value List<Code>):
   else Combine((value V return toString(V)), ', ')
 
 // This is the function that _should_ be selected at runtime for the
-// List distinguised overloads test. At the time this CQL was written,
-// The engine had a bug that was selecting the List<Code> overload
+// List distinguished overloads test. The engine currently cannot
+// distinguish between the List<Integer> and List<Code> overloads
+// (and throws the error "Ambiguous call to function 'toString'")
+// unless the library is compiled with signature level set to
+// Overloads or All.
+// See also https://github.com/cqframework/clinical_quality_language/issues/1408.
 define function toString(value List<Integer>):
   if value is null
     then 'null'