dgkf / R

An experimental reimagining of R
https://dgkf.github.io/R
GNU General Public License v3.0
136 stars 5 forks source link

`substitute()` fails with literals and other expressions #199

Closed sebffischer closed 1 month ago

sebffischer commented 1 month ago

Consider

substitute(1)
#> Internal Error
substitute("a")
#> Internal Error
substitute(fn(x) x)
#> Internal Error

The root cause of this is a combination of:

  1. Some arguments are currently evaluated eagerly, even in CallStack::eval_list_lazy: https://github.com/dgkf/R/blob/6c5e30465c6971d0f48f5086fc4a63d622cd519f/src/context/core.rs#L112-L118 I believe this should not be the case and we should still maintain them as expressions.
  2. substitute() expects its arguments to be promises: https://github.com/dgkf/R/blob/6c5e30465c6971d0f48f5086fc4a63d622cd519f/src/callable/primitive/substitute.rs#L58-L60

Solving this requires that CallStack::eval_list_lazy does not evaluate anything but stores the expressions as promises.

There is also a related problem, that promises contained in an Expr::Ellipsis(_) are currently not evaluated by CallStack::eval_list_eager. If one implements the above fix, this means that (fn(...) list(...))(1, 2) returns a list of promises instead of list(1, 2).

In princple there is also the question whether dgkf/R should behave like R with respect to literals. In R, we have quote(1) == 1 evaluating to TRUE. I tend to think that it is more consistent to distinguish between the expression 1 and the evaluated length one vector [1] 1.

More comments:

sebffischer commented 1 month ago

The problem here is that the Context::eval_list_lazy method which substitute uses, evaluates certain expressions:

https://github.com/dgkf/R/blob/e4ddc020d86d3c865561bef10060ba2255a7de11/src/context/core.rs#L113-L117

sebffischer commented 1 month ago

I also realized that R evaluates literals to their vectors even when quoting:

> quote(1)
[1] 1
> quote("a")
[1] "a"
> quote(c("a", "b"))
c("a", "b")
> 

I think it would be more consistent if literals were not evaluated by quote() and substitute() (it makes sense that substitute(1) evaluates to c(1) but for quote() I think it's inconsistent).

sebffischer commented 1 month ago

In principle I think this can be solved by not evaluating the arguments in Context::eval_list_lazy()

sebffischer commented 1 month ago

Fixed by https://github.com/dgkf/R/commit/5c13c4950991c821aa5fd2255751d3eabc3316a1