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

Upcoming fix in R devel parser requires action #419

Open
lorenzwalthert opened this Issue Aug 23, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@lorenzwalthert
Collaborator

lorenzwalthert commented Aug 23, 2018

> getParseData(parse(text = "t <- 1")) - current R
# current R 3.5.1
  line1 col1 line2 col2 id parent     token terminal text
1     1    1     1    1  1      3    SYMBOL     TRUE    t
3     1    1     1    1  3      0      expr    FALSE
2     1    2     1    2  2      0 EQ_ASSIGN     TRUE    =
4     1    3     1    3  4      5 NUM_CONST     TRUE    1
5     1    3     1    3  5      0      expr    FALSE

# R devel after committed by Tomas Kalibera
  line1 col1 line2 col2 id parent        token terminal text
8     1    1     1    3  8      0 equal_assign    FALSE <==== I added this
1     1    1     1    1  1      3       SYMBOL     TRUE    t
3     1    1     1    1  3      8         expr    FALSE
2     1    2     1    2  2      8    EQ_ASSIGN     TRUE    =
4     1    3     1    3  4      5    NUM_CONST     TRUE    1
5     1    3     1    3  5      8         expr    FALSE

Note the new token, which is not an expr and that all ids are changed. I was notified by the creator of the bug fix Tomas Kalibera as this will cause R CMD CHECK of styler (and reverse dependencies) to fail on R devel.

I see the following solution: Nest the parse data as is. Then, don't relocate the EQ_ASSIGN if a token equal_assign is present in a nest.

relocate_eq_assign <- function(pd) {
  if (any(pd$token == "equal_assign")) {
    pd
  } else {
    pd %>%
      post_visit(c(relocate_eq_assign_nest))
  }
}

@krlmlr what do you think should we do? Should we wait until the bug fix of the base R parser is implemented in R devel, fix the problem here and create a new release? I think I can't guarantee that this works and submit before the changes are implemented in R devel and I won't build R devel myself.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Aug 23, 2018

Thanks for looking into this. Can you send Tomas an R CMD build of the package with your proposed fix? He should be able to check if the tests pass with his patch.

We have r-devel on Travis, so we'll see there if it works. At some point we need to update on CRAN, but we might be able to wait until early next year.

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Aug 24, 2018

Ok. I suggest to just wait then and prepare a new release so we can submit to CRAN as soon as committed into devel.

@kalibera

This comment has been minimized.

kalibera commented Aug 24, 2018

Sure I can test an updated source package for you on my system. There may be some more changes you'd need to make if your code assumes that the node for equality assignment is expr. Also some tests may need adjustment. As I said earlier I can also give you my patch for R.

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Aug 25, 2018

Thanks Tomas. I'd like to use your patch but I am a bit afraid of building R devel from source myself if this is required at all. Is it? I run macOs, Linux and Windows.
You are right that more adjustments may be needed, but I think we hardly / never conditioned on whether a token is expr but instead used terminal for the operations that could be affected. Will try to check. Thanks for offering me to run my code, I will send you an email with the updated source package, taken from this branch. I just want to make sure this is not too time-consuming for you. I mocked GetParseData(parse(...)) with the code I posted above and it worked with the revision I am sending you, but obviously, this does not mean that R CMD CHECK will pass.

@lorenzwalthert

This comment has been minimized.

Collaborator

lorenzwalthert commented Aug 29, 2018

Decision: Wait until on R devel and fix it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment