Skip to content

ChoiceTypeSpecifier built in source order breaks signature matching after #1708 #1711

Merged
JPercival merged 4 commits intomainfrom
fix/choice-type-specifier-order-independent-matching-
Mar 25, 2026
Merged

ChoiceTypeSpecifier built in source order breaks signature matching after #1708 #1711
JPercival merged 4 commits intomainfrom
fix/choice-type-specifier-order-independent-matching-

Conversation

@c-schuler
Copy link
Copy Markdown
Contributor

@c-schuler c-schuler commented Mar 25, 2026 •
edited
Loading

Summary

  • Fix: ChoiceType serialization is now deterministic #1708 sorted ChoiceType.types alphabetically for deterministic serialization, but visitChoiceTypeSpecifier still built the ChoiceTypeSpecifier.choice list in CQL source order
  • At runtime, the engine's typeSpecifiersEqual does positional comparison of ChoiceTypeSpecifier.choice lists, so the ordering mismatch between FunctionRef signature and FunctionDef operand caused function resolution to fail
  • Manifested as Could not resolve call to operator 'Normalize Interval(java.lang.Object)' in clinical-reasoning's EXM108 measure tests
  • Fixed by building the specifier list from the sorted ChoiceType.types, and removed a duplicate comparison block in SimpleElmEngine.typeSpecifiersEqual

Test plan

  • Added choiceTypeSpecifierSignatureMatchesSameOrder in FunctionRefEvaluatorTest
  • Full engine, elm, and cql-to-elm test suites pass
  • Verified against clinical-reasoning SimpleMeasureProcessorTest (14/14 pass)

@c-schuler c-schuler requested a review from JPercival March 25, 2026 17:44
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026 •
edited
Loading

Related Issues

The following open issues may be related to this PR:

Issue Title Score Matched Terms
#891 Could not resolve function call 20 "call operator", "engine elm", "resolve call", runtime, operator, resolve, call, against, list, java (path), opencds (path), engine (path), function (path), common (path), org (path), cql (path), cqf (path), elm (path), test (path)
#448 Equivalent between FHIR.AdministrativeGender System.Code 19 "call operator", "elm cql", "resolve call", between, operator, resolve, signature, call, types, comparison, list, common (path), org (path), cql (path), elm (path)
#1505 'Collapse' on at least 2 closed datetime intervals without a high value always fails 18 "interval java", "engine elm", between, runtime, interval, comparison, clinical, list, cqframework (path), java (path), src (path), evaluator (path), opencds (path), engine (path), executing (path), main (path), org (path), cql (path), cqf (path), elm (path)
#1133 ToList is not implemented according to the spec 17.5 "engine elm", "cql elm", operator, lists, operand, types, clinical, list, cqframework (path), java (path), src (path), opencds (path), engine (path), main (path), org (path), cql (path), cqf (path), elm (path), test (path)
#1435 Ambiguous call to operator error with QiCore function overload 15.5 "call operator", "cql elm", runtime, operator, resolve, signature, operand, call, clinical, cqframework (path), function (path), org (path), cql (path), elm (path)

Tip: If this PR addresses any of these issues, please link them using Closes #NNN or Refs #NNN in the PR description.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026 •
edited
Loading

Formatting check succeeded!

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.00%. Comparing base (20b6cd7) to head (e681255).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...lm/preprocessor/CqlPreprocessorElmCommonVisitor.kt 80.00% 0 Missing and 1 partial ⚠️
.../cqframework/cql/elm/evaluating/SimpleElmEngine.kt 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1711      +/-   ##
============================================
+ Coverage     61.98%   62.00%   +0.01%     
  Complexity     3944     3944              
============================================
  Files           210      210              
  Lines         20383    20374       -9     
  Branches       3884     3882       -2     
============================================
- Hits          12634    12632       -2     
+ Misses         6139     6131       -8     
- Partials       1610     1611       +1

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@c-schuler c-schuler changed the title ChoiceTypeSpecifier matching should be order-independent ChoiceTypeSpecifier built in source order breaks signature matching after #1708 Mar 25, 2026
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 25, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@JPercival JPercival merged commit d2b8a7e into main Mar 25, 2026
8 of 10 checks passed
@JPercival JPercival deleted the fix/choice-type-specifier-order-independent-matching- branch March 25, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JPercival JPercival JPercival approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@c-schuler @JPercival