Merge pull request #255 from rstudio/bugfix/python-access-items-attri…

…butes

define separate semantics for `$`, and `[[`

NAMESPACE

@@ -1,6 +1,7 @@
 # Generated by roxygen2: do not edit by hand
 
 S3method("!=",python.builtin.object)
+S3method("$",python.builtin.dict)
 S3method("$",python.builtin.module)
 S3method("$",python.builtin.object)
 S3method("$<-",python.builtin.dict)
@@ -10,6 +11,10 @@ S3method("<=",python.builtin.object)
 S3method("==",python.builtin.object)
 S3method(">",python.builtin.object)
 S3method(">=",python.builtin.object)
+S3method("[",python.builtin.dict)
+S3method("[",python.builtin.object)
+S3method("[<-",python.builtin.dict)
+S3method("[[",python.builtin.dict)
 S3method("[[",python.builtin.object)
 S3method("[[<-",python.builtin.dict)
 S3method("[[<-",python.builtin.object)

R/python-dict.R

@@ -0,0 +1,47 @@
+#' @export
+`$.python.builtin.dict` <- function(x, name) {
+  if (py_is_null_xptr(x) || !py_available())
+    return(NULL)
+
+  if (py_has_attr(x, name)) {
+    item <- py_get_attr(x, name)
+    return(py_maybe_convert(item, py_has_convert(x)))
+  }
+
+  `[.python.builtin.dict`(x, name)
+}
+
+#' @export
+`[.python.builtin.dict` <- function(x, name) {
+  if (py_is_null_xptr(x) || !py_available())
+    return(NULL)
+
+  item <- py_dict_get_item(x, name)
+  py_maybe_convert(item, py_has_convert(x))
+}
+
+#' @export
+`[[.python.builtin.dict` <- `[.python.builtin.dict`
+
+#' @export
+`$<-.python.builtin.dict` <- function(x, name, value) {
+  if (!py_is_null_xptr(x) && py_available())
+    py_dict_set_item(x, name, value)
+  else
+    stop("Unable to assign value (dict reference is NULL)")
+  x
+}
+
+#' @export
+`[<-.python.builtin.dict` <- `$<-.python.builtin.dict`
+
+#' @export
+`[[<-.python.builtin.dict` <- `$<-.python.builtin.dict`
+
+#' @export
+length.python.builtin.dict <- function(x) {
+  if (py_is_null_xptr(x) || !py_available())
+    0L
+  else
+    py_dict_length(x)
+}

R/python.R

@@ -220,9 +220,28 @@ py_has_convert <- function(x) {
     TRUE
 }
 
