lintr icon indicating copy to clipboard operation
lintr copied to clipboard

lintr usage of `glue` failing on `lint_package()`

Open colearendt opened this issue 2 years ago • 2 comments

lint_package() is currently failing, as used here:

https://github.com/r-lib/actions/blob/v2-branch/examples/lint.yaml

The error message:

Run lintr::lint_package()
Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open,  : 
  Expecting '}'
Calls: <Anonymous> ... extract_glued_symbols -> eval -> eval -> <Anonymous> -> glue_data
Execution halted

Potentially related to some recent changes in glue (the .literal argument):

https://github.com/tidyverse/glue/issues/259#issuecomment-1035205742

colearendt avatar Jul 18 '22 12:07 colearendt

Hi, can you name the package that lint_package() fails on? Or, even better, try to provide a minimal reprex?

AshesITR avatar Jul 18 '22 13:07 AshesITR

Oops. Apologies for missing this. This is currently failing on https://github.com/rstudio/connectapi. I haven't had a chance to narrow it down to a reprex, which may be tricky without creating another package... which might be interesting as a reprex haha. But CI deployment failure is here, for example: https://github.com/rstudio/connectapi/runs/7453997000?check_suite_focus=true

colearendt avatar Aug 20 '22 01:08 colearendt

Can reproduce, thanks.

Backtrace:
     ▆
  1. └─lintr::lint_package("../connectapi/")
  2.   ├─base::do.call(...) at lintr/R/lint.R:263:2
  3.   └─lintr (local) `<fn>`(...)
  4.     ├─lintr:::flatten_lints(...) at lintr/R/lint.R:176:2
  5.     │ ├─base::structure(flatten_list(x, class = "lint"), class = "lints") at lintr/R/utils.R:18:2
  6.     │ └─lintr:::flatten_list(x, class = "lint") at lintr/R/utils.R:18:2
  7.     │   └─lintr (local) assign_item(x) at lintr/R/utils.R:37:2
  8.     └─base::lapply(...) at lintr/R/lint.R:176:2
  9.       └─lintr (local) FUN(X[[i]], ...)
 10.         ├─base::do.call(...) at lintr/R/lint.R:182:6
 11.         └─lintr (local) `<fn>`(...)
 12.           └─lintr:::get_lints(...) at lintr/R/lint.R:98:8
 13.             ├─lintr:::flatten_lints(linter_fun(expr)) at lintr/R/lint.R:302:4
 14.             │ ├─base::structure(flatten_list(x, class = "lint"), class = "lints") at lintr/R/utils.R:18:2
 15.             │ └─lintr:::flatten_list(x, class = "lint") at lintr/R/utils.R:18:2
 16.             │   └─lintr (local) assign_item(x) at lintr/R/utils.R:37:2
 17.             └─lintr (local) linter_fun(expr) at lintr/R/lint.R:302:4
 18.               └─base::lapply(...) at lintr/R/object_usage_linter.R:57:4
 19.                 └─lintr (local) FUN(X[[i]], ...)
 20.                   └─lintr:::get_used_symbols(fun_assignment, interpret_glue = interpret_glue) at lintr/R/object_usage_linter.R:70:6
 21.                     └─lintr:::extract_glued_symbols(expr) at lintr/R/object_usage_linter.R:310:2
 22.                       ├─base::eval(parsed_cl) at lintr/R/object_usage_linter.R:186:4
 23.                       │ └─base::eval(parsed_cl)
 24.                       └─glue::glue(...)
 25.                         └─glue::glue_data(...)

IndrajeetPatil avatar Oct 01 '22 04:10 IndrajeetPatil

This reveals that the problem is coming from remote.R:

purrr::map(fs::dir_ls("../connectapi/R/"), purrr::safely(lintr::lint))
$`../connectapi/R/remote.R`
$`../connectapi/R/remote.R`$result
NULL

$`../connectapi/R/remote.R`$error
<simpleError in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open,     .close = .close, .na = .na, .null = .null, .comment = .comment,     .literal = .literal, .transformer = .transformer, .trim = .trim): Expecting '}'>

IndrajeetPatil avatar Oct 01 '22 04:10 IndrajeetPatil

Okay, the problem is indeed caused by a missing } on this line:

https://github.com/rstudio/connectapi/blob/b0eb54f41d6a52bc859a18765c0bbfff617d60c8/R/remote.R#L91

