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

LSP features don't work across projects in a workspace #5460

Open
asterite opened this issue Jul 9, 2024 · 1 comment · Fixed by #5461
Open

LSP features don't work across projects in a workspace #5460

asterite opened this issue Jul 9, 2024 · 1 comment · Fixed by #5461
Assignees
Labels
bug Something isn't working

Comments

@asterite
Copy link
Collaborator

asterite commented Jul 9, 2024

Aim

When doing "find all references" or "rename", the returned occurrences are only those in the project where the command was issued, and that project dependencies. However, "find all references" should always return the same results regardless of where the command was triggered.

Expected Behavior

For example, if we have two projects, "one" and "two" in a workspace:

- one
  - src
    - lib.nr
  - Nargo.toml
- two
  - src
    - lib.nr
  - Nargo.toml
- Nargo.toml

where two depends on one, and inside the root Nargo.toml both one and two are workspace members, and lib.nr have these contents:

# one/src/lib.rs
fn function_one() {}
# two/src/lib.nr
use one::function_one;

If we trigger "find all references" from "fn function_one" then we just get that function as a result. If we trigger it from "use one::function_one" we get both results (because "two" knows about "one", but "one" doesn't know about "two").

Ideally triggering "find all references" from any place always gives the same results.

Bug

Results vary depending on when the action was executed.

To Reproduce

No response

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@asterite asterite added the bug Something isn't working label Jul 9, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 9, 2024
@asterite asterite self-assigned this Jul 9, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 11, 2024
# Description

## Problem

Resolves #5460

## Summary

Sorry for the many commits, the PR was stacked on top of other PRs.
Here's the first relevant commit:
[link](2d4d013)

Before this PR, LSP "find all references" and "rename" worked like this:
1. Find what's in the cursor location. That could either be a
definition, or it could be a reference to a definition (or, well,
nothing)
2. If it's a reference, go to the definition (otherwise keep the
definition)
3. From the definition, find all references to that definition.

That works well, but that always happened in a single Package. So, if
you ran this in a Package A, but there's Package B that depends on
Package B, references in B weren't returned (they can't be seen from A!)

To solve this, a few things were done here:
1. First, the old LSP code always used the `Nargo.toml` file nearest to
a file. That means that it would never find workspace. Now we continue
searching a `Nargo.toml` upwards even if we find one, up until the LSP
root directory, and keep the last one (if we find anothe one it's likely
a workspace `Nargo.toml`). With this change it means that when opening
any file in a package, if that package is included in a workspace (and
that workspace is in the editor) we actually invoke "nargo check" on
each package in the workspace, not just in the package where the file
is. That's the first step in actually tracking everything in the
workspace.
2. Second, when searching for references, we search for references in
all packages. To do this we first find a definition in the current
package, then search references to that definition in all other
packages.

I believe that with this, the LSP features are now much more usable!
Searching a type, a struct member, an alias, a global, etc., anywhere,
will always yield the same results. This is useful, and necessary, if,
say, we want to rename a type or a member that's used across the
workspace: changing that only in some packages wouldn't be (that)
useful.

Here it is in action, using https://github.com/vlayer-xyz/monorepo to
try it out:


https://github.com/noir-lang/noir/assets/209371/015b6baf-2e44-4f17-a651-361a2ee93dd5

Here `StorageWithinBlock` is defined in a `lib` project which has no
dependencies to other packages in the workspace. However, other packages
depend on it, for example `get_storage`, and those references are found.

Note: the first commit is a refactor so it's easier to see where
references to specific things are tracked. With just `add_reference` it
was kind of hard to debug when something didn't work well for a specific
thing (for example globals). It also removes some redundancy because in
some cases the `is_self_type` boolean doesn't make sense.

## Additional Context

None.

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 11, 2024
@asterite asterite reopened this Aug 1, 2024
@asterite
Copy link
Collaborator Author

asterite commented Aug 1, 2024

Reopening because we are going to revert a change that made this work because it ended up being very slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant