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

Handle overloaded forward function declarations. #1213

Conversation

lukedegruchy
Copy link
Contributor

@lukedegruchy lukedegruchy commented Sep 20, 2023

  • Enhance CqlPreprocessor visitor to compile function headers (AKA "preCompile")
  • Introduce a new common base class for CqlPreprocessor and Cql2ElmVisitor
  • Extract common methods from Cql2ElmProcessor to the new superclass
  • Store expression definition as a hash generated from the semantic function definition instead of a function String
  • Move loading of the system library from Cql2ElmProcessor to CqlPreprocessor
  • Move loading of the model from Cql2ElmProcessor to CqlPreprocessor
  • Note that errors will now be compiled in CqlPrerprocessor as they are in Cql2ElmProcessor instead of letting Exceptions get thrown
  • Introduce a new ForwardInvocationValidator that will use a CallContext derived from the calling function's representation of the called function versus an eligible function derived from a list of functions found by searching from a method name. This includes comparing each side's list of parameter DataTypes as well as taking into account implicit conversions (ex FHIRHelpers)
  • Address the use case of multiple implicit conversion matches and choose the resolved function with the lowest score, addressing various edge cases.
  • Address the case of duplicate errors in the 1204 unit test by caching results calculated in the preprocessor. Ensure error results are not recomputed twice and halt execution gracefully the second time.
  • Add new unit tests for various scenarios including dealing with overloads with normal types, generic types, as well as implicit conversions and namespace collisions (ex: System.Quantity vs FHIR.Quantity)

Closes: #1191

…class to ensure equivalence between the calling and called functions instead of previously just matching by name. Handle various edge cases such as implicit conversions and choice types. Add pre compile (function header) functionality to CqlPreprocessorVisitor. Refactor Cql2ElmVisitor to share functionality with CqlPreprocessorVisitor. Add several new unit tests to validate the new functionality.
@lukedegruchy lukedegruchy marked this pull request as ready for review September 20, 2023 15:17
@JPercival
Copy link
Contributor

@lukedegruchy - I merged master into this branch and fixed a minor typo. However, that exposed an issue:

  1. The new test for a better type error message is failing because it reports the same error twice.
  2. It reports the same error twice because visitNamedTypeSpecifier is executed twice.
  3. VisitNamedTypeSpecifier is executed twice because it's in base ElmCommonVisitor and the parse tree is visited in its entirety by both the PreProcessor and the Cql2ElmVisitor

One potential fix for that is to update the PreProcessor to do all that type resolution itself, store that information on the library builder, and then override the Cql2ElmVisitor to no-op for visiting TypeSpecifiers.

@JPercival JPercival enabled auto-merge (squash) October 11, 2023 15:15
@JPercival JPercival merged commit 69e010f into cqframework:master Oct 11, 2023
1 check passed
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.

Overloaded forward declarations fail to resolve
3 participants