FirelyTeam / firely-cql-sdk

BSD 3-Clause "New" or "Revised" License
28 stars 16 forks source link

How to handle type matching cases #198

Open ewoutkramer opened 6 months ago

ewoutkramer commented 6 months ago

When we encounter case statements like this:

 when Event is AllergyIntolerance then
                  Interval[start of Event.onset.toInterval(), Event.lastOccurrence] overlaps after day of 
                                        ModerateOrSevereLVSDHFOutpatientEncounter.period
                    and Event.clinicalStatus ~ QICoreCommon."allergy-active"
                    and not (Event.verificationStatus ~ QICoreCommon."allergy-unconfirmed"
                        or Event.verificationStatus ~ QICoreCommon."allergy-refuted"
                        or Event.verificationStatus ~ "allergy-entered-in-error")
  when Event is MedicationRequest then
                  First( ( collapse (Event.dosageInstruction.timing.repeat.bounds DoseTime
                                        return DoseTime.toInterval()) ) DrugPeriods
                           sort by start of $this ) overlaps after day of ModerateOrSevereLVSDHFOutpatientEncounter.period
                    and Event.status in { 'active', 'completed' }
                    and Event.intent in { 'order', 'original-order', 'reflex-order', 'filler-order', 'instance-order' }
                    and Event.doNotPerform is not true

Here, it looks like the ExpressionBuilder is supposed to know that inside a when Event is TYPE, the type of Event is actually TYPE. I haven't checked deeply, but I would think this is the task of the CQL->ELM compiler to determine if this is the case, and this should somehow be present in the ELM. At the moment however, the type of Event is unknown, so things like Event.status are determined to be "late bound properties", for which the statically determined type is "Object". This means that when these properties are then used, e.g. in Event.status in { 'active', .... }, the ExpressionBuilder returns a null, instead of a boolean - causing further harm.

I have for now provided a dummy body for this function in CMS AHAOverall.cql, so we can at least continue our work.

EvanMachusak commented 6 months ago

I have raised this issue with Bryn multiple times. The general problem is this:

case
     when foo is Patient then foo.birthDate
     else null
end

The cql-to-elm compiler does not automatically cast foo to Patient in the then clause. You must write the CQL like this:

case
     when foo is Patient then (foo as Patient).birthDate
     else null
end

If not, then foo remains a Choice type, which remains an object in our C# rendition of the ELM, and accessing a property on object ends up in the late bound property searching scenario.

The reason eCQMS get away with not doing this is that they all use the MITRE engine which is an untyped runtime - everything is just a JavaScript var behind the scenes. They are getting away with an untyped language (JavaScript's) property accessing semantics. The CQL spec does clearly allow you to access any property that exists on any of the types in the Choice.

For HEDIS measures, we disallow anyone from declaring Choice anywhere in their code. The only time we interact with Choice types is accessing FHIR properties that are choices. But the eCQM authors don't have that same style restriction.

I would recommend that we ballot a change to the CQL spec with language like this:

When a case item's condition is an is phrase, the operand of the is phrase will be cast (using as) wherever it is located in the corresponding then clause of that case item.

I considered doing this in the ExpressionBuilder myself, but I decided against it since I wanted to be faithful to the spec.

ewoutkramer commented 6 months ago

The reason eCQMS get away with not doing this is that they all use the MITRE engine which is an untyped runtime - everything is just a JavaScript var behind the scenes. They are getting away with an untyped language (JavaScript's) property accessing semantics. The CQL spec does clearly allow you to access any property that exists on any of the types in the Choice.

Yes, so the MITRE engine deals with this at runtime, that's fair, they can do that. But CQL, as far as I know, is a statically typed language, so the CQL->ELM compiler should be validating these - it knows foo is a Patient, so foo.birthDate makes sense. If it does not do that, it must allow dynamic runtime behaviour at that point, basically disabling typesafety for this part of the expression. Asserting that foo is Choice<Patient, whatever> is not making this type safe if you don't know the "type" of foo.birthDate here.

If the compiler already has taken the effort of determining the static type of foo at this point, I would love it to express that in the ELM, so we don't have to do that again. If it assumes "anything goes from here - switch to dynamic dispatch and runtime errors", then that's really interesting to learn, since that means we (and others) should not just rely on static code generation (there you go, CQL on FHIR view builders & transpilers), but should also support late-time binding and type checks (as you do in LateBoundProperty).

ewoutkramer commented 6 months ago

See #193.