Skip to content

Add symbols imported in packages to workspace #872

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

Open
wants to merge 6 commits into
base: feature/static-imports
Choose a base branch
from

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Jul 15, 2025

Addresses posit-dev/positron#2252.
Addresses posit-dev/positron#8549.
Addresses posit-dev/positron#8550.
Progress towards posit-dev/positron#2321.

Branched from #870. It was rather easy to implement based on the infrastructure provided in that PR.

This fixes diagnostics for imported symbols but I was still seeing some weirdness with local definitions because we didn't synchronise the indexer and the diagnostics properly:

// FIXME: The initial indexer is currently racing against our state notification
// handlers. The indexer is synchronised through a mutex but we might end up in
// a weird state. Eventually the index should be moved to WorldState and created
// on demand with Salsa instrumenting and cancellation.

This is now fixed. I've also made a change to take into account objects assigned globally. We were detecting global functions but not other kinds of objects.

I've hacked in testthat imports inside testthat/ files. Should be good enough for now. Will fail when people edit their testthat.R file with additional library loading.

QA Notes

You should now be able to open a package like ellmer and not see diagnostics. This won't be 100% proof for all packages, but I've checked with rlang and ellmer.

See also posit-dev/positron#8549 and posit-dev/positron#8550 for reprexes for adjacent fixes.

@lionel- lionel- marked this pull request as draft July 15, 2025 14:41
@lionel- lionel- marked this pull request as ready for review July 16, 2025 14:27
}
}

async fn process_diagnostics_batch(batch: Vec<RefreshDiagnosticsTask>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconsider batching? Based on zoom call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it back, the interleaved indexer/diagnostics processing will arise only if the queue tasks are processed faster than they arrive so the current setup is fine. If we get a bunch of tasks very rapidly, we will collect them, split them by type, and process the indexer tasks first. So the batching is still useful.

This setup will be entirely replaced by Salsa dependencies. Diagnostics tasks will be cancelled automatically as document updates arrive. So we shouldn't worry about this temporary setup too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adapted the loop so that we check back for more indexer tasks once we have finished a round of indexing. We do that at most 10 times so diagnostics get refreshed once in a while if the user is writing too fast to keep up. I think I'm happy with the queue setup now!

@lionel-
Copy link
Contributor Author

lionel- commented Jul 17, 2025

@DavisVaughan I'm out of the day but this should be ready for review. When I come back I'll add some diagnostics tests for packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants