Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unsuppress ChangeType errors #545

Open
wants to merge 3 commits into
base: develop-2.0
Choose a base branch
from
Open

Unsuppress ChangeType errors #545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32b4f31
WIP
ewoutkramer Sep 16, 2024
57d68f7
Merge branch 'develop-2.0' into unsuppress-changetype-errors
ewoutkramer Sep 16, 2024
95c343b
Fixed or suppressed unit tests that failed after making ChangeType er…
ewoutkramer Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 39 additions & 73 deletions Cql/Cql.Compiler/ExpressionBuilderContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ private Expression TranslateElement(Element element) =>
// NOTE: Do not rename ICqlOperators.CreateValueSetFacade to ExpandValueSet
ExpandValueSet e => _cqlOperatorsBinder.BindToMethod(nameof(ICqlOperators.CreateValueSetFacade), TranslateArgs(GetBindArgs(element)), TranslateTypes(GetTypeArgs(element))),

// Special case for intervals with null boundaries. See https://github.com/FirelyTeam/firely-cql-sdk/issues/543
Interval { low: Null, high: Null } => Expression.Constant(null, typeof(CqlInterval<object>)),

// All other Elm types matches on type name to the ICqlOperators method name
_ => _cqlOperatorsBinder.BindToMethod(element.GetType().Name, TranslateArgs(GetBindArgs(element)), TranslateTypes(GetTypeArgs(element))),
//@formatter:on
Expand All @@ -206,20 +209,7 @@ Expression ConvertToResultType()
var tsType = TypeFor(element.resultTypeSpecifier, false);
if (tsType is not null)
{
var converted = ChangeType(expression, element.resultTypeSpecifier, out var typeConversion);
if (typeConversion != TypeConversion.NoMatch)
return converted;

// If we make this a hard fail, 15 unit tests fail.
// throw this.NewExpressionBuildingException(
// $"Cannot convert {expression.Type.ToCSharpString(Defaults.TypeCSharpFormat)} to {tsType.ToCSharpString(Defaults.TypeCSharpFormat)}");

_logger.LogDebug(
"Failed to change expression '{elementType}' at '{elementLocator}' from type '{expressionType}' to '{resultType}'",
element.GetType().Name,
element.locator,
tsType.ToCSharpString(Defaults.TypeCSharpFormat),
expression.Type.ToCSharpString(Defaults.TypeCSharpFormat));
return ChangeType(expression, element.resultTypeSpecifier, throwOnError:true);
}

return expression;
Expand Down Expand Up @@ -311,11 +301,8 @@ Expression ConvertToResultType()
{
if (leftType != right.Type)
{
var typeConversion = TypeConversion.NoMatch;
if (leftType.IsAssignableFrom(right.Type))
right = ChangeType(right, leftType, out typeConversion);
if (typeConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert Contains target {right.Type.ToCSharpString(Defaults.TypeCSharpFormat)} to {leftType.ToCSharpString(Defaults.TypeCSharpFormat)}");
right = ChangeType(right, leftType, throwOnError: true);
}
return [left, right, e.GetPrecision()];
}
Expand Down Expand Up @@ -577,11 +564,8 @@ protected Expression Instance(Instance ine)
}
var ctor = ConstructorInfos.CqlQuantity;