The { in {glue::glue_collapse( is never closed. If I remove this line of code, the package lints successfully.

So this is not a {lintr} issue, but rather a bug in your code that needs to be fixed. Hope this helps.

IndrajeetPatil avatar Oct 01 '22 04:10 IndrajeetPatil

It doesn't look broken to me:

remote_users_res = list(list(username = 'a'), list(username = 'b'))
message(glue::glue("Users found: {glue::glue_collapse(purrr::map_chr(remote_users_res, ~ .x[['username']]), sep = \", \")}"))
# Users found: a, b

MichaelChirico avatar Oct 01 '22 07:10 MichaelChirico

https://github.com/rstudio/connectapi/blob/b0eb54f41d6a52bc859a18765c0bbfff617d60c8/R/remote.R#L91

message(glue::glue("Groups found: {glue::glue_collapse(purrr::map_chr(remote_groups$results, ~ .x$name), sep = \", \")"))

IndrajeetPatil avatar Oct 01 '22 07:10 IndrajeetPatil

Where is the closing } here?

IndrajeetPatil avatar Oct 01 '22 07:10 IndrajeetPatil

Oh sorry, I missed it on L91; thought I'd checked both. But L47 you mentioned doesn't have the error.

MichaelChirico avatar Oct 01 '22 07:10 MichaelChirico

It doesn't look broken to me:

remote_users_res = list(list(username = 'a'), list(username = 'b'))
message(glue::glue("Users found: {glue::glue_collapse(purrr::map_chr(remote_users_res, ~ .x[['username']]), sep = \", \")}"))
# Users found: a, b

With L91:

remote_users_res = list(list(username = 'a'), list(username = 'b'))
message(glue::glue("Groups found: {glue::glue_collapse(purrr::map_chr(remote_groups$results, ~ .x$name), sep = \", \")"))
#> Error in glue_data(.x = NULL, ..., .sep = .sep, .envir = .envir, .open = .open, : Expecting '}'

Created on 2022-10-01 with reprex v2.0.2

IndrajeetPatil avatar Oct 01 '22 07:10 IndrajeetPatil

But L47 you mentioned doesn't have the error.

Yeah, I thought they were duplicated lines of code, but they turned out to be slightly different.

IndrajeetPatil avatar Oct 01 '22 07:10 IndrajeetPatil

OK filed https://github.com/rstudio/connectapi/pull/174; re-closing this as invalid, though maybe we could improve the debugging experience here.

MichaelChirico avatar Oct 01 '22 07:10 MichaelChirico

maybe we could improve the debugging experience here

What can we do to improve it further?

I tried to do my best by walking through each debugging step and finally pointing to the offending piece of code and a successful lint run by removing it.

What could I change about this workflow?

IndrajeetPatil avatar Oct 01 '22 07:10 IndrajeetPatil

Package user runs lint_package() and gets an inscrutable error ("how did glue get involved?") with a 25-layer stack trace that really doesn't do much to differentiate "this is an error in my code" vs. "this is a lintr error". So not a great debugging experience.

MichaelChirico avatar Oct 01 '22 08:10 MichaelChirico

Oh, you mean we need to improve the message that {lintr} produces in such cases. Can't agree more.

I thought you were referring to the debugging methods I used to figure out the offending line of code wasn't great 😅

IndrajeetPatil avatar Oct 01 '22 08:10 IndrajeetPatil

oh not at all, sorry for the confusion :)

MichaelChirico avatar Oct 01 '22 08:10 MichaelChirico

I think what we should do is silently ignore "invalid" glue calls. An object_usage_linter wouldn't be expected to produce "invalid glue expression" as a lint, would it?

The only other option I see would be to treat it by emitting a warning, similar to how we deal with no-op #nolint comments.

I'd still regard this as a bug because linting shouldn't fail when a linted package contains a software bug.

AshesITR avatar Oct 01 '22 08:10 AshesITR

Hmm but the code itself produces the exact same error (referenced by Indrajeet above: https://github.com/r-lib/lintr/issues/1459#issuecomment-1264277979).

I have a PR going that will help more generally, let's start from there.

MichaelChirico avatar Oct 01 '22 08:10 MichaelChirico

Woops - entirely missed this thread somehow 🙈 Thanks for the help y'all!

colearendt avatar Nov 24 '22 01:11 colearendt