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

fix: fixed an error when := assignment is used inside the code in qenv #233

Closed
wants to merge 1 commit into from

Conversation

kpagacz
Copy link
Contributor

@kpagacz kpagacz commented Nov 22, 2024

Here is an example illustrating the problem:

# MRE
# devtools::load_all("../../teal.code")
# devtools::load_all("../../teal.data")
library(teal)
library(teal.modules.general)
library(ggplot2)
library(data.table)

iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
code <- "
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
"
data <- teal.data::teal_data(iris = iris, code = code)
app <- teal::init(
  data = data,
  modules = teal::modules(
    teal.modules.general::tm_g_scatterplot(
      label = "Scatterplot",
      x = teal.transform::data_extract_spec(
        dataname = "iris",
        select = teal.transform::select_spec(
          choices = teal.transform::variable_choices("iris"),
          selected = "Sepal.Length"
        )
      ),
      y = teal.transform::data_extract_spec(
        dataname = "iris",
        select = teal.transform::select_spec(
          choices = teal.transform::variable_choices("iris"),
          selected = "Sepal.Width"
        )
      )
    )
  )
)
if (interactive()) {
  shiny::shinyApp(ui = app$ui, server = app$server)
}

This errors out with an informative:

Warning in sym_cond > ass_cond :
longer object length is not a multiple of shorter object length
Warning: Error in if: the condition has length > 1
5: runApp
4: print.shiny.appobj
2: base::source
1: nvimcom:::source.and.clean

And precludes launching the application.

After some investigation, I narrowed the issue to the fact that the custom := is unfortunately parsed by whatever teal.code uses to parse as LEFT_ASSIGNMENT token, which causes problems in these lines:

ass_cond <- grep("ASSIGN", x$token)
if (!length(ass_cond)) {
return(c("<-", unique(x[sym_cond, "text"])))
}
sym_cond <- sym_cond[sym_cond > ass_cond] # NOTE 1
# If there was an assignment operation detect direction of it.
if (unique(x$text[ass_cond]) == "->") { # NOTE 2
sym_cond <- rev(sym_cond)
}

What happens is that ass_cond (great naming convention, guys) is length > 1, which means that the if at the end errors. As a simple, dirty fix, I propose a Filter on such a custom := assignment operator, which cannot be used for any real assignments:

l := 1

This does not parse in R > 3 (and probably even more ancient), so it should be OK with the rest of the function's logic, which, I gathered, tries to establish parent-child relationships between symbols using the assignment operators.

Let me know if you want me to change something, scrap it or do something else entirely.

Copy link
Contributor

github-actions bot commented Nov 22, 2024

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@kpagacz
Copy link
Contributor Author

kpagacz commented Nov 22, 2024

I have read the CLA Document and I hereby sign the CLA

@m7pr
Copy link
Contributor

m7pr commented Nov 22, 2024

I think I'd like to write a test for this case and see what are the results. Will handle on Monday, thanks!

@m7pr m7pr self-requested a review November 25, 2024 16:52
@m7pr m7pr self-assigned this Nov 25, 2024
@kpagacz
Copy link
Contributor Author

kpagacz commented Nov 27, 2024

Hi, @m7pr how is this one going?

I am asking because we have quite a few of the scripts that could use solving the := issue because refactoring several thousands of lines of preprocessing code is no fun, and the workarounds leave much to be desired.

@m7pr
Copy link
Contributor

m7pr commented Nov 27, 2024

@kpagacz on it

@m7pr
Copy link
Contributor

m7pr commented Nov 27, 2024

Hey, narrowing it down to a simpler case:

code <- "
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
"
data <- teal.data::teal_data(iris = iris, code = code)

eval_code(teal_data(), code)

@m7pr
Copy link
Contributor

m7pr commented Nov 27, 2024

Hey @kpagacz thanks for reporting.
I created a separate PR that fixes this issue #234
I added changes to the code in different place and also added a test.

I need team's opinion on how we test this edge case as it needs data.table package, and that's a big big package to be added in Suggests of the package. Will keep you posted. Probably we can sort this out today

@m7pr
Copy link
Contributor

m7pr commented Nov 27, 2024

Ok, we changed the test to use := from rlang. 6d38745
We will merge soon today

@kpagacz
Copy link
Contributor Author

kpagacz commented Nov 27, 2024

I am closing this then.

@kpagacz kpagacz closed this Nov 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
m7pr added a commit that referenced this pull request Nov 27, 2024
Closes #233 and alternative for #233

This removed `:=` from extracted calls so that it is not treated as
`LEFT_ASSIGNMENT`.


**Current main** - check row 26

