Skip to content

Bug: Coalesce type inference returns Any instead of Integer in aggregate#1710

Open
JPercival wants to merge 1 commit into
mainfrom
fix/coalesce-aggregate-type-inference
Open

Bug: Coalesce type inference returns Any instead of Integer in aggregate#1710
JPercival wants to merge 1 commit into
mainfrom
fix/coalesce-aggregate-type-inference

Conversation

@JPercival
Copy link
Copy Markdown
Contributor

Summary

  • Coalesce(R, 1) inside an aggregate with null starting value infers result type Any instead of Integer
  • This causes the downstream Multiply to insert an unnecessary As(Integer) cast around the Coalesce result
  • The correct behavior: Coalesce should compute its result type as the common type of concrete (non-Any/non-null) arguments

Reproduction

define Sequence: { 1, 2, 3 }
define Factorial:
  Sequence X
    aggregate R: Coalesce(R, 1) * X

The aggregate accumulator R starts as null (type Any). Coalesce(R, 1) resolves through generic operator overload matching, which unifies T to Any because the first arg has type Any. This gives the Coalesce a result type of Any, forcing the Multiply operator to insert an As(Integer) cast on its first operand.

Expected ELM: Multiply(Coalesce(QueryLetRef("R"), Literal(1)), AliasRef("X"))
Actual ELM: Multiply(As(Integer, Coalesce(QueryLetRef("R"), Literal(1))), AliasRef("X"))

Test

The included test (AggregateCoalesceTypeBugTest) demonstrates the bug by asserting that the first Multiply operand is wrapped in As — this assertion passes, confirming the unnecessary cast exists.

Test plan

  • AggregateCoalesceTypeBugTest passes (demonstrates the bug)
  • When fixed, the test assertion should be updated to assertIs<Coalesce>(firstOperand)

🤖 Generated with Claude Code

Adds a failing-by-assertion test demonstrating that the translator
infers `Coalesce(R, 1)` result type as `Any` instead of `Integer`
when `R` is an aggregate accumulator starting from null.

The downstream `Multiply` operator wraps the Coalesce result in an
unnecessary `As(Integer)` cast. The correct behavior: Coalesce should
compute its result type as the common type of concrete (non-Any)
arguments, giving `Integer` directly without a cast.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Related Issues

The following open issues may be related to this PR:

Issue Title Score Matched Terms
#1137 Inconsistent Casting in Unions 16 "cql define", literal, integer, cast, returns, define, operand, through, behavior, null, elm, value, type (path), bug (path), org (path), cql (path)
#1133 ToList is not implemented according to the spec 15.5 returns, com, define, code, operand, behavior, because, null, https, elm, value, operator (path), type (path), cqframework (path), java (path), src (path), bug (path), org (path), cql (path), test (path)
#734 Width allows Interval as a parameter 14 result, integer, returns, define, through, because, null, https, elm, generic, value, operator (path), type (path), operators (path), org (path), cql (path), tests (path)
#1400 patientBirthDatePropertyName marked as deprecated but still required for SystemFunctionResolver 13 non, instead, com, define, code, behavior, because, null, https, elm, cqframework (path), java (path), src (path), cql2elm (path), org (path), cql (path)
#840 ExpandValueSet is called aggressively 12.5 "cql define", instead, correct, overload, define, code, operand, because, elm, value, cql (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

Formatting check succeeded!

@sonarqubecloud
Copy link
Copy Markdown

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.98%. Comparing base (e64b32f) to head (42adbb3).
⚠️ Report is 23 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1710      +/-   ##
============================================
- Coverage     65.68%   61.98%   -3.71%     
- Complexity     1645     3944    +2299     
============================================
  Files           481      210     -271     
  Lines         27890    20383    -7507     
  Branches       5553     3884    -1669     
============================================
- Hits          18320    12634    -5686     
+ Misses         7231     6139    -1092     
+ Partials       2339     1610     -729     

☔ 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.

JPercival added a commit that referenced this pull request Mar 28, 2026
The Aggregate parity test skip is not a regression — our type inference
is more correct. Document the pattern (Any-typed aggregate accumulators
losing precision in Coalesce resolution) so future agents can recognize
similar cases where the legacy inserts unnecessary casts that our
pipeline correctly omits. References PR #1710.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JPercival added a commit that referenced this pull request Mar 28, 2026
…ypeResolver and ElmEmitter

Resolve model types (QUICK Element, Extension, etc.) through loaded models
instead of only the system type namespace. This completes parity for the
TupleAndClassConversions test, bringing results to 31/32 (1 intentional
skip: Aggregate legacy bug #1710).

TypeResolver:
- Add resolveNamedType() and resolveModelType() for system+model lookup
- Update resolveTypeSpecifier to handle qualified and unqualified model types
- Update inferInstanceLiteralType to delegate to resolveTypeSpecifier

EmissionContext:
- Add resolveTypeQName() for name-to-QName with correct model namespace
- Add dataTypeToQName() and dataTypeToTypeSpecifier() that bypass
  TypeBuilder's system-only ModelResolver for model types
- Replace all operatorRegistry.typeBuilder.dataTypeToQName/TypeSpecifier
  calls across codegen package (EmissionContext, CollectionOperatorEmission,
  IntervalOperatorEmission)

TypeOperatorEmission:
- Update emitTypeSpecifier, emitImplicitCastExpression to use resolveTypeQName
- Fix emitConversionExpression for class/tuple conversions: emit As with
  asType (QName) for named types, asTypeSpecifier for complex types,
  matching legacy buildAs behavior

FullParityTest:
- Register QuickModelInfoProvider in test model manager
- Update results documentation
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.

1 participant