Skip to content

Commit 37ea0a5

Browse files
committed
Drop runtime guards for dynamic subscript bounds
Dynamic range guards added work to hot loops while checking only lower bounds. Upper bounds and symbolic scalar and vector subscripts remained unchecked, so the partial guard did not provide a coherent safety contract. Keep zero-cost compile-time validation for unsupported or statically invalid subscripts, and keep runtime validation required for seq() step semantics. Dynamically computed array bounds remain the caller's responsibility.
1 parent 1e7d819 commit 37ea0a5

5 files changed

Lines changed: 32 additions & 97 deletions

File tree

R/r2f-iterables-helpers.R

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -334,9 +334,8 @@ seq_like_r2f <- function(
334334
glue("{start}, {end}, {step}")
335335
}
336336
} else if (context == "[") {
337-
# `:`/`seq` sections are only correct for bounds >= 1 (and a step whose
338-
# sign matches); seq_len/seq_along need no check -- their worst case is
339-
# a legal zero-length section, which matches R's x[integer(0)].
337+
# Validate statically-known unsupported bounds and seq() step semantics.
338+
# Dynamically computed array bounds remain the caller's responsibility.
340339
if (kind %in% c(":", "seq")) {
341340
check_subscript_range_bounds(
342341
info,
@@ -364,14 +363,11 @@ seq_like_r2f <- function(
364363
Fortran(fr, val)
365364
}
366365

367-
# Validate an x[a:b] / x[seq(a, b, by)] index range.
368-
# In subscript context the emitted section `from:to:sign(1, to-from)` and its
369-
# claimed length `abs(to - from) + 1` are only correct when both bounds are
370-
# >= 1: R's x[1:0] drops the 0 (a value-dependent shape quickr cannot
371-
# produce), and the sign-stride section would read x(0). Statically-bad
372-
# literal bounds are compile errors; unknown bounds get a statement-level
373-
# runtime check via emit_quickr_error_if(). An explicit seq() step is
374-
# handled below (literal-only, plus a runtime wrong-sign check).
366+
# Validate an x[a:b] / x[seq(a, b, by)] index range where doing so has no
367+
# general bounds-checking cost. Statically bad literal bounds are compile
368+
# errors. Dynamic bounds are trusted, consistently with symbolic scalar and
369+
# vector subscripts. An explicit seq() step is handled below (literal-only,
370+
# plus a runtime wrong-sign check required by seq() semantics).
375371
# Used by: seq_like_r2f() (subscript context)
376372
check_subscript_range_bounds <- function(info, from, to, by_f, hoist, scope) {
377373
lit <- function(e) {
@@ -409,17 +405,6 @@ check_subscript_range_bounds <- function(info, from, to, by_f, hoist, scope) {
409405
emit_quickr_error_if(condition, message, hoist, scope)
410406
}
411407

412-
checks <- character()
413-
if (is.na(from_lit)) {
414-
checks <- c(checks, glue("{from} < 1_c_int"))
415-
}
416-
if (is.na(to_lit)) {
417-
checks <- c(checks, glue("{to} < 1_c_int"))
418-
}
419-
if (length(checks)) {
420-
emit(paste(checks, collapse = " .or. "), bounds_msg)
421-
}
422-
423408
# Explicit seq() step. When the endpoints differ, the result length divides
424409
# by the step, and that length is evaluated in the C bridge *before* any
425410
# Fortran guard can run (a zero step would be a division-by-zero crash

R/r2f-subscript.R

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,9 @@ r2f_handlers[["["]] <- function(
242242
# reads with NA and grows the vector on out-of-range writes -- neither
243243
# representable in quickr's static-shape model -- while the generated
244244
# Fortran would silently read or write out of bounds. Literal `:` range
245-
# endpoints are checked against the extent here too; their >= 1 lower-bound
246-
# validation (including the runtime guard for symbolic endpoints) stays in
247-
# check_subscript_range_bounds().
245+
# endpoints are checked against the extent here too; literal lower-bound and
246+
# seq() step validation stays in check_subscript_range_bounds(). Dynamic
247+
# subscript bounds remain the caller's responsibility.
248248
check_subscript_expr <- function(e, extent = NULL) {
249249
e <- unwrap_parens(e)
250250
while (is_call(e, quote(`-`)) && length(e) == 2L) {

tests/testthat/_snaps/example-roll_mean.md

Lines changed: 4 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,15 @@
2424
Code
2525
cat(fsub)
2626
Output
27-
subroutine fn(x, weights, normalize, out, weights__len_, x__len_, quickr_err_msg) bind(c)
28-
use iso_c_binding, only: c_char, c_double, c_int, c_null_char, c_ptrdiff_t
27+
subroutine fn(x, weights, normalize, out, weights__len_, x__len_) bind(c)
28+
use iso_c_binding, only: c_double, c_int, c_ptrdiff_t
2929
implicit none
3030
3131
! manifest start
3232
! sizes
3333
integer(c_ptrdiff_t), intent(in), value :: x__len_
3434
integer(c_ptrdiff_t), intent(in), value :: weights__len_
3535
36-
! error
37-
character(kind=c_char), intent(inout) :: quickr_err_msg(256)
38-
3936
! args
4037
real(c_double), intent(in) :: x(x__len_)
4138
real(c_double), intent(in out) :: weights(weights__len_)
@@ -54,24 +51,8 @@
5451
weights = ((weights / sum(weights)) * size(weights))
5552
end if
5653
do i = 1, size(out)
57-
if (i < 1_c_int .or. (((i + n) - 1_c_int)) < 1_c_int) then
58-
call quickr_set_error_msg("index ranges in x[a:b] must have bounds >= 1")
59-
return
60-
end if
6154
out(i) = (sum((x(i:(((i + n) - 1_c_int)):sign(1, (((i + n) - 1_c_int))-i)) * weights)) / real(size(weights), kind=c_double))
6255
end do
63-
64-
contains
65-
subroutine quickr_set_error_msg(msg)
66-
character(len=*), intent(in) :: msg
67-
integer :: i
68-
integer :: n
69-
if (quickr_err_msg(1) == c_null_char) then
70-
n = min(len(msg), 256 - 1)
71-
quickr_err_msg(1:n) = [(msg(i:i), i = 1, n)]
72-
quickr_err_msg(n + 1) = c_null_char
73-
end if
74-
end subroutine quickr_set_error_msg
7556
end subroutine
7657
Code
7758
cat(cwrapper)
@@ -87,8 +68,7 @@
8768
const int* const normalize__,
8869
double* const out__,
8970
const R_xlen_t weights__len_,
90-
const R_xlen_t x__len_,
91-
char* quickr_err_msg);
71+
const R_xlen_t x__len_);
9272
9373
SEXP fn_(SEXP _args) {
9474
// x
@@ -127,21 +107,13 @@
127107
SEXP out = PROTECT(Rf_allocVector(REALSXP, out__len_));
128108
double* out__ = REAL(out);
129109
130-
char quickr_err_msg[256];
131-
quickr_err_msg[0] = '\0';
132-
133-
134110
fn(
135111
x__,
136112
weights__,
137113
normalize__,
138114
out__,
139115
weights__len_,
140-
x__len_,
141-
quickr_err_msg);
142-
if (quickr_err_msg[0] != '\0') {
143-
Rf_error("%s", quickr_err_msg);
144-
}
116+
x__len_);
145117
146118
UNPROTECT(1);
147119
return out;

tests/testthat/_snaps/subscript-validation.md

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# x[a:b] guards against non-positive bounds at runtime
1+
# dynamic x[a:b] bounds compile without runtime bounds guards
22

33
Code
44
fn
@@ -11,41 +11,22 @@
1111
Code
1212
cat(fsub)
1313
Output
14-
subroutine fn(x, n, out_, x__len_, quickr_err_msg) bind(c)
15-
use iso_c_binding, only: c_char, c_double, c_int, c_null_char, c_ptrdiff_t
14+
subroutine fn(x, n, out_, x__len_) bind(c)
15+
use iso_c_binding, only: c_double, c_int, c_ptrdiff_t
1616
implicit none
1717
1818
! manifest start
1919
! sizes
2020
integer(c_ptrdiff_t), intent(in), value :: x__len_
2121
22-
! error
23-
character(kind=c_char), intent(inout) :: quickr_err_msg(256)
24-
2522
! args
2623
real(c_double), intent(in) :: x(x__len_)
2724
integer(c_int), intent(in) :: n
2825
real(c_double), intent(out) :: out_((abs((n - 1)) + 1))
2926
! manifest end
3027
3128
32-
if (n < 1_c_int) then
33-
call quickr_set_error_msg("index ranges in x[a:b] must have bounds >= 1")
34-
return
35-
end if
3629
out_ = x(1_c_int:n:sign(1, n-1_c_int))
37-
38-
contains
39-
subroutine quickr_set_error_msg(msg)
40-
character(len=*), intent(in) :: msg
41-
integer :: i
42-
integer :: n
43-
if (quickr_err_msg(1) == c_null_char) then
44-
n = min(len(msg), 256 - 1)
45-
quickr_err_msg(1:n) = [(msg(i:i), i = 1, n)]
46-
quickr_err_msg(n + 1) = c_null_char
47-
end if
48-
end subroutine quickr_set_error_msg
4930
end subroutine
5031
Code
5132
cat(cwrapper)
@@ -59,8 +40,7 @@
5940
const double* const x__,
6041
const int* const n__,
6142
double* const out___,
62-
const R_xlen_t x__len_,
63-
char* quickr_err_msg);
43+
const R_xlen_t x__len_);
6444
6545
SEXP fn_(SEXP _args) {
6646
// x
@@ -89,19 +69,11 @@
8969
SEXP out_ = PROTECT(Rf_allocVector(REALSXP, out___len_));
9070
double* out___ = REAL(out_);
9171
92-
char quickr_err_msg[256];
93-
quickr_err_msg[0] = '\0';
94-
95-
9672
fn(
9773
x__,
9874
n__,
9975
out___,
100-
x__len_,
101-
quickr_err_msg);
102-
if (quickr_err_msg[0] != '\0') {
103-
Rf_error("%s", quickr_err_msg);
104-
}
76+
x__len_);
10577
10678
UNPROTECT(1);
10779
return out_;

tests/testthat/test-subscript-validation.R

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
# Unit tests for subscript validation: negative/zero rejection and
2-
# runtime guards on index ranges
1+
# Unit tests for compile-time subscript validation and seq() step guards
32

43
skip_on_cran()
54

@@ -168,16 +167,14 @@ test_that("assignment subscripts get the same validation as reads", {
168167
expect_quick_identical(fok, list(matrix(as.double(1:6), 2, 3)))
169168
})
170169

171-
test_that("x[a:b] guards against non-positive bounds at runtime", {
170+
test_that("dynamic x[a:b] bounds compile without runtime bounds guards", {
172171
fn <- function(x, n) {
173172
declare(type(x = double(NA)), type(n = integer(1)))
174173
x[1:n]
175174
}
176-
expect_translation_snapshots(fn) # pins the guard text
175+
expect_translation_snapshots(fn) # pins the absence of a bounds guard
177176
qfn := quick(fn)
178177
expect_equal(qfn(as.double(1:5), 3L), c(1, 2, 3))
179-
expect_error(qfn(as.double(1:5), 0L), "bounds >= 1")
180-
expect_error(qfn(as.double(1:5), -2L), "bounds >= 1")
181178

182179
# descending ranges still work
183180
fdesc <- function(x, n) {
@@ -186,7 +183,6 @@ test_that("x[a:b] guards against non-positive bounds at runtime", {
186183
}
187184
qdesc := quick(fdesc)
188185
expect_equal(qdesc(as.double(1:4), 4L), c(4, 3, 2, 1))
189-
expect_error(qdesc(as.double(1:4), 0L), "bounds >= 1")
190186
})
191187

192188
test_that("literal in-range bounds emit no guard; bad literals error at compile time", {
@@ -227,6 +223,16 @@ test_that("non-singleton x[seq(a, b, by)] validates the step", {
227223
qfs := quick(fs)
228224
expect_equal(qfs(as.double(1:9), 8L), c(5, 6, 7, 8))
229225
expect_error(qfs(as.double(1:9), 2L), "wrong sign")
226+
227+
# Scalar results do not need the sequence length in the C bridge, so the
228+
# Fortran guard must continue to validate the step in this context.
229+
fsum <- function(x, n) {
230+
declare(type(x = double(NA)), type(n = integer(1)))
231+
sum(x[seq(5L, n, by = 1L)])
232+
}
233+
qsum := quick(fsum)
234+
expect_equal(qsum(as.double(1:9), 8L), sum(5:8))
235+
expect_error(qsum(as.double(1:9), 2L), "wrong sign")
230236
})
231237

232238
test_that("a singleton seq() subscript accepts a symbolic step", {

0 commit comments

Comments
 (0)