Skip to content

Commit bd43518

Browse files
committed
Enforce the ifelse() branch-shape contract
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.
1 parent cc8def7 commit bd43518

3 files changed

Lines changed: 246 additions & 8 deletions

File tree

R/r2f-conditionals.R

Lines changed: 89 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,86 @@
11
# r2f-conditionals.R
22
# Handlers for vectorized conditionals: ifelse
33

4-
# --- Handlers ---
4+
# --- Local Helpers ---
55

6-
r2f_handlers[["ifelse"]] <- function(args, scope, ...) {
7-
.[mask, tsource, fsource] <- lapply(args, r2f, scope, ...)
8-
mask <- booleanize_logical_as_int(mask)
6+
ifelse_branch_shape_msg <- paste0(
7+
"ifelse() `yes` and `no` must be scalars or match the shape of `test`; ",
8+
"R-style recycling is not supported"
9+
)
910

10-
# merge() requires same-typed branches; promote both to their common mode.
11-
promoted <- promote_operands(list(tsource, fsource), context = "ifelse()")
12-
.[tsource, fsource] <- promoted$args
13-
mode <- promoted$mode
11+
# Three-valued conformability verdict for one axis of an ifelse() branch
12+
# against `test`: ok+known (no guard), not-ok+known (compile error), or
13+
# unknown (runtime guard). NA dims are always unknown: two unknown lengths
14+
# are not the same quantity.
15+
ifelse_axis_verdict <- function(test_dim, branch_dim) {
16+
if (is_wholenumber(test_dim) && is_wholenumber(branch_dim)) {
17+
return(list(
18+
ok = identical(as.integer(test_dim), as.integer(branch_dim)),
19+
unknown = FALSE
20+
))
21+
}
22+
if (!is_scalar_na(test_dim) && !is_scalar_na(branch_dim)) {
23+
test_norm <- fortranize_expr_symbols(test_dim)
24+
branch_norm <- fortranize_expr_symbols(branch_dim)
25+
if (identical(test_norm, branch_norm)) {
26+
return(list(ok = TRUE, unknown = FALSE))
27+
}
28+
}
29+
list(ok = TRUE, unknown = TRUE)
30+
}
31+
32+
# Enforce the shape contract for one ifelse() branch: scalars broadcast
33+
# natively; a non-scalar branch must match `test`'s shape, because
34+
# merge() requires conformable arguments and a runtime mismatch would
35+
# read past the shorter branch. Statically unequal dims are a compile
36+
# error; symbolic dims get a statement-level runtime size guard.
37+
check_ifelse_branch_shape <- function(branch, mask, hoist, scope) {
38+
if (passes_as_scalar(branch@value)) {
39+
return(invisible())
40+
}
41+
if (branch@value@rank != mask@value@rank) {
42+
stop(ifelse_branch_shape_msg, call. = FALSE)
43+
}
44+
unknown_axes <- integer()
45+
for (axis in seq_len(mask@value@rank)) {
46+
verdict <- ifelse_axis_verdict(
47+
dim_or_one(mask, axis),
48+
dim_or_one(branch, axis)
49+
)
50+
if (!verdict$ok) {
51+
stop(ifelse_branch_shape_msg, call. = FALSE)
52+
}
53+
if (verdict$unknown) {
54+
unknown_axes <- c(unknown_axes, axis)
55+
}
56+
}
57+
if (!length(unknown_axes)) {
58+
return(invisible())
59+
}
60+
if (is.null(hoist)) {
61+
stop(
62+
"cannot emit a runtime length guard here; ",
63+
"ifelse() branch lengths must match `test` statically",
64+
call. = FALSE
65+
)
66+
}
67+
# size() is an inquiry, so applying it to operand expression text does
68+
# not evaluate the operands.
69+
condition <- str_flatten(
70+
map_chr(
71+
unknown_axes,
72+
function(axis) glue("size({branch}, {axis}) /= size({mask}, {axis})")
73+
),
74+
" .or. "
75+
)
76+
emit_quickr_error_if(condition, ifelse_branch_shape_msg, hoist, scope)
77+
invisible()
78+
}
79+
80+
# --- Handlers ---
81+
82+
r2f_handlers[["ifelse"]] <- function(args, scope, ..., hoist = NULL) {
83+
.[mask, tsource, fsource] <- lapply(args, r2f, scope, ..., hoist = hoist)
1484

1585
# R: the result is shaped like `test` (branches only contribute values).
1686
# A scalar test with array branches is not representable with merge().
@@ -25,6 +95,17 @@ r2f_handlers[["ifelse"]] <- function(args, scope, ...) {
2595
)
2696
}
2797

98+
# Checked before casts so guards splice the bare operand text.
99+
check_ifelse_branch_shape(tsource, mask, hoist, scope)
100+
check_ifelse_branch_shape(fsource, mask, hoist, scope)
101+
102+
mask <- booleanize_logical_as_int(mask)
103+
104+
# merge() requires same-typed branches; promote both to their common mode.
105+
promoted <- promote_operands(list(tsource, fsource), context = "ifelse()")
106+
.[tsource, fsource] <- promoted$args
107+
mode <- promoted$mode
108+
28109
Fortran(
29110
glue("merge({tsource}, {fsource}, {mask})"),
30111
Variable(mode, mask@value@dims)

tests/testthat/_snaps/ifelse.md

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,132 @@
7979
return out_;
8080
}
8181

82+
# ifelse guards unknown branch lengths at runtime
83+
84+
Code
85+
fn
86+
Output
87+
function(c, a, b) {
88+
declare(type(c = logical(NA)), type(a = double(NA)), type(b = double(NA)))
89+
ifelse(c, a, b)
90+
}
91+
<environment: 0x0>
92+
Code
93+
cat(fsub)
94+
Output
95+
subroutine fn(c, a, b, out_, a__len_, b__len_, c__len_, quickr_err_msg) bind(c)
96+
use iso_c_binding, only: c_char, c_double, c_int, c_null_char, c_ptrdiff_t
97+
implicit none
98+
99+
! manifest start
100+
! sizes
101+
integer(c_ptrdiff_t), intent(in), value :: c__len_
102+
integer(c_ptrdiff_t), intent(in), value :: a__len_
103+
integer(c_ptrdiff_t), intent(in), value :: b__len_
104+
105+
! error
106+
character(kind=c_char), intent(inout) :: quickr_err_msg(256)
107+
108+
! args
109+
integer(c_int), intent(in) :: c(c__len_) ! logical
110+
real(c_double), intent(in) :: a(a__len_)
111+
real(c_double), intent(in) :: b(b__len_)
112+
real(c_double), intent(out) :: out_(c__len_)
113+
! manifest end
114+
115+
116+
if (size(a, 1) /= size((c/=0), 1)) then
117+
call quickr_set_error_msg("ifelse() `yes` and `no` must be scalars or match the shape of `test`; R-style recycling is not&
118+
& supported")
119+
return
120+
end if
121+
if (size(b, 1) /= size((c/=0), 1)) then
122+
call quickr_set_error_msg("ifelse() `yes` and `no` must be scalars or match the shape of `test`; R-style recycling is not&
123+
& supported")
124+
return
125+
end if
126+
out_ = merge(a, b, (c/=0))
127+
128+
contains
129+
subroutine quickr_set_error_msg(msg)
130+
character(len=*), intent(in) :: msg
131+
integer :: i
132+
integer :: n
133+
if (quickr_err_msg(1) == c_null_char) then
134+
n = min(len(msg), 256 - 1)
135+
quickr_err_msg(1:n) = [(msg(i:i), i = 1, n)]
136+
quickr_err_msg(n + 1) = c_null_char
137+
end if
138+
end subroutine quickr_set_error_msg
139+
end subroutine
140+
Code
141+
cat(cwrapper)
142+
Output
143+
#define R_NO_REMAP
144+
#include <R.h>
145+
#include <Rinternals.h>
146+
147+
148+
extern void fn(
149+
const int* const c__,
150+
const double* const a__,
151+
const double* const b__,
152+
double* const out___,
153+
const R_xlen_t a__len_,
154+
const R_xlen_t b__len_,
155+
const R_xlen_t c__len_,
156+
char* quickr_err_msg);
157+
158+
SEXP fn_(SEXP _args) {
159+
// c
160+
_args = CDR(_args);
161+
SEXP c = CAR(_args);
162+
if (TYPEOF(c) != LGLSXP) {
163+
Rf_error("typeof(c) must be 'logical', not '%s'", Rf_type2char(TYPEOF(c)));
164+
}
165+
const int* const c__ = LOGICAL(c);
166+
const R_xlen_t c__len_ = Rf_xlength(c);
167+
168+
// a
169+
_args = CDR(_args);
170+
SEXP a = CAR(_args);
171+
if (TYPEOF(a) != REALSXP) {
172+
Rf_error("typeof(a) must be 'double', not '%s'", Rf_type2char(TYPEOF(a)));
173+
}
174+
const double* const a__ = REAL(a);
175+
const R_xlen_t a__len_ = Rf_xlength(a);
176+
177+
// b
178+
_args = CDR(_args);
179+
SEXP b = CAR(_args);
180+
if (TYPEOF(b) != REALSXP) {
181+
Rf_error("typeof(b) must be 'double', not '%s'", Rf_type2char(TYPEOF(b)));
182+
}
183+
const double* const b__ = REAL(b);
184+
const R_xlen_t b__len_ = Rf_xlength(b);
185+
186+
const R_xlen_t out___len_ = c__len_;
187+
SEXP out_ = PROTECT(Rf_allocVector(REALSXP, out___len_));
188+
double* out___ = REAL(out_);
189+
190+
char quickr_err_msg[256];
191+
quickr_err_msg[0] = '\0';
192+
193+
194+
fn(
195+
c__,
196+
a__,
197+
b__,
198+
out___,
199+
a__len_,
200+
b__len_,
201+
c__len_,
202+
quickr_err_msg);
203+
if (quickr_err_msg[0] != '\0') {
204+
Rf_error("%s", quickr_err_msg);
205+
}
206+
207+
UNPROTECT(1);
208+
return out_;
209+
}
210+

tests/testthat/test-ifelse.R

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,3 +50,31 @@ test_that("ifelse with scalar test and array branch errors cleanly", {
5050
}
5151
expect_error(quick(fn), "shape of `test`")
5252
})
53+
54+
test_that("ifelse with statically mismatched branch lengths is a compile error", {
55+
fn <- function(c, a) {
56+
declare(type(c = logical(3)), type(a = double(2)))
57+
ifelse(c, a, 0)
58+
}
59+
expect_error(quick(fn), "R-style recycling is not supported")
60+
})
61+
62+
test_that("ifelse guards unknown branch lengths at runtime", {
63+
fn <- function(c, a, b) {
64+
declare(type(c = logical(NA)), type(a = double(NA)), type(b = double(NA)))
65+
ifelse(c, a, b)
66+
}
67+
# locks the size guards: a bare merge() with runtime-mismatched
68+
# assumed-shape vectors read past the shorter branch (returned garbage
69+
# like 4.65e-310 where R recycles)
70+
expect_translation_snapshots(fn)
71+
qfn <- quick(fn)
72+
73+
cc <- c(TRUE, FALSE, TRUE)
74+
a <- c(10, 20, 30)
75+
b <- c(1, 2, 3)
76+
expect_identical(qfn(cc, a, b), ifelse(cc, a, b))
77+
78+
expect_error(qfn(cc, c(10, 20), b), "match the shape of `test`")
79+
expect_error(qfn(cc, a, c(1, 2, 3, 4)), "match the shape of `test`")
80+
})

0 commit comments

Comments
 (0)