```r
devtools::load_all(".")
code <- "
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
"

code_split <- split_code(paste(code, collapse = "\n"))[[1]]
current_call <- parse(text = code_split, keep.source = TRUE)
pd <- normalize_pd(utils::getParseData(current_call))
reordered_pd <- extract_calls(pd)
reordered_pd[[1]]


   line1 col1 line2 col2 id parent                token terminal       text
46     2    1     3   36 46      0                 expr    FALSE           
5      2    1     2    4  5     46                 expr    FALSE           
4      2    6     2    7  4     46          LEFT_ASSIGN     TRUE         <-
45     2    9     3   36 45     46                 expr    FALSE           
3      2    1     2    4  3      5               SYMBOL     TRUE       iris
17     2    9     2   36 17     45                 expr    FALSE           
18     2   38     2   40 18     45              SPECIAL     TRUE        %>%
43     3    3     3   36 43     45                 expr    FALSE           
9      2    9     2   30  9     17                 expr    FALSE           
10     2   31     2   31 10     17                  '('     TRUE          (
13     2   32     2   35 13     17                 expr    FALSE           
12     2   36     2   36 12     17                  ')'     TRUE          )
6      2    9     2   18  6      9       SYMBOL_PACKAGE     TRUE data.table
7      2   19     2   20  7      9               NS_GET     TRUE         ::
8      2   21     2   30  8      9 SYMBOL_FUNCTION_CALL     TRUE data.table
11     2   32     2   35 11     13               SYMBOL     TRUE       iris
22     3    3     3    3 22     43                 expr    FALSE           
21     3    4     3    4 21     43                  '['     TRUE          [
23     3    5     3    5 23     43                  ','     TRUE          ,
39     3    7     3   35 39     43                 expr    FALSE           
38     3   36     3   36 38     43                  ']'     TRUE          ]
20     3    3     3    3 20     22               SYMBOL     TRUE          .
27     3    7     3   16 27     39                 expr    FALSE           
26     3   18     3   19 26     39          LEFT_ASSIGN     TRUE         :=
37     3   21     3   35 37     39                 expr    FALSE           
25     3    7     3   16 25     27               SYMBOL     TRUE NewSpecies
30     3   21     3   26 30     37                 expr    FALSE           
29     3   27     3   27 29     37                  '('     TRUE          (
33     3   28     3   34 33     37                 expr    FALSE           
32     3   35     3   35 32     37                  ')'     TRUE          )
28     3   21     3   26 28     30 SYMBOL_FUNCTION_CALL     TRUE     factor
31     3   28     3   34 31     33               SYMBOL     TRUE    Species
```

**Feature branch** - row removed

```r
   line1 col1 line2 col2 id parent                token terminal       text
46     2    1     3   36 46      0                 expr    FALSE           
5      2    1     2    4  5     46                 expr    FALSE           
4      2    6     2    7  4     46          LEFT_ASSIGN     TRUE         <-
45     2    9     3   36 45     46                 expr    FALSE           
3      2    1     2    4  3      5               SYMBOL     TRUE       iris
17     2    9     2   36 17     45                 expr    FALSE           
18     2   38     2   40 18     45              SPECIAL     TRUE        %>%
43     3    3     3   36 43     45                 expr    FALSE           
9      2    9     2   30  9     17                 expr    FALSE           
10     2   31     2   31 10     17                  '('     TRUE          (
13     2   32     2   35 13     17                 expr    FALSE           
12     2   36     2   36 12     17                  ')'     TRUE          )
6      2    9     2   18  6      9       SYMBOL_PACKAGE     TRUE data.table
7      2   19     2   20  7      9               NS_GET     TRUE         ::
8      2   21     2   30  8      9 SYMBOL_FUNCTION_CALL     TRUE data.table
11     2   32     2   35 11     13               SYMBOL     TRUE       iris
22     3    3     3    3 22     43                 expr    FALSE           
21     3    4     3    4 21     43                  '['     TRUE          [
23     3    5     3    5 23     43                  ','     TRUE          ,
39     3    7     3   35 39     43                 expr    FALSE           
38     3   36     3   36 38     43                  ']'     TRUE          ]
20     3    3     3    3 20     22               SYMBOL     TRUE          .
27     3    7     3   16 27     39                 expr    FALSE           
37     3   21     3   35 37     39                 expr    FALSE           
25     3    7     3   16 25     27               SYMBOL     TRUE NewSpecies
30     3   21     3   26 30     37                 expr    FALSE           
29     3   27     3   27 29     37                  '('     TRUE          (
33     3   28     3   34 33     37                 expr    FALSE           
32     3   35     3   35 32     37                  ')'     TRUE          )
28     3   21     3   26 28     30 SYMBOL_FUNCTION_CALL     TRUE     factor
31     3   28     3   34 31     33               SYMBOL     TRUE    Species
```


This lead to the fact that below can be executed without errors

```r
devtools::load_all(".")
code <- "
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
"

q <- eval_code(qenv(), code)
cat(get_code(q))
```

```r

iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
```

---------

Signed-off-by: Marcin <[email protected]>
Co-authored-by: André Veríssimo <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants