posit-dev / positron

Positron, a next-generation data science IDE
Other
1.89k stars 55 forks source link

Ark: Diagnostics: Populate `library()` symbols statically #2649

Open lionel- opened 3 months ago

lionel- commented 3 months ago

Currently we inspect the runtime state of the search path to determine if a symbol is in scope. This has all sorts of downsides, the biggest one being that scripts get decorated with distracting diagnostics until the user has evaluated all library calls:

Screenshot 2024-04-04 at 15 31 50

Instead of inspecting the current state of R, we should determine symbols in scope from lexical analysis. This way each line of code will get correct diagnostics independently from the state of the runtime. This will also allow to flag this as problematic even when the user has attached ggplot2:

ggplot(data) # <-- not in scope

library(ggplot2)

Part of https://github.com/posit-dev/positron/issues/2321

DavisVaughan commented 3 months ago

We do some static analysis already, like if you define a function in a file in the workspace then it will get picked up as a valid symbol even if you haven't sent it to the console yet. But we definitely don't do anything static for symbols that come from library() calls - and we will need a side loaded R session to do that anyways, to be able to load the package in a separate R session to get its symbols (i.e. something we've been talking about for awhile now)

lionel- commented 3 months ago

I think we don't need a side R session, instead we can look for the package in libpaths and inspect the NAMESPACE file.

kevinushey commented 3 months ago

For completeness, I think you really do need to load the package. Off the top of my head...

jennybc commented 3 months ago

This feels very related to #2252, maybe even a duplicate?

lionel- commented 3 months ago
  • the NAMESPACE file might use exportPattern(), or other export() entries that are more challenging to parse

Good point. Probably not a blocker though?

  • we wouldn't have a way to provide diagnostics for code using internal functions like ggplot2:::foobar()

This can be solved by analysing the R code statically once we have https://github.com/posit-dev/positron/issues/2286 or https://github.com/posit-dev/positron/issues/2285 (in this latter case we might need the side session unless we accept the package-loading side effect in the user session).

  • some packages might mutate the package:foo environment in .onAttach

For this we'll need to introduce a declare() syntax that packages can use to declare additional exports. I think it's crucial that this sort of things can be fully deduced from source code.

juliasilge commented 3 months ago

I believe our lack of this kind of diagnostic is what has come up in https://github.com/posit-dev/positron-beta/discussions/76, correct?

lionel- commented 3 months ago

It does look like it.