[15/15] Deduplicate handler boilerplate and close the remaining correctness gaps by mns-nordicals · Pull Request #151 · t-kalinowski/quickr

mns-nordicals · 2026-07-25T10:43:56Z

Stacked PR 15 of 15 — branch conformability/15-maintainability,
cut from PR 14 (#150), branch conformability/14-semantics-vignette.
GitHub can only diff a fork branch against main, so until PR 14 merges
this page also shows PRs 6–14's commits; the 18 commits new to this
PR are Share the BLAS/LAPACK output-resolution and info-guard boilerplate
… Take R's diag() identity form for any length-1 x.

Stack overview and suggested review order: #152.

Fixes #136 (part 2 below; part 1 has no tracking issue — it is a
behavior-neutral refactor).

Two parts on one branch, each self-contained and separately revertible:

Deduplication — idioms that existed as N verbatim copies are spelled
once. Behavior-neutral: generated Fortran byte-identical, full
conformability grid (QUICKR_FULL_GRID=1) green at the same assertion
count. Net −105 lines.
Correctness and missing semantics — five real bugs (a crash, two
silent memory-corruption paths, two silent wrong answers), two
diagnostics that were inverted or internal, and two pieces of R semantics
that were simply unimplemented (c(<matrix>), as.vector()). Each
carries a regression test, and the behavior changes carry NEWS entries.

The whole branch keeps generated Fortran byte-identical apart from part 2's
new behavior, so snapshot churn is confined to part 2. Full suite green
with QUICKR_FULL_GRID=1: 14304 passing, 0 failures, 0 skips — up from
PR 14's 14241 purely by the new regression tests.

Part 1 — deduplication

BLAS/LAPACK emitters (`R/r2f-matrix-blas.R`)

One output-resolution path. Every emitter hand-rolled the same dance:
try can_use_output(), fall back to a hoisted temp, then wrap the result
and set writes_to_dest. resolve_blas_output() +
finalize_blas_output() spell it once; eleven emitters use them.
gemm/gemv/dger had duplicated their full call strings in both
branches — a standing "edited one branch, forgot the other" hazard — and
each is now a single emit.
One LAPACK info guard pair. The > 0 / < 0
emit_quickr_error_if() pair after dgesv/dgetrf/dgetri/dpotrf/dpotri/
dgesdd becomes emit_lapack_info_guards(). The uniform "illegal
argument" message is generated; the routine-specific message stays at the
call site so it is still greppable. dgesdd keeps its negative-first order
via a flag.
lapack_solve() split. Its two disjoint lowerings behind
if (context != "qr.solve") become lapack_solve_gesv() and
lapack_solve_qr(), behind the shared preamble (casts, rank checks,
conformability guard, expected dims).
assert_vector_or_matrix_rhs() (was assert_rhs_rank()) drops two
never-used parameters; the duplicated gemm/gemv comment headers merge.

Matrix handlers (`R/r2f-matrix.R`, `R/r2f-matrix-infer.R`)

cbind/rbind merged. The two handlers were 115 near-identical lines
differing only in orientation. compile_bind() (registered for both,
reading the direction from the call) and bind_piece_expr() spell the
shared skeleton once.
diag() argument matching shared. The ~30-line named/positional
x/nrow/ncol extraction existed verbatim in the handler and in
infer_dest_diag() — exactly the handler/inference pair whose drift
causes wrong declarations. Both now call diag_call_args(), and the two
identical missing-nrow stops collapse to one.
infer_dest_chol2inv() becomes an alias of the byte-identical
infer_dest_chol(). crossprod_like()'s three transpose flags were
fully determined by one of them and are now derived from a single trans.
bind_output_mode() gains a comment explaining why it is deliberately
not on the shared mode lattice (it also accepts raw, and refuses
complex mixing).

Subscripts and superassignment (`R/r2f-subscript.R`, `R/r2f-assign.R`, `R/r2f-closures.R`)

