Skip to content

[4/15] Fix result modes in ifelse(), t(), and diag(); enforce ifelse() branch shapes - #140

Merged
t-kalinowski merged 5 commits into
t-kalinowski:mainfrom
mns-nordicals:conformability/04-ifelse-t-diag
Jul 31, 2026
Merged

[4/15] Fix result modes in ifelse(), t(), and diag(); enforce ifelse() branch shapes#140
t-kalinowski merged 5 commits into
t-kalinowski:mainfrom
mns-nordicals:conformability/04-ifelse-t-diag

Conversation

@mns-nordicals

Copy link
Copy Markdown
Contributor

Stacked PR 4 of 15 — branch conformability/04-ifelse-t-diag,
cut from PR 3 (#139), branch conformability/03-type-promotion.
GitHub can only diff a fork branch against main, so until PR 3 merges
this page also shows PRs 1–3's commits; the 3 commits new to this
PR are Promote ifelse() branches and shape the result like test
Enforce the ifelse() branch-shape contract.

Stack overview and suggested review order: #152.

Fixes #125.

What changed

Three handlers now match R's result types (and, for ifelse(), R's
result shape):

ifelse() promotes its branches and shapes the result like test.
Mixed-mode branches previously emitted an invalid mixed-type merge()
(Fortran requires same-typed branches), so this failed at the gfortran
stage:

fn <- function(c, a) {
  declare(type(c = logical(n)), type(a = double(n)))
  ifelse(c, 1L, a)
}
# before: merge(1_c_int, a, (c/=0)) declared integer -> gfortran error
# after:  merge(real(1_c_int, kind=c_double), a, (c/=0)) declared double

The result's shape now comes from test, per R's documented contract,
instead of from the first non-scalar of (test, yes, no). One case
becomes a compile-time error: a scalar test with array-valued
branches (R returns a length-1 result there, which merge cannot
express — previously this silently emitted a branch-shaped result):

Error: ifelse() result takes the shape of `test`; array-valued yes/no
with scalar test is not supported

ifelse() enforces branch shapes. Fortran's merge() requires
conformable arguments, so a runtime length mismatch between test and a
non-scalar branch read past the shorter branch and returned garbage
(c(10, 2, 4.65e-310) where R recycles). A non-scalar branch must now
match the shape of test: statically unequal dims (including rank
mismatches) are a compile error, and symbolic dims emit a statement-level
runtime size guard — the same three-valued policy the elementwise
operators adopt later in this series. NA dims always count as unknown
(two unknown lengths are not the same quantity). Scalar branches
broadcast natively, as before. R-style branch recycling is deliberately
not implemented — refusal keeps the "matches R or errors" invariant.

t() and diag() preserve the input mode. Both unconditionally
cast to double; R preserves the input's type:

fn <- function(m) {
  declare(type(m = integer(2, 3)))
  t(m)
}
typeof(quick(fn)(matrix(1:6, 2, 3)))
# before: "double"   after: "integer" (matches R)

This covers t(<matrix>), t(<vector>), diag(<matrix>) (diagonal
extraction), and the constructor forms diag(x) / diag(x, nrow, ncol)
(R preserves typeof(x): diag(1:3) is integer, diag(TRUE, 2) is
logical). The identity forms (diag(n), diag(nrow = n)) stay double,
matching R.

Not changed, deliberately: the transposes feeding matrix products
(%*%, crossprod, ...) keep their double casts — R's matrix products
always return double — with a comment added saying why.

How

  • R/r2f-conditionals.R: the ifelse handler promotes both branches
    through promote_operands() (the helper introduced for operators and
    c()), takes dims from the booleanized mask, and errors on the
    scalar-test/array-branch case. Branch shapes are checked per axis
    (ifelse_axis_verdict()); unknown axes are guarded through
    emit_quickr_error_if(), combined into one .or. condition per
    branch.
  • R/r2f-matrix.R, R/r2f-matrix-blas.R: t(), diag_extract(), and
    diag_matrix() drop their maybe_cast_double() and carry the input
    mode through the result Variable, hoisted temporaries, and
    diag_matrix()'s zero fill.
  • R/r2f-matrix-blas.R: can_use_output() gains a mode argument
    (default "double", all other callers unchanged) so an in-place
    destination is only used when its declared mode matches the result.
  • R/r2f-matrix-infer.R: infer_dest_diag() reports the input's mode
    instead of hard-coding double, so out <- diag(m) declares out
    with the right mode on the inferred-destination path too. When the
    input's mode is not yet known it reports no destination rather than
    guessing one.
  • diag_matrix()'s zero fill is spelled per mode (0_c_int, .false.,
    …) and refuses modes it cannot spell.

Tests

  • test-ifelse.R: mixed-mode branches compile and match R (including
    typeof()), logical branches join as logical, and the
    scalar-test/array-branch case raises the new error. A translation
    snapshot locks the promoted merge spelling. New shape tests: a
    statically mismatched branch is a compile error, unknown-length
    branches guard at runtime (conformable inputs still match R;
    mismatched inputs raise instead of returning garbage), with a
    snapshot pinning the guard text.
  • test-matrix.R: t() and diag() preserve integer and logical
    modes across the matrix, vector, rectangular-with-recycling, and
    inferred-destination (out <- diag(m)) paths; the identity form
    stays double. A snapshot locks the integer diag extraction writing
    directly into the integer output.

