plotly / plotly/plotly.R

NSE Arguments in functions calling plotly raise an error

Open
#2,249 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
R
Stars
2.7k
Forks
641
PR merge metrics
No merged PRs in 30d

Description

While toying with different NSE approaches I stumbled upon the following behaviour which seems to me like a bug. Any NSE argument (even if not used in plotly) results in an error:

library(plotly)

nse_arg_raises_error <- function(xx) {
   mf <- rlang::new_formula(NULL, rlang::ensym(xx), globalenv())
   plot_ly(mtcars, x = ~ cyl, y = ~ mpg, type = "scatter",
           mode = "markers")
}
nse_arg_raises_error()    ## works
nse_arg_raises_error(cyl) ## does not work
#> Error in as.list.environment(x, all.names = TRUE): object 'cyl' not found

This behaviour is unique to plotly, b/c if I leave out the plot_ly call, the error does not happen.

it_is_a_plotly_problem <- function(xx) {
   mf <- rlang::new_formula(NULL, rlang::ensym(xx), globalenv())
   1 + 1
}
it_is_a_plotly_problem()     ## works
#> [1] 2
it_is_a_plotly_problem(cyl)  ## also works
#> [1] 2

However, some preliminary debugging shows that the culprit sits (deep) in base:::as.list.environment (which is called from lazyeval:::clone_env, which in turn together with its brethren from lazyeval is responsible for the NSE part in plot_ly)

it_is_a_as_list_environment_issue <- function(xx) {
   mf <- rlang::new_formula(NULL, rlang::ensym(xx), globalenv())
   cp <- as.list.environment(environment())
   1 + 1
}
it_is_a_as_list_environment_issue(cyl)
#> Error in as.list.environment(x, all.names = TRUE): object 'cyl' not found

So basically, when lazyeval:::clone_env copies the function environment which contains an NSE argument this error from within the C code implementation of base::as.list.environment bubbles up.

I am inclined to say that this a feature and not a bug of as.list.environment and the user of it should make sure that environments using it are in fact able to be safely transformed into a list. Thus, I am posting the issue here as it touches the whole NSE strategy of plotly - coming from plotly:::eval_attr (especially since lazyeval is retired in favor of rlang).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the issue with the provided nse_arg_raises_error example, then trace plotly:::eval_attr into lazyeval:::clone_env and the call to base:::as.list.environment. Check how the plotly NSE path handles a function environment containing an unused NSE argument. Done means the demonstrated plot_ly call no longer raises the as.list.environment error; the payload names no test file.

Written by the indexing model from the issue text.

Assessment

Tech stack
r
Domain
data-visualization
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.