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.

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

Make all types instantiable from System.Any #1405

Closed
wants to merge 6 commits into from

Conversation

antvaset
Copy link
Contributor

@antvaset antvaset commented Aug 24, 2024

This PR addresses #1401 and unblocks #1402.

Here's what happens currently when this library

library Lib1
define a: Message('x', true, '1', 'Message', 'This is a message')
define b: Message('y', null, '1', 'Message', 'This is a message')

is translated: The OperatorMap inside the System CompiledLibrary initially has one generic signature (T, System.Boolean, System.String, System.String, System.String) and no concrete signatures for the Message operator. When Message('x', true, '1', 'Message', 'This is a message') is visited by the translator, it doesn't find any matching concrete signatures at first, so it adds a new concrete signature (System.String, System.Boolean, System.String, System.String, System.String) to the OperatorMap after making sure the generic signature is instantiable from that concrete signature. This concrete signature is then used to resolve the first Message call. Then, when it visits the second Message('y', null, '1', 'Message', 'This is a message'), it finds the existing matching concrete signature in the OperatorMap and uses that to resolve the call.

Here's what happens currently when this library

library Lib2
define b: Message('y', null, '1', 'Message', 'This is a message')

fails to translate. The OperatorMap inside the System CompiledLibrary initially has no concrete signatures and one generic signature (T, System.Boolean, System.String, System.String, System.String) for the Message operator. When Message('y', null, '1', 'Message', 'This is a message') is visited, it doesn't find any existing matching concrete signatures (same as above). However, the generic signature is not currently instantiable from the concrete signature (System.String, System.Any, System.String, System.String, System.String) because non-any simple types are not instantiable from System.Any (in this case System.Boolean is not instantiable from System.Any). The Message call doesn't resolve because there are no matching signatures.

This PR makes all types instantiable from System.Any which fixes the Message call resolution in the above example.

This change also affects the translation of expressions like Contains(null, 'a'). The Contains operator has two generic signatures (List<T>, T) and (Interval<T>, T), and the expression currently translates to (List<System.Any>, System.String) when list promotion is enabled and fails to translate (Could not resolve call to operator Contains with signature (System.Any,System.String).) when list promotion is disabled. After the change, the expression fails to translate (Ambiguous generic instantiation of operator Contains between signature (list<System.Any>,System.Any) and (interval<System.Any>,System.Any).) both with and without list promotion. This is why I had to modify some existing tests to explicitly cast nulls to lists or intervals to resolve the ambiguity.

This change is also related to #435. Before the change, null = null is translated with this error:

Call to operator Equal(System.Any,System.Any) is ambiguous with:
  - Equal(System.Decimal,System.Decimal)
  - Equal(System.Long,System.Long)
  - Equal(System.Boolean,System.Boolean)
  - Equal(System.Integer,System.Integer)
  - Equal(System.DateTime,System.DateTime)
  - Equal(System.Ratio,System.Ratio)
  - Equal(System.Date,System.Date)
  - Equal(System.String,System.String)
  - Equal(System.Quantity,System.Quantity)
  - Equal(System.Code,System.Code)
  - Equal(list<System.Any>,list<System.Any>)
  - Equal(System.Concept,System.Concept)
  - Equal(System.Time,System.Time)

After the change, the error is:

Ambiguous generic instantiation of operator Equal between signature (interval<System.Any>,interval<System.Any>) and (list<System.Any>,list<System.Any>).

Another issue related to this is #435. Before this change, collapse null fails to translate and gives Could not resolve call to operator Collapse with signature (System.Any,System.Quantity).. After the change, it translates successfully and resolves with Collapse(List<Interval<System.Any>>).

Corresponding PR to update cql-tests: cqframework/cql-tests#49.

Copy link

Formatting check succeeded!

Copy link

codecov bot commented Aug 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.68%. Comparing base (a7c1636) to head (41a8345).
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1405      +/-   ##
============================================
+ Coverage     64.38%   64.68%   +0.29%     
- Complexity     1921     1966      +45     
============================================
  Files           494      494              
  Lines         28020    28042      +22     
  Branches       5559     5569      +10     
============================================
+ Hits          18042    18138      +96     
+ Misses         7737     7647      -90     
- Partials       2241     2257      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@antvaset antvaset marked this pull request as ready for review August 24, 2024 11:57
Copy link
Member

@brynrhodes brynrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs more investigation, I don't understand why we would need to cast the nulls in all list-valued operators as lists in order to prevent the instantiation of an interval. Is it the interplay with list promotion/demotion? Even then shouldn't the operator precedence resolve that?

@JPercival
Copy link
Contributor

JPercival commented Aug 26, 2024

We don't have a full-fledged solution for type unification. We detect the null as Any rather than as a type variable. IOW, for the type equation:

T1 = T2

We solve the first side first and end up with:

Any = T2

Then we traverse the right-side:

Any = List<Integer>

which is compatible and therefore type checks. Instead, what we need to do is check the left-side, realize we can't make a full inference:

X = T2

And then solve the right side:

X = List<Integer>

and then on the left side find the most specific type that satisfies the requirements of the right side, typically the same type.

List<Integer> = List<Integer>

@antvaset
Copy link
Contributor Author

Hi @brynrhodes @JPercival this change only affects the operators that have both list and interval overloads where null are passed in, such as Contains etc. Currently, something like Contains(null, 'a') gives different results depending on whether list promotion is enabled (details above). After the change, it always raises an ambiguity error. Presumably, this should be expected because there's really not enough information in just Contains(null, 'a') because both List and Interval are allowed. Even with list promotion enabled, Contains(null, 'a') shouldn't always resolve as Contains(List<Any>, String) and "forget" about the interval overload, what do you think?

A potential solution for the ambiguity problem for these expressions might be something like "implicit cast precedence"?

Expressions like Exists(null) are still translated without errors and interpreted as Exists(List<Any>), with no casts needed, both with and without list promotion turned on. More examples of where casts for nulls aren't needed:

Flatten(null)       // always translates as Flatten(List<List<Any>>)
IndexOf(null, 5)    // Index(List<Any>, Integer)
SingletonFrom(null) // SingletonFrom(List<Any>)
distinct null       // Distinct(List<Any>)
null = { 'a', 'b', 'c' } // Equal(List<Any>, List<String>)

The last one is interesting. Currently, null = { 'a', 'b', 'c' } is translated as Equal(List<Any>, List<String>) when list promotion is enabled and raises Could not resolve call to operator Equal with signature (System.Any,list<System.String>). when list promotion is disabled. After the change, it's Equal(List<Any>, List<String>) both with list promotion enabled/disabled.

Ideally it should be translated as Equal(List<String>, List<String>) as per @JPercival's comment. I can look into it separately from this.

Copy link

sonarcloud bot commented Oct 14, 2024

@antvaset
Copy link
Contributor Author

Closing in favor of #1428

@antvaset antvaset closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants