ELM parser for negation is implemented incorrectly

[ewoutkramer]

When an expression contains the unary '-', our current parser only accepts literals and quantities and uses string concatenation and multiplication to execute the negation. However, negation is not limited to literals as the following is valid CQL:

library BareMinimum version '0.0.1'

using FHIR version '4.0.1'

include cq_core.FHIRHelpers version '4.0.1' called FHIRHelpers

context Patient

define x: 4

define y: -"x"

define z: -5

define q: 4 'kg'

define m: -"q"

The ELM tree should simply use the Negate ELM node instead, and accept any expression with the correct type. See https://cql.hl7.org/04-logicalspecification.html#negate

[EvanMachusak]

To quote the spec (https://cql.hl7.org/04-logicalspecification.html#negate)

The Negate operator is defined for the Integer, Long, Decimal, and Quantity types.

This example:

define y: -"x"

Is most definitely not legal. What is the definition of a negative string?

I implemented the negation unary operator without the Negate element because Negate isn't necessary, as multiplication is supported on all types to which this applies. If we wanted to get fancy, we could implement Negate to do some bit manipulation instead of invoking a multiplication to save a few cycles. If someone wants to take a crack at that I'd accept that PR, but make sure ExpressionBuilder actually implements Negate because it might not.

Keep in mind that there are flaws in the grammar and simply because the grammar allows a particular construct doesn't mean it's actually legal. This is particularly true for complex "sentence" rules which include a lot of optional positional elements. The grammar does not expressly state that "if optional element at position 2 is present, optional element at position 4 is then required" but when you start reading the spec and see how operators are defined it becomes clear that is the case.

[ewoutkramer]

Is most definitely not legal. What is the definition of a negative string?

It's not a string, it's a quoted identifier referring to the define x: 4 above, which is of the negatable type Integer. So, yes, -"x" should work.

In fact, the code above compiles fine in CQLab Sandbox.

[ewoutkramer]

I implemented the negation unary operator without the Negate element because Negate isn't necessary, as multiplication is supported on all types to which this applies.

No, you need Negate because it applies to any expression of the listed types, not any literal of the listed types. You cannot negate a FunctionRef expression, unless you use the Negate node. How would you negate parameters to a function?

Multiplying directly is an optimization for literals in the AST, but not a complete solution.

To show some actual code generated in the sandbox:

library BareMinimum version '0.0.1'

define function Neg(a Integer): -a

which will produce this fragment:

 "expression": {
            "type": "Negate",
            "operand": {
              "type": "OperandRef",          
              "resultTypeName": "{urn:hl7-org:elm-types:r1}Integer",
              "name": "a"
            },
            "resultTypeName": "{urn:hl7-org:elm-types:r1}Integer"
          },