diff --git a/Src/java/elm/src/main/java/org/cqframework/cql/elm/evaluating/SimpleElmEvaluator.java b/Src/java/elm/src/main/java/org/cqframework/cql/elm/evaluating/SimpleElmEvaluator.java index d1a838c33..4d9294069 100644 --- a/Src/java/elm/src/main/java/org/cqframework/cql/elm/evaluating/SimpleElmEvaluator.java +++ b/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); + } } diff --git a/Src/java/engine-fhir/build.gradle b/Src/java/engine-fhir/build.gradle index 178c6ba77..3072caf70 100644 --- a/Src/java/engine-fhir/build.gradle +++ b/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", diff --git a/Src/java/engine/build.gradle b/Src/java/engine/build.gradle index 03871059e..03212da9a 100644 --- a/Src/java/engine/build.gradle +++ b/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", )) diff --git a/Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluator.java b/Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluator.java index afca5d3ec..3708d7e0d 100644 --- a/Src/java/engine/src/main/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluator.java +++ b/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 arguments) { - return resolveFunctionRef(state, functionRef.getName(), arguments, functionRef.getSignature()); - } + protected static FunctionDef resolveFunctionRef(State state, FunctionRef functionRef, List arguments) { + var name = functionRef.getName(); + var signature = functionRef.getSignature(); - public static FunctionDef resolveFunctionRef( - State state, final String name, final List arguments, final List signature) { - FunctionDef ret; + var functionDefs = resolveFunctionRef(state, name, arguments, signature); - final List types = signature.isEmpty() ? arguments : signature; + return pickFunctionDef(state, name, arguments, signature, functionDefs); + } - ret = getResolvedFunctionDef(state, name, types, !signature.isEmpty()); + static List resolveFunctionRef( + State state, String name, List arguments, List 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 types, final boolean hasSignature) { - var namedDefs = Libraries.getFunctionDefs(name, state.getCurrentLibrary()); + static boolean functionDefOperandsSignatureEqual(FunctionDef functionDef, List 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 arguments, String name) { + static FunctionDef pickFunctionDef( + State state, + String name, + List arguments, + List signature, + List 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 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(); diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluatorTest.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/elm/executing/FunctionRefEvaluatorTest.java new file mode 100644 index 000000000..177445e00 --- /dev/null +++ b/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 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); + } +} diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlFunctionTest.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlFunctionTest.java index 2305e176f..167e1c173 100644 --- a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlFunctionTest.java +++ b/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")); diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloadsTest.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloadsTest.java index 67f86cfb9..b7859690f 100644 --- a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloadsTest.java +++ b/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()); } } diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlMainSuiteTest.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlMainSuiteTest.java index 42024405e..ff3d157fe 100644 --- a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlMainSuiteTest.java +++ b/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; } diff --git a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlPerformanceIT.java b/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlPerformanceIT.java index 1bc182602..4a447e600 100644 --- a/Src/java/engine/src/test/java/org/opencds/cqf/cql/engine/execution/CqlPerformanceIT.java +++ b/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; } } diff --git a/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlFunctionTests.cql b/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlFunctionTests.cql index 4a81de55b..2647423d5 100644 --- a/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlFunctionTests.cql +++ b/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) \ No newline at end of file diff --git a/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloads.cql b/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloads.cql index 1af5bba41..c733aa7f7 100644 --- a/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloads.cql +++ b/Src/java/engine/src/test/resources/org/opencds/cqf/cql/engine/execution/CqlListDistinguishedOverloads.cql @@ -17,8 +17,12 @@ define function toString(value List): 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 overload +// List distinguished overloads test. The engine currently cannot +// distinguish between the List and List 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): if value is null then 'null'