Read/write subscript lowering shared. The [ handler and
compile_subset_designator() carried verbatim copies of the
missing-arg/double-coercion pass, the scalar-[1]-no-op check, and the
c_ptrdiff_t cast (spelled four times). lower_subscript_args(),
subscript_is_scalar_noop(), and cast_subscript_to_integer() now serve
both sides — the same no-drift move check_subscript_exprs() already made
for subscript validation.
Superassignment target validation shared. <<-, [<<-, and
compile_subscript_lhs()'s host branch triplicated the target checks
(formals shadow, output variable, host-scope resolution, modified-flag
writeback). resolve_superassign_target() spells it once, with one copy
of the error messages.
check_assignment_compatible() and check_reassignment_narrowing() move
from scope.R (environment plumbing) to r2f-operators-helpers.R, next
to the mode_lattice they consult. Variable("int", ...) becomes
"integer" at four sites — partial matching had been making it work by
accident. A dead warning() and some stale comments go; the [ handler's
truncated contract comment is completed.

Hoisting core and consumers (`R/r2f-aab-core.R`, `R/r2f-constructors.R`, `R/r2f-control-flow.R`, `R/r2f-reductions.R`, `R/r2f-rev.R`)

One materialization idiom. The declare-tmp/emit-assign/rewrap pattern
existed at six sites while materialize_via_hoist() already encapsulated
it, hidden in the constructors file. It moves to the hoisting
infrastructure (with parent_call_name()), gains a logical_as_int
passthrough, and now backs hoist_unless_name(), both subscript-base
hoists, rev(), and array()'s scalar-target branch. It asserts its
hoist rather than raising a labelled internal error for a NULL one
(r2f() opens a hoist per statement before dispatching, so no caller can
supply one), which is what retires its what argument.
Guards route through the guard machinery. array()'s hand-rolled
recycling guard becomes an emit_quickr_error_if() call; matrix()'s
reshape-with-pad spelling reuses reshape_vector_for_matrix(), so the
pad-recycling spelling exists in exactly one place.
Dispatch core. unwrap_parens() replaces three hand-rolled
paren-unwrap loops; handler_field()/handler_for_call() replace four
copies of the R2FHandler-vs-attribute accessor branching; the dead — and
subtly buggy — r2f_default_handler() is deleted along with a stale
commented-out 'object' branch.
for handler. The OpenMP prologue/epilogue duplicated verbatim across
its two iteration paths becomes compile_for_body(), which computes the
directives and the post-loop error check while the OpenMP scope is still
entered (the ordering the duplication was protecting).
array()'s 90 lines of nested helpers lift to file level
(parse_array_dims(), known_dims_product()); the any/all handler's four
hand-rolled array-constructor probes share
renders_as_array_constructor()/is_declared_len1(); the reduction
handlers compute their call name once.

Part 2 — correctness and missing semantics

Bugs (a crash and two memory-corruption paths)

any()/all() forwarded an inherited mask hoister. A masked
reduction nested inside a numeric reduction —
sum(x * as.double(any(x[m] > 1))) — passed two hoist_mask arguments to
the [ handler and died with R's "matched by multiple actual arguments".
Both handler families now share lower_masked_reduction_arg(), which
installs exactly one mask hoister per reduction context.
Reassignment checked rank but not extents.
check_assignment_compatible() compared ranks only, so
```
x <- numeric(2)
x <- numeric(3)   # silently kept length 2
```
and non-broadcast array right-hand sides fell through to the Fortran
compiler. It now applies the same per-axis conformability policy the
elementwise operators use: compile error on a static mismatch, runtime
guard on symbolic dims. This is the shape analogue of the existing
narrowing check. Scalar broadcast and deferred-shape (NA-dim) locals
stay exempt. NEWS entry.
BLAS destination reuse accepted symbolic dim mismatches.
can_use_output() only rejected literal mismatches, so with
x declared double(n, 3):
```
out <- matrix(0, m, m)
out <- crossprod(x)     # dsyrk wrote `out` with the wrong leading dim
```
corrupted out for m > 3 and wrote out of bounds for m < 3. A
destination is now reused in place only when rank and every extent are
proven equal (dest_dims_proven_equal()); anything unproven routes
through a temporary, and the reassignment shape check above guards or
refuses the copy. NEWS entry.
Reassignment between a scalar and an array went unchecked in both
directions. The shape check exempted any assignment where either side
was length 1 — meant only to let a rank-0 value into a double(1) target,
but it swallowed every scalar/array pairing with it:
```
x <- numeric(n); x <- 0    # broadcast 0 across every element; R: length 1
x <- 1; x <- numeric(3)    # kept only the first element; R: length 3
```
Now only the both-sides-length-1 case is exempt (a declared double(1) is
rank 1, a literal is rank 0); everything else reaches the rank and
per-axis checks. Deferred-shape locals still reallocate for an array
value. NEWS entry.
diag(x) with a length-1 variable returned a 1×1 matrix. R's identity
form is length(x) == 1 with no nrow/ncol; quickr gated on the rank,
so a literal diag(3L) and a constant-folded local were the 3×3 identity
while diag(n) with n declared integer(1) built a 1×1 matrix holding
n — wrong in both shape and values, and infer_dest_diag() agreed with
the wrong lowering. Both now use the same length-1 predicate.

R derives the size with as.integer(x), which is also why diag(3.7) is
the 3×3 identity. Size expressions gain as.integer() so quickr can spell
the same thing — INT() in Fortran (truncating toward zero, as R does),
an integer cast in the generated C bridge, and r2size() folds a literal
instead of rejecting a non-whole double. So diag(x) now works for a
double or logical length-1 x too, not just an integer one.

Two dim renderers spell a dimension by deparsing the R expression
(bind_dim_string() and blas_int()); both now route through one small
rewriter, so as.integer cannot leak into emitted Fortran as an R name.
They are otherwise left unmerged, for the reasons below. NEWS entries for
both the diag() fix and the size-expression addition.

Diagnostics and unimplemented semantics

check_type_call()'s mode check was inverted. It raised "only atomic
modes are supported" precisely when the mode was atomic (bare
type(x = double)), while a genuinely non-atomic mode sailed past to an
internal error. The mode name (call head or symbol) must now be atomic,
the message names the offending mode, and a dims-less atomic mode gets a
clean form error.
c(<matrix>) and as.vector() are now supported. Both are ordinary R
semantics that quickr simply refused. A shared flatten_to_vector()
spells the column-major drop-dims reshape() once and backs
as.double(), as.integer(), the new as.vector(), and c()'s matrix
arguments. This also fixed as.integer() of an integer-backed logical
matrix, which used to keep its dims via an early return. NEWS entry.

Behavior-neutral cleanups riding along

Dead property plumbing in classes.R deleted rather than fixed: the
set_once/allow_na parameters no property ever enabled,
new_setter()'s NULL-coerce precedence quirk, and
new_scalar_validator()'s ignored env.
LAPACK declaration spellings. lapack_solve_qr() folds a literal
min(m, n) through diag_length_expr();
symmetrize_upper_to_lower()'s loop-index temps match
zero_lower_triangle()'s scalar dims = NULL (emitted text unchanged).
lapack_svd()'s 1 + 0 work-query dims turned out to be load-bearing —
quickr's scalar spelling is list(1L), and the query must stay a
subscriptable length-1 array — so they are documented in place rather
than "cleaned".
%*% shape computation shared between the handler and
infer_dest_matmul() via matmul_shapes().
matrix() argument policy normalized in one matrix_call_args(),
used by both the handler and the elementwise scalar-fill fast path. That
surfaced one fix: matrix(dimnames = ) was silently dropped and is now
refused, as array() already did.

Part 1b — the comparison and logical handlers

Six handlers that differed by one symbol

The six comparison handlers were byte-identical apart from the Fortran
operator, and &/| shared one registration whose body then switched on
the call name to choose .and./.or.. Both collapse onto one lowering
helper each — lower_comparison_operands() and lower_logical_operands()
— leaving every handler as its Fortran spelling plus its result mode:

r2f_handlers[["<"]] <- function(args, scope, ..., hoist = NULL) {
  .[left, right] <- lower_comparison_operands(args, scope, "<", ..., hoist = hoist)
  value <- infer_result_variable(left@value, right@value)
  value@mode <- "logical"
  Fortran(glue("({left} < {right})"), value)
}

& and | become separate handlers, so nothing dispatches on the call
name any more, and check_ordered_operands() — added in PR 12 with the
complex-ordering refusal, where it was the one thing the six handlers
could share — is absorbed into lower_comparison_operands(). The
short-circuit trio is renamed to say what it does rather than which
operators reach it: check_short_circuit_operand(),
is_eager_safe_condition(), and lower_short_circuit_operator(), which
&& and || both delegate to.

Behavior is unchanged: generated Fortran is byte-identical and the full
grid passes.

Names

One rename batch, no behavior change. The old names either described a
mechanism instead of a purpose, or were abbreviated past the point of being
guessable:

Before	After
`conform()`	`infer_result_variable()`
`maybe_reshape_vector_matrix()`	`conform_elementwise_operands()`
`guard_dim_f()`	`dimension_guard_expr()`
`dest_dims_proven()`	`dest_dims_proven_equal()`
`known_dims_prod()`	`known_dims_product()`
`array_dim_to_dims()`	`parse_array_dims()`
`renders_as_array_ctor()`	`renders_as_array_constructor()`
`subscript_as_index_int()`	`cast_subscript_to_integer()`
`blas_output_fortran()`	`finalize_blas_output()`
`assert_rhs_rank()`	`assert_vector_or_matrix_rhs()`
`reduce_arg_with_mask()`	`lower_masked_reduction_arg()`

conform() was the worst of these: it returns a Variable describing the
result of combining operands, which reads as a verb-that-mutates at every
call site. (The short-circuit helpers are renamed too, in the commit that
deduplicates them — see Part 1b.)

Verification

Generated Fortran is byte-identical for parts 1 and 3. Four _snaps/
files change only in the echoed R source (air format); a fifth changes
because its test body had to stop reassigning a scalar into an array.
Full suite with QUICKR_FULL_GRID=1: 14252 passing, 0 failures, 0 skips.
The +45 over the previous branch tip is part 2's new regression tests
(test-assignment-shape.R, test-flatten-vector.R, plus additions to
test-hoist-mask.R, test-errors.R, test-matrix.R,
test-matrix-mul.R, test-classes.R).
Each of part 2's bug fixes was reproduced against the pre-fix tree before
being fixed, and each has a test that fails without it.

Notes for review

Part 2's shape-related changes are the ones to scrutinize: the per-axis
reassignment check, the scalar/array refusal, and the proven-dims
requirement for BLAS destination reuse all turn code that used to compile
into a compile error or a runtime guard. They replace silent memory
corruption or silent wrong answers, but each can in principle reject a
program that happened to be correct at run time; NA-dim locals are the
one carve-out.
The scalar/array refusal is the widest-reaching: x <- numeric(n) followed
by x <- 0 is a plausible thing to have written, and it now fails to
compile. It has to, though — quickr was broadcasting where R rebinds, so
the old behavior was a wrong answer, not a convenience. Four tests in the
suite were themselves relying on it and are updated here.
as.integer() in size expressions is new public surface, small but real:
it is now a documented spelling users can write in declare(), not only
something diag() generates internally. It lowers to INT() and to a C
cast, both truncating toward zero like R.
blas_int() and bind_dim_string() both contain the
deparse-and-strip-L spelling with different branch orderings; their
input domains differ enough that unifying them looked like a drift risk
for no reader benefit. Left alone.
prod() still returns the operand join mode where R always returns
double. Pre-existing and orthogonal; not touched here.
codecov shows a drop; it is a measurement artifact. Project coverage goes
93.14% → 93.02%, and R/r2f-matrix.R alone accounts for more than the whole
regression (misses 25 → 78, +53, against a project net of +47 — every other
file combined improves by 6). The 53 lines are the body of compile_bind(),
which test-bind.R does exercise. covr instruments named namespace
bindings; on main the cbind/rbind handlers were anonymous functions passed
into register_r2f_handler(), so covr never instrumented them and they were
absent from the report entirely (0 lines reported, neither hit nor miss).
Naming the function makes covr instrument it, but the registry captures the
function by value (R/r2f-aaa-registry.R:15), so dispatch goes through the
pre-instrumentation copy and bypasses the counters. A blind spot became a
false negative.

The file already documents this hazard at R/r2f-aaa-registry.R:23-26 and
solves it for dest_infer (a dest_infer_name resolved by get0() at call
time in dest_infer_for_call()). Mirroring that for handler@fun — a
fun_name property plus a handler_callable() at the three dispatch sites in
R/r2f-aab-core.R — would fix it and also close the pre-existing blind spot,
so the number should end up higher than before this series. Deliberately not
done here: it changes the dispatch core to fix a metric rather than a defect.
Explained to the maintainer in a PR comment rather than fixed, at Martin's
direction (2026-07-25).

codecov · 2026-07-25T11:02:39Z

Codecov Report

❌ Patch coverage is 94.86504% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.86%. Comparing base (33a58ad) to head (e519be6).

Files with missing lines	Patch %	Lines
R/c-wrapper.R	79.77%	18 Missing ⚠️
R/r2f-constructors.R	81.81%	16 Missing ⚠️
R/r2f-operators-helpers.R	95.46%	14 Missing ⚠️
R/manifest.R	73.52%	9 Missing ⚠️
R/r2f-logical.R	96.36%	4 Missing ⚠️
R/classes.R	78.57%	3 Missing ⚠️
R/r2f-matrix-blas.R	99.41%	3 Missing ⚠️
R/r2f-matrix.R	96.00%	3 Missing ⚠️
R/r2f-aab-core.R	95.12%	2 Missing ⚠️
R/r2f-iterables-helpers.R	86.66%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   93.37%   93.86%   +0.49%     
==========================================
  Files          30       34       +4     
  Lines        6370     6997     +627     
==========================================
+ Hits         5948     6568     +620     
- Misses        422      429       +7

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

mns-nordicals · 2026-07-25T12:16:01Z

AI reason for codecov failure

On the codecov failures: the drop is a measurement artifact, not lost test coverage

Both codecov statuses are red on this PR, driven almost entirely by one file. Worth
explaining, because the number is misleading in a specific and fixable way.

The numbers. Project coverage goes 93.14% → 93.02%, misses 413 → 460 (+47).
R/r2f-matrix.R alone goes 89.08% → 69.53%, misses 25 → 78 (+53). So that one
file accounts for more than the entire project regression — every other file
combined improved by 6.

What those 53 lines are. They are the body of compile_bind()
(R/r2f-matrix.R:351), the merged cbind()/rbind() handler this PR introduces.
That code is exercised — test-bind.R covers both directions, all ranks, scalar
recycling and the error paths, and it passes.

Why covr can't see it. covr instruments a package by rebinding named
functions in the namespace. Compare the two sides:

	`cbind`/`rbind` body lines in covr's report
`main` (anonymous inline handlers)	0 — absent entirely, neither hit nor miss
this PR (named `compile_bind`)	54 — present, all recorded as missed

On main the handlers were anonymous functions passed straight into
register_r2f_handler(). covr never instrumented them, so they never appeared in
the report at all — they were invisible to the metric rather than covered by it.
This PR gives the function a name, so covr does instrument it; but
register_r2f_handler() captures the function by value
(handler <- R2FHandler(fun), R/r2f-aaa-registry.R:15), so the registry holds
the pre-instrumentation copy. Every dispatch goes through that copy and bypasses
the counters.

In other words: naming the function is what "lost" the coverage. The effect is to
replace a blind spot with a false negative.

This is already a known hazard in the file. R/r2f-aaa-registry.R:23-26 says:

covr rewrites function bindings in the namespace; resolving by name at call time
ensures instrumented/rebound functions are respected.

…and solves it for dest_infer, which stores a dest_infer_name and is resolved
via get0(name, envir = environment(handler)) at call time in
dest_infer_for_call() (R/r2f-aab-core.R:447), with the captured object as
fallback. handler@fun never needed the same treatment while every handler was
anonymous.

The fix, if you want it — mirror that pattern for the handler itself: a
fun_name property on R2FHandler set from substitute(fun) when it is a symbol,
and a small handler_callable() used at the three dispatch sites in
R/r2f-aab-core.R (239 / 248 / 259). About 15 lines, same convention, and it
would also close the pre-existing blind spot on main — so the reported number
should end up higher than before this series, not merely recovered.

I have deliberately not included that here. It is a change to the dispatch core
to fix a metric rather than a defect, it adds one get0 per handler call
(translation-time only, never in compiled-code hot loops, so the cost is
negligible — but it is still the dispatch path), and it seemed like your call
rather than mine. Happy to add it to this PR, split it into a separate one, or
leave it alone and let the artifact stand documented here.

Note also that the repo has no codecov.yml, so both statuses are running on
codecov's defaults, where any decrease fails.

check_recyclable_pair() blessed known unequal-but-divisible lengths (longer %% shorter == 0) for recycling that was never implemented, and its unknown verdict for differing symbolic lengths was ignored: with a = double(n), b = double(m), `a + b` lowered to `out = (a + b)` sized by `a`, silently truncating to min(n, m) where R recycles. Comparison operators, & and |, %%, and %/% consulted no length check at all. The replacement, check_elementwise_lengths(), returns a three-valued verdict applied uniformly by maybe_reshape_vector_matrix(), which every binary elementwise handler now routes through: - known lengths must be equal and nonzero, else compile error ("elementwise vector operations require equal lengths or a scalar operand; R-style recycling is not supported"); quickr cannot represent length-0 results, and implementing recycling would need per-element modulo indexing for little value - lengths not comparable statically (symbolic vs symbolic or constant, NA dims -- two unknown lengths are never the same quantity) emit one statement-level runtime size() guard via emit_quickr_error_if() - provably equal lengths and scalar broadcast stay guard-free Matrix-matrix operands get the same verdict per axis (previously the unknown case proceeded unchecked). Vector-matrix operands with unverifiable dims, previously a compile-time rejection, now compile with a size(vec) /= size(mat, 1) guard: strictly more programs compile, all safely.

Fill constructors (logical(k), integer(k), double(k), numeric(k)) lower to a single scalar literal whose Variable claims length k. c() spliced that literal as one element while sizing the result from the claimed dims, so c(numeric(2), x) emitted [ 0, x ] with a declared length of 2 + length(x) -- today a build failure only because the bare integer 0 next to a double x also made a mixed-type constructor. The c() handler now renders fill elements as implied-do spreads, [(0.0_c_double, i=1, int(2)), x], the same pattern array() already used for its fill branch (the detection is extracted into a shared is_fill_constructor_call()). The fill handlers also emit mode-correct literals (0.0_c_double, 0_c_int) so spliced or promoted fills carry the type their Variable claims. matrix(scalar, m, n) rebadged the scalar's Variable with rank-2 dims and returned the scalar text, which is only valid where Fortran's scalar broadcast applies: sum(matrix(2, 2, 3)) emitted the invalid sum(2.0_c_double). The scalar is now materialized into a hoisted rank-2 temporary in expression contexts; direct assignment keeps the free broadcast (m <- matrix(0, n, k) still emits m = 0.0_c_double). Snapshot churn is the typed fill literals (out = 0 becoming out = 0.0_c_double / 0_c_int).

Found by the conformability grid: a 1x1 matrix operand that needed a cast or booleanization was scalarized by appending (1, 1) to the cast expression text -- real(b, kind=c_double)(1, 1) -- which gfortran rejects as unclassifiable. Hoist non-name 1x1 operands to a temporary before subscripting. R only recycles length-1 arrays in *arithmetic* (deprecated but live); comparisons and & | error with "dims [product 1] do not match the length of object". quickr's uniform scalarization answered where R refuses. Comparisons and & | now pass scalarize_one_by_one = FALSE so the 1x1 falls through to the vector-matrix rule: known longer vectors are a compile error, unknown lengths get the runtime guard (length 1 still conforms, as in R).

A zero-fill constructor (numeric(k), integer(k), ...) lowers to one scalar literal carrying array dims. Whole-array assignment broadcasts that correctly and c()/array()/matrix() spread or pad it explicitly, but any other consumer saw a scalar where the dims claimed an array: c(numeric(2) + 1, x) emitted three elements where the length arithmetic counted four (build failure), and sum(numeric(2) + 3) returned 3 instead of 6 (silent wrong answer). Fill handlers now materialize into a hoisted temporary in every other context.

fill_constructor_value() and the matrix() scalar case duplicated both the parent-call sniff (positional indexing into the calls stack) and the declare_tmp/emit/Fortran materialize triplet. One copy of each now. Review finding (fable-final-review.md t-kalinowski#5); no behavior change.

…zing R's length-1-array recycling drops the array dims only when the vector's length is not 1; for a length-1 vector the 1x1 dims are kept. Scalarizing whenever the length was not *provably* 1 answered the compile-time question "is the length 1?" with "no" when the truth was "unknown", so a symbolic-length vector that turned out to have length 1 at run time returned a dimensionless vector where R returns a 1x1 matrix -- a silent shape divergence. Scalarize only when the length is statically known and not 1 (the cases where R itself drops the dims, including length 0). Symbolic lengths fall through to the vector-matrix rule: a runtime guard requires length 1, the result is a 1x1 matrix, and longer vectors raise an error where R would recycle (a deprecated behavior in R). Found by codex review (fable-final round); reproduces on upstream main.

Two paths added by this PR had no test reaching them. check_elementwise_lengths() rejects a known length-0 operand separately from the both-lengths-known case, for when the other length is not a number it can be compared to. The existing zero-length test declares double(0) against double(4), which takes the both-known branch, so the separate verdict was never exercised. Add the symbolic and NA-dim counterparts (numeric(0) + x with x of length n, and double(NA) - double(0)), where R answers numeric(0) and quickr has no length-0 result to return. The fill/matrix materialization decision reads the enclosing call to decide whether the scalar-with-dims form is understood, and falls back to "no enclosing call" when the stack is empty. A function body must be braced, so that fallback is unreachable at top level -- but a local closure's return expression is compiled on its own, with no calls stack, so a closure returning numeric(k) or matrix(scalar, m, n) lands there and has to materialize. Both compile and match R. Reported by codecov (PR 142 patch coverage).

emit_elementwise_size_guard() and materialize_via_hoist() each opened with a NULL-hoist branch raising a compile error. Neither is reachable: r2f() opens a hoist per statement before dispatching to a handler, and every operator and constructor handler forwards the one it received, so no R program can put a NULL there. The branches only showed up as uncovered patch lines. Drop both. emit_quickr_error_if() already asserts the hoist is an environment, so the guard needed nothing in its place; materialize_via_hoist() gets a stopifnot() and loses its `what` argument, which existed only to name the construct in the removed message. maybe_reshape_vector_matrix() drops its hoist/scope defaults for the same reason -- all callers pass both, and the defaults implied a caller that cannot exist. Reported by codecov (PR 142 patch coverage).

Linear-algebra lowerings had three reactions to dims they could not verify at compile time, none of which stopped the bad call: %*%, the gemv/gemm paths, triangular solve, solve() right-hand sides, and the square-matrix checks warned at compile time and proceeded (dgemv with a short x silently read out of bounds; other shapes died with a cryptic "BLAS/LAPACK routine 'DGEMM ' gave error code -10"); crossprod and tcrossprod hard-errored at compile time, rejecting programs that are fine at run time; and two operands both declared with NA dims skipped every check, because identical(NA, NA) is TRUE. One policy now, the same one the elementwise operators follow: a statically known mismatch is a compile error; anything unverifiable gets a statement-level runtime guard -- one scalar size() comparison emitted immediately before the BLAS/LAPACK call, raising the error R raises ("non-conformable arguments in %*%"). The compile-time warning is retired (it fired once per call site for a condition only the run time can decide); crossprod relaxes to the guard, so strictly more programs compile, all safely; NA dims are always treated as unverified, never equal. guard_conformable_dims() + guard_dim_f() in r2f-matrix-blas.R replace the warn/assert patchwork (assert_conformable_dims and warn_conformability_unknown are deleted; assert_square_matrix becomes a wrapper that also takes the operand and hoist/scope). A literal dim renders as the literal; anything else as size(operand[, axis]), an inquiry that does not evaluate operand expressions. check_conformable survives only where the verdict steers codegen rather than safety: lapack_solve's square-vs-rectangular dispatch, and bind_common_dim, whose unknown-dims compile error is deliberate (the common dim is needed to declare the cbind/rbind output) and now documented. Message change for statically known non-square triangular solves: "triangular solve requires a square matrix" (was "non-conformable arguments in triangular solve"), matching the other square checks.

The fallthrough %*% guard (reached by vector-vector products after the gemv special cases) hardcoded rank-2 axes, emitting size(x, 2) on a rank-1 array -- a gfortran error that made conformable unknown-length dot products fail to compile. Compare whole vector sizes for rank-1 operands instead. Also document why lapack_solve()'s squareness check is routing, not a guard: rectangular solve(a, b) deliberately falls through to least squares, a tested divergence from base R.

Transcribe the mode and shape contract tables into an expected-outcome function and check every (mode pair, shape pair, op) cell against plain R: valid cells must match values, typeof(), and shape; statically invalid cells must fail with the documented compile message; symbolic cells must guard at runtime. Cells sharing a shape pair pack into one compiled function per op family, so the default deterministic sample costs ~20 gfortran runs; QUICKR_FULL_GRID=1 compiles every shape pair.

The 02-2 fix routes arithmetic on a 1x1 matrix and a symbolic-length vector through the vector-matrix rule (guard on length 1, 1x1 result) instead of scalarizing at compile time, closing the shape divergence the review found: at runtime length 1, R keeps the 1x1 dims. Encode that in the verdict function, and give sym operands facing a 1x1 partner the conforming length 1 so the ok-path is exercised at the shape the guard admits. The old sym_len = 3 cells never ran the length-1 branch.

solve(a, b) with a rectangular a fell through to a least-squares dgels call, returning qr.solve()'s answer where R raises "'a' (m x n) must be square". Statically rectangular systems are now a compile error and symbolic squareness is guarded at run time before the dgesv call, via the assert_square_matrix() helper the other LAPACK lowerings already use. The now-unreachable rectangular tail of lapack_solve() (dgels, and a dgelsy branch qr.solve() never reached) is deleted; qr.solve() keeps its least-squares behavior. The solve output follows ncol(a) while b follows nrow(a); when ncol is statically 1 the output declares as a Fortran scalar, so a symbolic-length b is copied elementwise instead of by whole-array assignment.

Three duplications across the elementwise operators, ifelse() and the BLAS/LAPACK lowerings collapse into one helper each, all in r2f-operators-helpers.R: - guard_conformable_dims() becomes the single guard emitter for the conformability policy -- a statically known mismatch is a compile error, dims that cannot be compared statically get a statement-level runtime guard, provably equal dims need nothing. It moves out of r2f-matrix-blas.R (with guard_dim_f) and absorbs both private copies: emit_elementwise_size_guard() and ifelse()'s ifelse_axis_verdict() plus its inline .or. guard. - check_conformable() was dims_match() written in list form; both call sites (bind_common_dim, solve routing) now say so, and the weaker helper's contract is spelled out next to it. - real_floor_expr() carries the real-domain floor spelling shared by floor() and double %/%, so the aint/merge trick lives in one place. Behavior-neutral: zero snapshot churn and a full QUICKR_FULL_GRID=1 pass at 14207 assertions.

The dgesv branch ended in return(), leaving the qr.solve condition that followed always-true (and the function textually able to fall off the end). The branches are a plain if/else now, converging on one tail. The identical output-target selection is one shared spelling (dest_usable), still evaluated at each branch's original write point so declaration order and error order in the emitted block are unchanged. Review finding (fable-final-review.md #2); no behavior change.

`register_r2f_handler()` stores the handler as a function object captured at build time. covr rebinds its instrumented copies into the namespace after the package has loaded, so a handler registered as a top-level named function keeps dispatching the copy taken at registration: the instrumented copy never runs and the handler reads as 0% covered however well it is tested. Its callees still read as covered, because their names resolve from the namespace at call time -- which is what makes the pattern recognisable. The file already documents this hazard and works around it for `dest_infer`, by recording `dest_infer_name` and resolving it at call time. Do the same for the handler itself: record `fun_name` at registration and let `get_r2f_handler()` swap in the current namespace binding. The object stays authoritative. Of the registrations in the tree, 27 pass an anonymous function literal, which has no name to resolve; the name is a supplement for the ones that don't. Recording it is deliberately stricter than `dest_infer_name`: the argument must be a symbol *and* name this same function in a namespace, since that is the only environment covr rebinds into. That excludes the local `handler` closure built by `register_unary_intrinsic()`, whose name means something else on the next call, and `r2f_handlers[["<-"]]`, which is not a symbol at all. `dest_infer` is left as it is; it is advisory, whereas the handler is called, so a wrong resolution there would be a bug. No handler in the tree is currently registered by name, so nothing changes today. It is what lets a handler be extracted into a named function without the extraction reading as untested.

Every BLAS/LAPACK emitter hand-rolled the same dance: try can_use_output(), fall back to a hoisted temp, and finish by wrapping the result and setting writes_to_dest. gemm/gemv/dger even duplicated their full call strings in both branches. resolve_blas_output() + blas_output_fortran() now spell it once; emit_lapack_info_guards() replaces the six copies of the info >0/<0 guard pair (dgesdd keeps its negative-first order). lapack_solve()'s two disjoint lowerings split into lapack_solve_gesv()/lapack_solve_qr() behind the shared preamble. assert_rhs_rank() drops its never-used call_scalar/ call_high parameters. Emitted Fortran is unchanged.

cbind() and rbind() were 115 near-identical lines differing only in orientation; compile_bind() (registered for both, reading the direction from the call like compile_binop) and bind_piece_expr() spell the shared skeleton once. diag()'s ~30-line named/positional x/nrow/ncol extraction existed verbatim in the handler and in infer_dest_diag() -- exactly the pair that drifts -- and now both call diag_call_args(); the two identical missing-nrow stops collapse to one. infer_dest_chol2inv() is an alias of the byte-identical infer_dest_chol(). crossprod_like()'s trans_single/ opA/opB were fully determined by one flag, now derived from `trans`. bind_output_mode() gains a comment saying why it is not on the shared mode lattice (raw support, complex-mixing refusal). Emitted Fortran is unchanged.

The `[` handler and compile_subset_designator() carried three verbatim copies of the same logic: the missing-arg/double-coercion pass, the scalar-[1]-is-a-no-op check, and the inline c_ptrdiff_t cast (spelled four times). lower_subscript_args(), subscript_is_scalar_noop(), and subscript_as_index_int() in r2f-subscript.R now serve both sides, so read and write subscripts cannot drift. `<<-`, `[<<-`, and compile_subscript_lhs()'s host branch triplicated the superassignment target validation (formals shadow, output-variable, host resolution, modified-flag writeback); resolve_superassign_target() spells it once. Also: check_assignment_compatible()/check_reassignment_narrowing() move from scope.R (environment plumbing) to r2f-operators-helpers.R next to the mode lattice they consult; the `[` handler's truncated contract comment is completed and its commented-out placeholder arms deleted; Variable("int", ...) spelled "integer" (charmatch made it work by accident); a dead warning() directly before a stop() in scope.R is dropped; a stray design musing in r2f-assign.R is removed. Emitted Fortran is unchanged.

chatgpt-codex-connector · 2026-08-15T15:24:20Z