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

fix(lsp): Ensure stdlib is always added before the check_crate phase #1840

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Jun 29, 2023

Description

Problem*

Resolves #1833

Summary*

This moves the addition of the stdlib to the dependency graph into Driver::check_crate, which is the point we know the root is configured and all other dependencies have been added to the driver. This ensures that no matter how or when you call driver.check_crate (e.g. nargo_cli, lsp, or noir_wasm), the stdlib will always be available.

This solution is sub-optimal because the stdlib should probably be the first crate added to the dependency graph and then it should be propagated to each new dep as it is added, but that seemed like a much larger refactor because a lot of the code in the compiler expects FileId(0) to be the crate root.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

In making this change I also discovered #1839

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench TomAFrench changed the title fix(lsp): Ensure stdlib is always added before the check_crate phase fix(lsp): Ensure stdlib is always added before the check_crate phase Jul 3, 2023
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Agreed that this is suboptimal but it's better than the current state where we need to remember to add it in externally.

@TomAFrench TomAFrench added this pull request to the merge queue Jul 4, 2023
Merged via the queue into master with commit cb607f5 Jul 4, 2023
@TomAFrench TomAFrench deleted the phated/lsp-stdlib branch July 4, 2023 12:40
@vezenovm vezenovm mentioned this pull request Jul 4, 2023
5 tasks
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.

LSP reports use dep::std as an error
2 participants