FirelyTeam / firely-cql-sdk

BSD 3-Clause "New" or "Revised" License
30 stars 17 forks source link

No conversion from <type> to Hl7.Fhir.Model.Quantity #584

Open baseTwo opened 2 days ago

baseTwo commented 2 days ago

The following types cannot convert to Hl7.Fhir.Model.Quantity:

ewoutkramer commented 2 days ago

See Slack:

No, it's that somewhere along the way, we've started calling Convert when narrowing a choice type into one of its possibilities, which is probably incorrect behavior. 13:43 I updated the As operator to sometimes use ChangeType because at the C# level we have some type problems because our C# type model and the CQL type model aren't 1:1 13:44 But here, we are calling the specific Convert operator which you can actually call for real in CQL source code and I don't see any justification for it 13:45 The intent of the CQL is to check only Observation resources whose value is expressed as a Quantity: and BMIRatio.value as Quantity is not null 13:45 I would have used an is operator here, but this construct is also valid 13:46 Observation.value[x] is a simple choice type, of which FHIR Quantity is one of them 13:46 We should just be doing a C# as here because Observation.value's runtime value could be a FHIR Quantity. (edited) 13:46 However: 13:47 It is possible that this is happening because we have 2 different model types in play here 13:47 e.g., if Vonk is using a different class for Quantity than the one our TypeResolver is returning for the Firely SDK, then we are gonna have problems 13:48 And we would need to have all of those conversions, but only in Vonk context, meaning we would want Vonk to have custom converters that it adds to the type conversion list because it knows it's not using the exact SDK quantity clases and needs to be able to convert freely between them 13:50 At the time this offending expression was created when converting ELM to C#, we should know that Observation.value is a Choice<FHIR.Quantity, ...> (edited) 13:52 and we know the type in the as CQL expression is FHIR.Quantity. Since we are narrowing a choice type, we should be invoking our .NET As operator. It will be try to use our ChangeType function to perform the change and discover there isn't a conversion, at which point it should fall back to C# as and issue a warning 13:52 I thought that is how I changed the As operator to work, but maybe not

ewoutkramer commented 2 days ago

The main bit is this:

and BMIRatio.value as Quantity is not null

What the author is doing here is equivalent to an is, but we incorrectly try to find a cast for it, and upon not having one, reporting that we are missing a cast. Instead, we should return null.

A deeper question is: does this as actually try to call implicit conversions? We think not. So you would not call a cast. But then again, we use the casts to allow casts that should be possible because of subtyping, also for the cases where the POCO subtyping is not 1-on-1 with the CQL subtyping. So if we don't call it, we introduce another bug where perfectly valid as statements would not work anymore.

baseTwo commented 2 days ago

Start by writing a unit test for define function test(obs FHIR.Observation): obs.value as Quantity

ewoutkramer commented 2 days ago

Evan suggested this test:

define f: 1 as System.Long
does not compile
define function test(obs FHIR.Observation): obs.value as Quantity (edited)