Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recognize symbols created by delayedAssign / assign / makeActiveBinding #364

Merged
merged 15 commits into from
Jan 12, 2021

Conversation

shrektan
Copy link
Contributor

@shrektan shrektan commented Jan 8, 2021

Closes #362

A simple test file

x <- 1
y <- "a"
z <- function() {
  'hello'
}
delayedAssign("d1", 1)
delayedAssign(value = function() 2, x = "d2")
base::delayedAssign(value = "3", "d3")
var <- "d4"
delayedAssign(var, 4)
delayedAssign(("d5"), var)

makeActiveBinding("a1", function() 1, environment())
makeActiveBinding(function() "2", sym = "a2")
base::makeActiveBinding(fun = function() stop("3"), sym = "a3")
makeActiveBinding(("a4"), function() 1, environment())

assign(value = "1", "assign1")

Outline can display the promise / active binding symbols now

image

R/document.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@renkun-ken
Copy link
Member

It does not seem to work with the following:

makeActiveBinding(("a"), function() 1, environment())

We could simply ignore these calls with the name being not simply a string literal.

@shrektan shrektan changed the title Recongize promise / active binding symbols Recognize symbols created by delayedAssign / assign / makeActiveBinding Jan 9, 2021
@renkun-ken
Copy link
Member

renkun-ken commented Jan 9, 2021

I just recall the reason we didn't detect these assign functions. Consider the following examples:

e <- new.env()
assign("var", 1, e)
assign("var", 2, globalenv())
assign("var", 3, .GlobalEnv)

All these assign functions have an argument to specify which environment the assignment should be done. The document outline only lists the global symbols in the document. However, these assignments may be done in any environment. Should we detect the pos or envir argument?

@shrektan
Copy link
Contributor Author

shrektan commented Jan 9, 2021

Yes, I'm aware of this issue. My personal points are:

  1. The outlined symbols are not necessarily "global" - as the script may be used in an R package or be source() with local=TRUE. So maybe we should call them as "top-level" symbols.
  2. Except assign(), I guess in majority cases, users use delayedAssign() or makeActiveBinding() on the top level to make "top-level" variables.
  3. "Greedy" may be a good choice for the document outline purpose.

Simple improvements for this, in my opinion, could be detecting the pos / envir / assign.env argument and only mark the symbols as top-level, when these arguments:

  • are not provided, or
  • are provided and equal to default (-1 / parent.frame(1)) or .GlobalEnv, etc...

However, I'm hesitate to do it as it complicates the code and it's difficult to consider all those possibilities.

So my suggestion is that we may choose one of the two strategy:

  1. Greedy strategy: Ignore the possibility that those symbols may be assigned in a different context, for the time being. We can improve this when users complain that this causes trouble.
  2. Detecting those assigning environment arguments but using a simple strategy. That is we may assume that if those args are provided, it means users want to assign the symbols to a different environment - so we don't display them in the outline. In short, we ignore those symbols whose assigning env arguments (e.g., pos or env) are provided.

What do you think?

@renkun-ken
Copy link
Member

Detecting those assigning environment arguments but using a simple strategy. That is we may assume that if those args are provided, it means users want to assign the symbols to a different environment - so we don't display them in the outline. In short, we ignore those symbols whose assigning env arguments (e.g., pos or env) are provided.

I think this is good enough for most cases.

@shrektan
Copy link
Contributor Author

As makeActiveBinding() doesn't have a default env, I need to check the env agains a list of possible "top-level" candidates. Thus, the checks for assign() and delayedAssign() are added in the meantime.

Copy link
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@renkun-ken renkun-ken merged commit 70e50e1 into REditorSupport:master Jan 12, 2021
@shrektan shrektan deleted the promise-symbol branch January 12, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Display promise / active-binding objects in the OUTLINE
3 participants