-#' @export
-`$.python.builtin.object` <- function(x, name) {
+py_maybe_convert <- function(x, convert) {
+  if (convert || py_is_callable(x)) {
+
+    # capture previous convert for attr
+    attrib_convert <- py_has_convert(x)
+
+    # temporarily change convert so we can call py_to_r and get S3 dispatch
+    envir <- as.environment(x)
+    assign("convert", convert, envir = envir)
+    on.exit(assign("convert", attrib_convert, envir = envir), add = TRUE)
 
+    # call py_to_r
+    x <- py_to_r(x)
+  }
+
+  x
+}
+
+# helper function for accessing attributes or items from a
+# Python object, after validating that we do indeed have
+# a valid Python object reference
+py_get_attr_or_item <- function(x, name, prefer_attr) {
   # resolve module proxies
   if (py_is_module_proxy(x))
     py_resolve_module_proxy(x)
@@ -231,41 +250,66 @@ py_has_convert <- function(x) {
   if (py_is_null_xptr(x) || !py_available())
     return(NULL)
 
-  # deterimine whether this object converts to python
-  convert <- py_has_convert(x)
-
   # special handling for embedded modules (which don't always show
   # up as "attributes")
   if (py_is_module(x) && !py_has_attr(x, name)) {
-    module <- py_get_submodule(x, name, convert)
+    module <- py_get_submodule(x, name, py_has_convert(x))
     if (!is.null(module))
       return(module)
   }
 
-  # get the attrib
-  if (is.numeric(name) && (length(name) == 1) && py_has_attr(x, "__getitem__"))
-    attrib <- x$`__getitem__`(as.integer(name))
-  else if (inherits(x, "python.builtin.dict"))
-    attrib <- py_dict_get_item(x, name)
-  else
-    attrib <- py_get_attr(x, name)
+  # re-cast numeric values as integers
+  if (is.numeric(name))
+    name <- as.integer(name)
 
-  # convert
-  if (convert || py_is_callable(attrib)) {
+  # attributes must always be indexed by strings, so if
+  # we receive a non-string 'name', we call py_get_item
+  if (!is.character(name)) {
+    item <- py_get_item(x, name)
+    return(py_maybe_convert(item, py_has_convert(x)))
+  }
 
-    # capture previous convert for attr
-    attrib_convert <- py_has_convert(attrib)
+  # get the attrib and convert as needed
+  if (prefer_attr) {
 
-    # temporarily change convert so we can call py_to_r and get S3 dispatch
-    envir <- as.environment(attrib)
-    assign("convert", convert, envir = envir)
-    on.exit(assign("convert", attrib_convert, envir = envir), add = TRUE)
+    if (py_has_attr(x, name)) {
+      object <- py_get_attr(x, name)
+    } else {
+      object <- py_get_item(x, name)
+    }
+
+  } else {
+
+    # if we have an attribute, attempt to get the item
+    # but allow for fallback to that attribute
+    if (py_has_attr(x, name)) {
+      object <- py_get_item(x, name, silent = TRUE)
+      if (is.null(object))
+        object <- py_get_attr(x, name)
+    } else {
+      # we don't have an attribute; only attempt item
+      # access and allow normal error propagation
+      object <- py_get_item(x, name)
+    }
 
-    # call py_to_r
-    py_to_r(attrib)
   }
-  else
-    attrib
+
+  py_maybe_convert(object, py_has_convert(x))
+}
+
+#' @export
+`$.python.builtin.object` <- function(x, name) {
+  py_get_attr_or_item(x, name, TRUE)
+}
+
+#' @export
+`[.python.builtin.object` <- function(x, name) {
+  py_get_attr_or_item(x, name, FALSE)
+}
+
+#' @export
+`[[.python.builtin.object` <- function(x, name) {
+  py_get_attr_or_item(x, name, FALSE)
 }
 
 
@@ -302,28 +346,6 @@ as.environment.python.builtin.object <- function(x) {
 `[[<-.python.builtin.object` <- `$<-.python.builtin.object`
 
 
-#' @export
-`$<-.python.builtin.dict` <- function(x, name, value) {
-  if (!py_is_null_xptr(x) && py_available())
-    py_dict_set_item(x, name, value)
-  else
-    stop("Unable to assign value (dict reference is NULL)")
-  x
-}
-
-#' @export
-`[[<-.python.builtin.dict` <- `$<-.python.builtin.dict`
-
-#' @export
-length.python.builtin.dict <- function(x) {
-  if (py_is_null_xptr(x) || !py_available())
-    0L
-  else
-    py_dict_length(x)
-}
-
-
-
 #' @export
 .DollarNames.python.builtin.module <- function(x, pattern = "") {
 
@@ -822,30 +844,30 @@ py_list_attributes <- function(x) {
 #'
 #' Retrieve an item from a Python object, similar to how
 #' \code{x[name]} might be used in Python code to access an
-#' item called `name` on an object `x`. The object's
+#' item indexed by `key` on an object `x`. The object's
 #' `__getitem__` method will be called.
 #'
 #' @param x A Python object.
-#' @param name The item name.
+#' @param key The key used for item lookup.
 #' @param silent Boolean; when \code{TRUE}, attempts to access
 #'   missing items will return \code{NULL} rather than
 #'   throw an error.
 #'
 #' @family item-related APIs
 #' @export
-py_get_item <- function(x, name, silent = FALSE) {
+py_get_item <- function(x, key, silent = FALSE) {
   ensure_python_initialized()
   if (py_is_module_proxy(x))
     py_resolve_module_proxy(x)
 
   if (!py_has_attr(x, "__getitem__"))
-    stop("Python object has no '__getitem__' method")
+    stop("Python object has no '__getitem__' method", call. = FALSE)
   getitem <- py_to_r(py_get_attr(x, "__getitem__", silent = FALSE))
 
   item <- if (silent)
-    tryCatch(getitem(name), error = function(e) NULL)
+    tryCatch(getitem(key), error = function(e) NULL)
   else
-    getitem(name)
+    getitem(key)
 
   item
 }
@@ -871,7 +893,7 @@ py_set_item <- function(x, name, value) {
     py_resolve_module_proxy(x)
 
   if (!py_has_attr(x, "__setitem__"))
-    stop("Python object has no '__setitem__' method")
+    stop("Python object has no '__setitem__' method", call. = FALSE)
   setitem <- py_to_r(py_get_attr(x, "__setitem__", silent = FALSE))
 
   setitem(name, value)
@@ -896,7 +918,7 @@ py_del_item <- function(x, name) {
     py_resolve_module_proxy(x)
 
   if (!py_has_attr(x, "__delitem__"))
-    stop("Python object has no '__delitem__' method")
+    stop("Python object has no '__delitem__' method", call. = FALSE)
   delitem <- py_to_r(py_get_attr(x, "__delitem__", silent = FALSE))
 
   delitem(name)

tests/testthat/test-python-dict.R

@@ -41,3 +41,16 @@ test_that("Dictionary items can be get / set / removed with py_item APIs", {
   expect_error(py_get_item(d, "apple"))
   expect_identical(py_get_item(d, "apple", silent = TRUE), NULL)
 })
+
+test_that("$, [ operators behave as expected", {
+  skip_if_no_python()
+
+  d <- dict(items = 1, apple = 42)
+
+  expect_true(is.function(d$items))
+  expect_true(d['items'] == 1)
+
+  expect_true(d$apple == 42)
+  expect_true(d['apple'] == 42)
+
+})