Skip to content

Commit

Permalink
Check length of pluck indices (#896)
Browse files Browse the repository at this point in the history
Even when indexing `NULL`. Also correctly reports the undesired type of index.

Fixes #813
hadley authored Aug 29, 2022
1 parent 5d423cb commit eb7a929
Showing 6 changed files with 67 additions and 26 deletions.
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -30,6 +30,9 @@

## Features and fixes

* `pluck()` now requires indices to be length 1 (#813). It also now reports
the correct type if you supply an unexpected index.

* `pluck()` now accepts negative integers, indexing from the right (#603).

* `pluck()` and `chuck()` now fail if you provide named inputs to ... (#788).
27 changes: 5 additions & 22 deletions src/pluck.c
Original file line number Diff line number Diff line change
@@ -7,7 +7,6 @@
#include "coerce.h"
#include "conditions.h"

static int check_input_lengths(int n, SEXP index, int i, bool strict);
static int check_double_index_finiteness(double val, SEXP index, int i, bool strict);
static int check_double_index_length(double val, int n, int i, bool strict);
static int check_character_index(SEXP string, int i, bool strict);
@@ -29,8 +28,9 @@ int find_offset(SEXP x, SEXP index, int i, bool strict) {
return -1;
}

if (check_input_lengths(n, index, i, strict)) {
return -1;
int index_n = Rf_length(index);
if (index_n != 1) {
stop_bad_element_length(index, i + 1, 1, "Index", NULL, false);
}

switch (TYPEOF(index)) {
@@ -103,7 +103,7 @@ int find_offset(SEXP x, SEXP index, int i, bool strict) {
}

default:
stop_bad_element_type(x, i + 1, "a character or numeric vector", "Index", NULL);
stop_bad_element_type(index, i + 1, "a character or numeric vector", "Index", NULL);
}
}

@@ -226,6 +226,7 @@ SEXP pluck_impl(SEXP x, SEXP index, SEXP missing, SEXP strict_arg) {

switch (TYPEOF(x)) {
case NILSXP:
find_offset(x, index_i, i, strict);
if (strict) {
Rf_errorcall(R_NilValue, "Plucked object can't be NULL.");
}
@@ -264,24 +265,6 @@ SEXP pluck_impl(SEXP x, SEXP index, SEXP missing, SEXP strict_arg) {

/* Type checking */

static int check_input_lengths(int n, SEXP index, int i, bool strict) {
int index_n = Rf_length(index);

if (n == 0) {
if (strict) {
Rf_errorcall(R_NilValue, "Plucked object must have at least one element.");
} else {
return -1;
}
}

if (index_n > 1 || (strict && index_n == 0)) {
stop_bad_element_length(index, i + 1, 1, "Index", NULL, false);
}

return 0;
}

static int check_double_index_finiteness(double val, SEXP index, int i, bool strict) {
if (R_finite(val)) {
return 0;
8 changes: 8 additions & 0 deletions tests/testthat/_snaps/chuck.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
# trying to chuck NULL raises errors

Code
chuck(NULL, "a")
Condition
Error:
! Index 1 is attempting to pluck from an unnamed vector using a string name.

# dots must be unnamed

Code
36 changes: 36 additions & 0 deletions tests/testthat/_snaps/pluck.md
Original file line number Diff line number Diff line change
@@ -8,3 +8,39 @@
x Problematic argument:
* a = 1

# require length 1 character/double vectors

Code
pluck(1, 1:2)
Condition
Error in `stop_bad_length()`:
! Index 1 must have length 1, not 2
Code
pluck(1, integer())
Condition
Error in `stop_bad_length()`:
! Index 1 must have length 1, not 0
Code
pluck(1, NULL)
Condition
Error in `stop_bad_length()`:
! Index 1 must have length 1, not 0
Code
pluck(1, TRUE)
Condition
Error in `stop_bad_type()`:
! Index 1 must be a character or numeric vector, not a logical vector

# validate index even when indexing NULL

Code
pluck(NULL, 1:2)
Condition
Error in `stop_bad_length()`:
! Index 1 must have length 1, not 2
Code
pluck(NULL, TRUE)
Condition
Error in `stop_bad_type()`:
! Index 1 must be a character or numeric vector, not a logical vector

2 changes: 1 addition & 1 deletion tests/testthat/test-chuck.R
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# NULL input ----------------------------------------------------------------

test_that("trying to chuck NULL raises errors", {
expect_error(chuck(NULL, "a"), "can't be NULL")
expect_snapshot(chuck(NULL, "a"), error = TRUE)
})

test_that("dots must be unnamed", {
17 changes: 14 additions & 3 deletions tests/testthat/test-pluck.R
Original file line number Diff line number Diff line change
@@ -51,10 +51,21 @@ test_that("can pluck by name and position", {
expect_equal(pluck(x, "a", 1, "b"), 1)
})

test_that("require length 1 character/double vectors", {
expect_snapshot(error = TRUE, {
pluck(1, 1:2)
pluck(1, integer())
pluck(1, NULL)
pluck(1, TRUE)
})
})

test_that("validate index even when indexing NULL", {
expect_snapshot(error = TRUE, {
pluck(NULL, 1:2)
pluck(NULL, TRUE)
})

test_that("require length 1 vectors", {
expect_bad_element_length_error(pluck(1, letters), "must have length 1")
expect_bad_element_type_error(pluck(1, TRUE), "Index 1 must be a character or numeric vector")
})

test_that("special indexes never match", {

0 comments on commit eb7a929

Please sign in to comment.