TypeConversion typeConversion = TypeConversion.NoMatch;
if (unitExpr is not null)
unitExpr = ChangeType(unitExpr, typeof(string), out typeConversion);
if (typeConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Unit property cannot be converted to string.");
unitExpr = ChangeType(unitExpr, typeof(string), throwOnError: true);

var @new = Expression.New(ctor,
valueExpr ?? Expression.Default(typeof(decimal?)),
Expand Down Expand Up @@ -708,10 +692,7 @@ protected MemberAssignment Binding(Expression value, MemberInfo memberInfo)
else
{
var selectParameter = Expression.Parameter(valueEnumerableElement, TypeNameToIdentifier(value.Type, this));
var typeConversion = TypeConversion.NoMatch;
var body = ChangeType(selectParameter, memberArrayElement, out typeConversion);
if (typeConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert {selectParameter.Type} to {memberArrayElement}.");
var body = ChangeType(selectParameter, memberArrayElement, throwOnError: true);
var selectLambda = Expression.Lambda(body, selectParameter);
var callSelectMethod = BindCqlOperator(nameof(ICqlOperators.Select), [value, selectLambda
]);
Expand All @@ -734,10 +715,7 @@ protected MemberAssignment Binding(Expression value, MemberInfo memberInfo)
}
}

var propertyConversion = TypeConversion.NoMatch;
var convert = ChangeType(value, property.PropertyType, out propertyConversion);
if (propertyConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert {value.Type} to {property.PropertyType}.");
var convert = ChangeType(value, property.PropertyType, throwOnError: true);
return Expression.Bind(memberInfo, convert);
}

Expand Down Expand Up @@ -979,9 +957,7 @@ protected Expression Property(Property op)
var resultType = TypeFor(op) ?? throw this.NewExpressionBuildingException(message);
if (resultType != propogate.Type)
{
propogate = ChangeType(propogate, resultType, out var typeConversion);
if (typeConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert {propogate.Type} to {resultType}.");
propogate = ChangeType(propogate, resultType, throwOnError: true);
}

return propogate;
Expand Down Expand Up @@ -1055,17 +1031,11 @@ protected Expression PropertyHelper(Expression source, string? path, Type expect
{
if (expectedType != ifIs.Type)
{
var ifIsConversion = TypeConversion.NoMatch;
ifIs = ChangeType(ifIs, expectedType, out ifIsConversion);
if (ifIsConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert {ifIs.Type} to {expectedType}.");
ifIs = ChangeType(ifIs, expectedType, throwOnError: true);
}
if (expectedType != elseNull.Type)
{
var elseConversion = TypeConversion.NoMatch;
elseNull = ChangeType(elseNull, expectedType, out elseConversion);
if (elseConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert {elseNull.Type} to {expectedType}.");
elseNull = ChangeType(elseNull, expectedType, throwOnError: true);
}
}
var condition = Expression.Condition(isCheck, ifIs, elseNull);
Expand All @@ -1077,10 +1047,7 @@ protected Expression PropertyHelper(Expression source, string? path, Type expect

if (expectedType != null && expectedType != result.Type)
{
var resultConversion = TypeConversion.NoMatch;
result = ChangeType(result, expectedType, out resultConversion);
if (resultConversion == TypeConversion.NoMatch)
throw this.NewExpressionBuildingException($"Cannot convert {result.Type} to {expectedType}.");
result = ChangeType(result, expectedType, throwOnError: true);
}
return result;
}
Expand Down Expand Up @@ -1185,8 +1152,7 @@ Expression convertChoice(Expression argument, TypeSpecifier? targetTypeSpecifier
if(argument.Type == typeof(object)
&& targetTypeSpecifier is not null and not ChoiceTypeSpecifier)
{
var changeType = ChangeType(argument, targetTypeSpecifier, out var typeConversion, considerSafeUpcast: true);
Debug.Assert(typeConversion == TypeConversion.ExpressionTypeAs);
var changeType = ChangeType(argument, targetTypeSpecifier, considerSafeUpcast: true);
return changeType;
}

Expand Down Expand Up @@ -2090,14 +2056,8 @@ protected Expression AggregateClause(
var lambdaBody = TranslateArg(queryAggregate.expression!);
// when starting is not present, it is a null literal typed as Any (object).
// cast the null to the expression type.
var typeConversion = TypeConversion.NoMatch;
var starting = TranslateArg(queryAggregate.starting!);
var startingValue = ChangeType(starting, lambdaBody.Type, out typeConversion);
if (typeConversion == TypeConversion.NoMatch)
{
throw this.NewExpressionBuildingException(
$"Cannot convert starting value of type {starting.Type.ToCSharpString()} to the result type of the aggregate expression ({lambdaBody.Type.ToCSharpString()})");
}
var startingValue = ChangeType(starting, lambdaBody.Type, throwOnError: true);
if (queryAggregate.distinct)
@return = _cqlOperatorsBinder.BindToMethod(nameof(ICqlOperators.Distinct), [@return], [resultType]);
var lambda = Expression.Lambda(lambdaBody, resultParameter, sourceParameter);
Expand Down Expand Up @@ -2258,45 +2218,49 @@ partial class ExpressionBuilderContext
private Expression ChangeType(
Expression expr,
TypeSpecifier? typeSpecifier,
out TypeConversion typeConversion,
bool throwOnError = false,
bool considerSafeUpcast = false) // @TODO: Cast - ChangeType
{
if (typeSpecifier is not null)
{
if (TypeFor(typeSpecifier, false) is { } resultType)
if (TypeFor(typeSpecifier, throwOnError) is { } resultType)
{
if (resultType != expr.Type)
{
var typeAs = ChangeType(expr, resultType, out typeConversion, considerSafeUpcast);
var typeAs = ChangeType(expr, resultType, out _, throwOnError, considerSafeUpcast);
return typeAs;
}
}
else
{
typeConversion = TypeConversion.NoMatch;
return expr;
}
}

typeConversion = TypeConversion.ExactType;
return expr;
}


private Expression ChangeType(
Element element,
Type outputType,
bool throwOnError = false,
bool considerSafeUpcast = false)
=> ChangeType(
TranslateArg(element),
outputType,
out TypeConversion typeConversion,
throwOnError,
considerSafeUpcast); // @TODO: Cast - ChangeType

private Expression ChangeType(
Expression input,
Type outputType,
bool throwOnError = false,
bool considerSafeUpcast = false) =>
ChangeType(input, outputType, out _, throwOnError, considerSafeUpcast); // @TODO: Cast - ChangeType


private Expression ChangeType(
Expression input,
Type outputType,
out TypeConversion typeConversion,
bool throwOnError = false,
bool considerSafeUpcast = false) // @TODO: Cast - ChangeType
{
var (expression, tc) = input.TryNewAssignToTypeExpression(outputType, false, considerSafeUpcast);
Expand All @@ -2311,6 +2275,7 @@ private Expression ChangeType(
{
// unless they're the same type.
typeConversion = input.Type == outputType ? TypeConversion.ExactType : TypeConversion.NoMatch;
throwCannotCastIfNoMatch(typeConversion);
return input;
}

Expand All @@ -2320,23 +2285,24 @@ private Expression ChangeType(
var inputElementType = _typeResolver.GetListElementType(input.Type, true)!;
var outputElementType = _typeResolver.GetListElementType(outputType, true)!;
var lambdaParameter = Expression.Parameter(inputElementType, TypeNameToIdentifier(inputElementType, this));
var lambdaBody = ChangeType(lambdaParameter, outputElementType, out typeConversion);
if (typeConversion != TypeConversion.NoMatch)
{
var lambda = Expression.Lambda(lambdaBody, lambdaParameter);
return BindCqlOperator(nameof(ICqlOperators.Select), input, lambda);
}

typeConversion = TypeConversion.NoMatch;
return input;
var lambdaBody = ChangeType(lambdaParameter, outputElementType, out typeConversion, throwOnError: true);
var lambda = Expression.Lambda(lambdaBody, lambdaParameter);
return BindCqlOperator(nameof(ICqlOperators.Select), input, lambda);
}

Type toType = TryCorrectQiCoreBindingError(input.Type, outputType, out var correctedTo)
? correctedTo!
: outputType;
_cqlOperatorsBinder.TryConvert(input, toType, out (Expression arg, TypeConversion conversion) tryConvert);
typeConversion = tryConvert.conversion;
throwCannotCastIfNoMatch(tryConvert.conversion);
return tryConvert.arg;

void throwCannotCastIfNoMatch(TypeConversion result)
{
if(result == TypeConversion.NoMatch && throwOnError)
throw this.NewExpressionBuildingException($"Cannot convert {input.Type} to {outputType}.");
}
}

}
Expand Down
25 changes: 13 additions & 12 deletions Cql/CqlToElmTests/RefTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public void ValueSet_Local()
}

[TestMethod]
[Ignore("Will fix in https://github.com/FirelyTeam/firely-cql-sdk/issues/519")]
public void CodeSystem_Local()
{
var library = MakeLibrary($@"
Expand Down Expand Up @@ -238,7 +239,7 @@ public void InvokeParameter()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'

parameter x default 'bla'

define {nameof(InvokeParameter)}: x(4)
Expand All @@ -250,7 +251,7 @@ public void InvokeExpression()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'

define pi: 3.14

define {nameof(InvokeExpression)}: pi()
Expand Down Expand Up @@ -334,7 +335,7 @@ public void InvokeIntervalMember()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'

define lowI: Interval[1,3].low
");

Expand All @@ -351,7 +352,7 @@ public void InvokeIntervalClosedMember()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'

define closed: Interval[1,3].highClosed
");

Expand All @@ -368,7 +369,7 @@ public void InvokeTupleMember()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'

define tupleMember: Tuple {{ name: 'Ewout' }}.name
");

Expand Down Expand Up @@ -441,7 +442,7 @@ public void InvokeType()
{
_ = MakeLibrary($@"
library BareMinimum version '0.0.1'

using FHIR
include Math

Expand All @@ -453,7 +454,7 @@ include Math
public void InvokeProperty()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'
library BareMinimum version '0.0.1'
using FHIR

context Patient
Expand All @@ -475,7 +476,7 @@ context Patient
public void InvokeListProperty()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'
library BareMinimum version '0.0.1'
using FHIR

context Patient
Expand All @@ -497,7 +498,7 @@ context Patient
public void InvokeThroughListProperty()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'
library BareMinimum version '0.0.1'
using FHIR

context Patient
Expand All @@ -517,7 +518,7 @@ context Patient
public void InvokeListPropertyThroughListProperty()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'
library BareMinimum version '0.0.1'
using FHIR

context Patient
Expand All @@ -539,7 +540,7 @@ public void InvokeListPropertyViaFunction()
{
var library = MakeLibrary($@"
library BareMinimum version '0.0.1'

using FHIR

context Patient
Expand Down Expand Up @@ -591,4 +592,4 @@ define fluent function getContactName(contact List<FHIR.Patient.Contact>): conta
return Run<T>(library, expressionName, bundle);
}
}
}
}
3 changes: 3 additions & 0 deletions Cql/CqlToElmTests/SkippedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ internal static class SkippedTests

{ "EqualNullNull", "The spec states that null elements are considered equal for list equality." },
{ "ReplaceMatchesSpaces", "The first argument should have four backslashes instead of 2" },

// This compiler error should be repaired by https://github.com/FirelyTeam/firely-cql-sdk/issues/542
{ "Issue32DateTime", "We incorrectly compile a DateTime to an interval without a point type." }
};

internal static Dictionary<string, string> DoesNotMatchExpectation = new()
Expand Down
2 changes: 1 addition & 1 deletion submodules/Firely.Cql.Sdk.Integration.Runner
Open in desktop
Submodule Firely.Cql.Sdk.Integration.Runner updated from 6e39fd to 99b3b3