[1/15] Fix repeated side effects in floor/ceiling and runif bounds, reject na.rm= in reductions, and three small crash/diagnostic fixes - #137
Conversation
floor() and ceiling() splice their argument into the emitted Fortran expression three times (once for aint(), twice for the sign adjustment), so an impure argument such as runif(1) was evaluated three times: the result mixed different random draws and the RNG state diverged from R. Extract the hoist-to-temporary pattern already used by matrix() into a shared helper, hoist_unless_name(), and use it in floor()/ceiling() to evaluate non-trivial arguments once. Bare variable names are passed through unchanged, so existing translations without side effects are unaffected. matrix() now uses the shared helper.
The reductions handler never inspected argument names, so `sum(x, na.rm = TRUE)` translated `na.rm = TRUE` as an extra data argument, emitting `(sum(x) + .true.)`. Reject named arguments with a clear compile-time error, matching the existing any()/all() guard.
Binding a new variable to an expression that produces no value (e.g. `y <- if (x) 1 else 2`, where `if` lowers to a statement) crashed with "no applicable method for `@` applied to an object of class NULL". Diagnose it at the assignment site instead, naming the offending expression.
`var@mode != "integer"` fails with "argument is of length zero" when @mode is NULL (a binding whose mode is still being inferred, as in the deferred-mode path in r2f-assign.R). Use !identical() so the intended "size is not an integer" warning path is reached instead.
The dims setter early-returned on empty input, so resetting a variable to scalar (NULL dims, per the class's own convention) silently kept the stale dims. r2f-assign.R's deferred-mode path assigns `var@dims <- value@value@dims` with a legitimately-NULL RHS and relied on this working. Assign the attribute directly (the same S7 workaround already used by the `r` property) to avoid recursing through the setter.
Same defect class as the floor()/ceiling() fix: `min` is spliced twice into the emitted expression, and for array results the implied-do re-evaluates spliced bounds once per element, so an impure bound such as `runif(2L, runif(1L), 10)` drew a fresh `min` value repeatedly where R evaluates it once. Hoist non-trivial bounds via hoist_unless_name(); bare names and literal bounds are unaffected.
583c465 to
f3e2460
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
==========================================
+ Coverage 93.14% 93.17% +0.03%
==========================================
Files 28 28
Lines 6028 6040 +12
==========================================
+ Hits 5615 5628 +13
+ Misses 413 412 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
t-kalinowski
left a comment
t-kalinowski
left a comment
There was a problem hiding this comment.
Thank you very much. These are great correctness fixes, and the clearer diagnostics make the failure modes easier to understand. I left two inline comments linked to follow-up issues, but neither needs to be addressed in this PR. Approving.
| test_that("Variable@dims can be reset to scalar with NULL", { | ||
| v <- quickr:::Variable("double", list(2L, 3L)) | ||
| expect_identical(v@dims, list(2L, 3L)) | ||
| v@dims <- NULL |
There was a problem hiding this comment.
This is consistent with the package’s current representation, where NULL means scalar, so I think it is fine for this PR.
Longer term, I think we should distinguish an uninferred shape (NULL) from a rank-0 scalar (list()). Variable() is used as a placeholder during inference, but its dims = NULL currently makes it report itself as scalar before its shape is known. I opened this follow-up issue to track that refactor; it requires a package-wide audit and does not need to expand this PR.
| test_that("assigning an expression that produces no value errors cleanly", { | ||
| fn <- function(x) { | ||
| declare(type(x = logical(1))) | ||
| y <- if (x) 1 else 2 |
There was a problem hiding this comment.
In R, this if does produce a value: this form always evaluates to either 1 or 2. “Does not produce a value” describes the current lowering limitation rather than the R expression.
I think we should eventually resolve a common type and shape from the two branches and assign the selected branch to y; we need that metadata to infer y anyway. I opened this follow-up issue to track that work. The new diagnostic is still clearer than the current crash, so this does not need to expand this PR.
2 participants
Fixes #122.
What changed
Six small, independent fixes, one commit each (plus a final formatting-only
commit, see notes):
floor()/ceiling()now evaluate their argument exactly once.Previously the argument expression was spliced into the emitted Fortran
three times, so
floor(runif(1) * 10)mixed two different random drawsand advanced the RNG state three times. Non-trivial arguments are now
hoisted to a block temporary:
Bare variable names are passed through unchanged, so side-effect-free
translations are unaffected.
runif()bounds are now evaluated exactly once — same defect class:minwas spliced twice, and for array results the implied-do re-evaluatedspliced bounds once per element, so
runif(2L, runif(1L), 10)drew afresh "min" repeatedly where R evaluates it once:
Bare-name and literal bounds are unaffected (no new temporaries).
max()/min()/sum()/prod()now reject named arguments with acompile-time error. Previously
sum(x, na.rm = TRUE)treatedna.rm = TRUEas a data argument and emitted(sum(x) + .true.), failinglater at the gfortran stage with an inscrutable type error (or, where the
operand typed through, computing the wrong thing). This matches the
existing
any()/all()guard.Assigning an expression that produces no value errors cleanly.
y <- if (x) 1 else 2(a value-less statement in the lowering) previouslycrashed with
no applicable method for `@` applied to an object of class "NULL"; it now errors withcannot assign `if (x) 1 else 2`: expression does not produce a value.r2size()no longer crashes on a variable whose mode is still beinginferred (
@modeNULL):var@mode != "integer"yieldedlogical(0), theenclosing
||collapsed toNA, andifaborted with "missing value whereTRUE/FALSE needed".
!identical(var@mode, "integer")reaches the intendedwarning path.
Variable@dims <- NULLnow resets the variable to scalar per theclass's documented "NULL means scalar" convention, instead of being a
silent no-op that kept stale dims. (
r2f-assign.R's mode-inferencereassignment path assigns
var@dims <- value@value@dims, where alegitimately-NULL right-hand side means scalar.)
How
New shared helper
hoist_unless_name(x, hoist)inR/r2f-aab-core.R(next to
new_hoist()), extracted from thematrix()handler's existinghoist-to-temporary logic;
matrix()now uses it, andfloor()/ceiling()(
R/r2f-math.R) and therunif()bounds (R/r2f-random.R) call it beforesplicing. Other handlers that splice an operand more than once can adopt it
incrementally (
rev()already hoists inline with the same pattern).The helper skips the temporary only for a bare variable name, so it
over-hoists literals —
floor(2.5)now spends a temporary it does notneed. That is deliberate here: teaching it about literals changes emitted
code for every existing caller, which would churn snapshots in a PR whose
point is a targeted correctness fix.
runif()takes a localis.atomic()shortcut to avoid the worst of it. PR 13 removes both theover-hoisting and the shortcut, in a PR where the snapshot churn is the
expected content.
R/r2f-reductions.R: named-argument guard at the top of themax/min/sum/prod handler, copied from any/all.
R/r2f-assign.R: NULL-@valuecheck in the new-binding path.R/sizes.R:!identical(var@mode, "integer").R/classes.R: thedimssetter assigns the attribute directly on emptyinput (same S7 workaround as the
rproperty) instead of early-returning.Tests
test-hoist-unless-name.R(new): floor/ceiling withrunif(1)argumentsmatch R under a fixed seed and leave the RNG state identical to R's; a
translation snapshot pins the single-
unif_randform; a bare-variable caseasserts no temporary is emitted.
test-errors.R: compile-time rejection ofna.rm =for all fourreductions; the clean void-assignment diagnostic.
test-runif.R:runif(2L, runif(1L), 10)matches R under a fixed seedwith identical RNG state after the call; translation snapshot pins the
hoisted bound.
test-internal-utils.R:r2size()warns instead of crashing on adeferred-mode variable.
test-classes.R:@dims <- NULLyieldsis_scalar.Snapshot churn: only the new
_snaps/hoist-unless-name.mdand an added casein
_snaps/runif.md. No existing snapshots change (floor/ceiling on bare names, the dominant case in thesuite, are untouched).
Notes for review
bare name still splices as
real(x, kind=c_double)(pure) without atemporary; only genuinely non-trivial expressions pay for one.
air format):=assignment becomes<-in three pre-existing test files (test-example-convolve.R,test-loops.R,test-size-constraint.R). No behavior change.test-internal-utils.R's "print.quickr_ordered_env outputs bindings" testfails on a clean checkout of
mainin my environment (R-version-dependentcapture.output/strbehavior, unrelated to this change) — called out soit isn't attributed to this PR.
diagnostics mentioned.