Subsequent `import::from` into "imports" does not work

[hutch3232]

Setup: first.R

foo <- function(){
  message("hi")
}

second.R

bar <- function(){
  message("hello")
}

Using import 1.2.0:

import::from("first.R", .all = TRUE)
import::from("second.R", .all = TRUE)

exists("foo") && exists("bar")
# [1] TRUE

Using import 1.3.0:

import::from("first.R", .all = TRUE)
import::from("second.R", .all = TRUE)

exists("foo") && exists("bar")
# [1] FALSE

Walking through the code, I can see that bar exists (and lives in import:::scripts$second.R):

import:::scripts$second.R$bar()
# hello

I think use_into is incorrectly returning FALSE in this case. https://github.com/rticulate/import/blob/310bed17e1c42ab2c387bc010261297226764a12/R/from.R#L153-L154

Because use_into is FALSE, the symbol is being placed in pos = -1: https://github.com/rticulate/import/blob/310bed17e1c42ab2c387bc010261297226764a12/R/from.R#L248

I think a possible solution is to revise this line: https://github.com/rticulate/import/blob/310bed17e1c42ab2c387bc010261297226764a12/R/from.R#L150 with:

into_is_env <- tryCatch(expr = is.environment(as.environment(.into)),
                          error = function(cnd) return(FALSE))

This line tests if .into is coercible to an environment, and if it errors, return FALSE.

[hutch3232]

After looking at this a bit more, I think I've partially identified what change has caused this regression. Under import 1.2.0:

import::from("first.R", .all = TRUE)
# turn on debugger
import::from("second.R", .all = TRUE)
exists(".packageName", parent.frame(), inherits = TRUE)
# FALSE

use_into becomes TRUE Under import 1.3.0:

import::from("first.R", .all = TRUE)
# turn on debugger
import::from("second.R", .all = TRUE)
exists(".packageName", parent.frame(), inherits = TRUE)
# TRUE

use_into becomes FALSE

Digging into the changes, nothing stood out to me that would cause this, so I'm a little stumped. https://github.com/rticulate/import/compare/v1.2.0...v1.3.0

[hutch3232]

Actually I think I realized what's happening. It was the hidden objects change that I introduced: https://github.com/rticulate/import/pull/64 Under import 1.2.0:

import::from("first.R", .all = TRUE)
ls(envir=import:::scripts$first.R, all.names = TRUE)
# [1] ".packageName"      "__last_modified__" "foo"              
> ls(envir=as.environment("imports"), all.names = TRUE)
# [1] "?"   "foo"

Under import 1.3.0:

import::from("first.R", .all = TRUE)
ls(envir=import:::scripts$first.R, all.names = TRUE)
# [1] ".packageName"      "__last_modified__" "foo"              
> ls(envir=as.environment("imports"), all.names = TRUE)
# [1] ".packageName" "?"            "foo"    

The addition of all.names caused .packageName variable to be transferred over when it previously was not. Two possible questions:

1. Is the creation of .packageName important for something in the from_is_script section?
2. Or could we just exclude the transfer like is done with __last_modified__?

[hutch3232]

Opted to go for number (2.), excluding it during the transfer. I initially thought (1.) would be good, because I could not find any past discussion as to why the .packageName functionality was needed, however, I found taking it out broke the S3 methods tests. Option (2.) mimics the behavior of 1.2.0 anyways, the version before the regression.

[torfason]

Fixed in dev and main, will be released to CRAN shortly.