No existing snapshots changed; the only snapshot additions are the two
new tests above.

Notes for review

  • Breaking behavior: ifelse() with scalar test and non-scalar
    branches now errors at compile time instead of returning a
    branch-shaped result (a silent divergence from R before).
  • Breaking behavior: ifelse() with branch lengths that differ from
    test now errors (compile-time when static, runtime when symbolic)
    instead of recycling in R and reading out of bounds in quickr.
  • Result typeof() changes (double → integer/logical) for t() and
    diag() on non-double inputs — previously a silent divergence from R.
  • diag()'s constructor forms preserve typeof(x) in (at least
    reasonably modern) R — typeof(diag(1:3)) is "integer" — so those
    are fixed here too, not just diag(<matrix>).

@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.80220% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.26%. Comparing base (91c8dac) to head (9af5e14).

Files with missing lines Patch % Lines
R/r2f-matrix-blas.R 97.29% 1 Missing ⚠️
R/r2f-matrix-infer.R 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   93.17%   93.26%   +0.08%     
==========================================
  Files          28       29       +1     
  Lines        6083     6161      +78     
==========================================
+ Hits         5668     5746      +78     
  Misses        415      415              

☔ View full report in Codecov by Harness.
📢 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.

Fortran's merge() requires same-typed branches, but the ifelse()
handler took its result mode from `yes` alone and never cast `no`:
mixed-mode branches emitted an invalid mixed-type merge()
(ifelse(c, 1L, a) with double `a` failed at the gfortran stage). The
result shape also came from the first non-scalar of (test, yes, no),
where R documents ifelse() as returning a result shaped like `test`.

Promote both branches to their common lattice mode with
promote_operands(), and take the result's mode and shape from the
promoted branches and the (booleanized) test respectively. A scalar
test with array-valued branches is now a compile-time error --
merge() cannot represent R's length-1 result -- instead of silently
emitting branch-shaped code.
R's t() and diag() preserve their input's type, but the handlers
unconditionally cast to double: t(m) and diag(m) on an integer matrix
returned doubles, and the constructor forms diag(x) / diag(x, nrow,
ncol) lost typeof(x) too. Only the identity forms (diag(n),
diag(nrow = n)) are double in R, which is what quickr already emits
for them.

Drop the maybe_cast_double() calls and carry the input mode through
the result Variable, the hoisted temporaries, and the zero fill in
diag_matrix(). can_use_output() gains a `mode` argument (default
"double"; all other callers unchanged) so an in-place destination is
only used when its declared mode matches, and infer_dest_diag()
reports the input's mode instead of hard-coding double -- a
double-inferred dest would have mislabeled an integer result's
declaration.

The transpose casts in unwrap_transpose_arg() stay, now with a
comment saying why: that path only feeds matrix products, which
always return double in R.
ifelse() promoted branch modes and rejected scalar-test/array-branch
calls, but never validated branch shapes against `test`. Fortran's
merge() requires conformable arguments, so a runtime length mismatch
read past the shorter branch and returned garbage where R recycles.

A non-scalar branch must now match the shape of `test`: statically
unequal dims (including rank mismatches) are a compile error, and
symbolic dims emit a statement-level runtime size guard, matching the
elementwise-operator policy. NA dims always count as unknown -- two
unknown lengths are not the same quantity.
@t-kalinowski
t-kalinowski force-pushed the conformability/04-ifelse-t-diag branch from d9bbeee to e1b3e77 Compare July 28, 2026 22:48
@t-kalinowski

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1b3e77ce2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread R/r2f-matrix.R
Comment thread R/r2f-conditionals.R
Comment thread R/r2f-conditionals.R
Coverage flagged both stop() calls in check_ifelse_branch_shape() as
untested.

The rank check is reachable: a matrix branch under a vector `test`, or
the reverse, hits it, and the existing mismatch test only exercises the
per-axis verdict. Add a case for each direction.

The `is.null(hoist)` arm is not reachable. r2f() replaces a NULL hoist
with a fresh one before dispatching to any handler, and the only other
dispatch route resolves `f<-`-style names, so the ifelse() handler
always has a hoist to emit the size guard into. Delete it and record
the invariant in the function's comment rather than testing dead code.
@mns-nordicals

mns-nordicals commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Fixed code coverage and removed an unreachable branch.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab11aa00b0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread R/r2f-matrix-blas.R Outdated
Comment thread R/r2f-matrix-blas.R Outdated
@t-kalinowski
t-kalinowski merged commit fafce1c into t-kalinowski:main Jul 31, 2026
8 checks 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.

[4/15] ifelse() miscompiles mixed-mode branches and ignores test's shape; t()/diag() coerce integer to double

2 participants