Skip to content

Commit

Permalink
feat: let nargo and LSP work well in the stdlib (#5969)
Browse files Browse the repository at this point in the history
# Description

## Problem

`nargo test` doesn't work in the standard library, and LSP gives lots of
(incorrect) errors when opening the standard library.

## Summary

The main issue is that when you run `nargo` against the standard
library, a copy of it is also loaded as the standard library. Then we
get duplicate definitions, etc.

I tried several things to solve this and one that worked and ended up
making a bit of sense is having a crate be the root and the standard
library at the same time.

Now with this PR we can run `nargo test`, LSP works well, and you can
even run tests directly from the editor, which could speed up the stdlib
development.

## Additional Context

1. 7 warnings still remain in the standard library (all unused
functions) but I'd like to solve that in a separate PR.
2. If this gets merged we could eventually change CI to compile nargo,
then run `nargo test`, instead of running stdlib tests as part of one
crate's tests.

## 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.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
asterite authored Sep 9, 2024
1 parent 45f4ae0 commit 8e8e97c
Show file tree
Hide file tree
Showing 15 changed files with 89 additions and 45 deletions.
22 changes: 12 additions & 10 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,23 +212,25 @@ fn add_debug_source_to_file_manager(file_manager: &mut FileManager) {

/// Adds the file from the file system at `Path` to the crate graph as a root file
///
/// Note: This methods adds the stdlib as a dependency to the crate.
/// This assumes that the stdlib has already been added to the file manager.
/// Note: If the stdlib dependency has not been added yet, it's added. Otherwise
/// this method assumes the root crate is the stdlib (useful for running tests
/// in the stdlib, getting LSP stuff for the stdlib, etc.).
pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId {
let path_to_std_lib_file = Path::new(STD_CRATE_NAME).join("lib.nr");
let std_file_id = context
.file_manager
.name_to_id(path_to_std_lib_file)
.expect("stdlib file id is expected to be present");
let std_crate_id = context.crate_graph.add_stdlib(std_file_id);
let std_file_id = context.file_manager.name_to_id(path_to_std_lib_file);
let std_crate_id = std_file_id.map(|std_file_id| context.crate_graph.add_stdlib(std_file_id));

let root_file_id = context.file_manager.name_to_id(file_name.to_path_buf()).unwrap_or_else(|| panic!("files are expected to be added to the FileManager before reaching the compiler file_path: {file_name:?}"));

let root_crate_id = context.crate_graph.add_crate_root(root_file_id);
if let Some(std_crate_id) = std_crate_id {
let root_crate_id = context.crate_graph.add_crate_root(root_file_id);

add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap());
add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap());

root_crate_id
root_crate_id
} else {
context.crate_graph.add_crate_root_and_stdlib(root_file_id)
}
}

pub fn link_to_debug_crate(context: &mut Context, root_crate_id: CrateId) {
Expand Down
38 changes: 35 additions & 3 deletions compiler/noirc_frontend/src/graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ pub enum CrateId {
Root(usize),
Crate(usize),
Stdlib(usize),
/// The special case of running the compiler against the stdlib.
/// In that case there's only one crate, and it's both the root
/// crate and the stdlib crate.
RootAndStdlib(usize),
Dummy,
}

Expand All @@ -25,11 +29,17 @@ impl CrateId {
}

pub fn is_stdlib(&self) -> bool {
matches!(self, CrateId::Stdlib(_))
match self {
CrateId::Stdlib(_) | CrateId::RootAndStdlib(_) => true,
CrateId::Root(_) | CrateId::Crate(_) | CrateId::Dummy => false,
}
}

pub fn is_root(&self) -> bool {
matches!(self, CrateId::Root(_))
match self {
CrateId::Root(_) | CrateId::RootAndStdlib(_) => true,
CrateId::Stdlib(_) | CrateId::Crate(_) | CrateId::Dummy => false,
}
}
}

Expand Down Expand Up @@ -178,7 +188,7 @@ impl CrateGraph {
Some((CrateId::Root(_), _)) => {
panic!("ICE: Tried to re-add the root crate as a regular crate")
}
Some((CrateId::Stdlib(_), _)) => {
Some((CrateId::Stdlib(_), _)) | Some((CrateId::RootAndStdlib(_), _)) => {
panic!("ICE: Tried to re-add the stdlib crate as a regular crate")
}
Some((CrateId::Dummy, _)) => {
Expand Down Expand Up @@ -212,6 +222,28 @@ impl CrateGraph {
crate_id
}

pub fn add_crate_root_and_stdlib(&mut self, file_id: FileId) -> CrateId {
for (crate_id, crate_data) in self.arena.iter() {
if crate_id.is_root() {
panic!("ICE: Cannot add two crate roots to a graph - use `add_crate` instead");
}

if crate_id.is_stdlib() {
panic!("ICE: Cannot add two stdlib crates to a graph - use `add_crate` instead");
}

if crate_data.root_file_id == file_id {
panic!("ICE: This FileId was already added to the CrateGraph")
}
}

let data = CrateData { root_file_id: file_id, dependencies: Vec::new() };
let crate_id = CrateId::RootAndStdlib(self.arena.len());
let prev = self.arena.insert(crate_id, data);
assert!(prev.is_none());
crate_id
}

pub fn iter_keys(&self) -> impl Iterator<Item = CrateId> + '_ {
self.arena.keys().copied()
}
Expand Down
2 changes: 1 addition & 1 deletion tooling/lsp/src/modules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ pub(crate) fn module_id_path(
if !is_relative {
// We don't record module attributes for the root module,
// so we handle that case separately
if let CrateId::Root(_) = target_module_id.krate {
if target_module_id.krate.is_root() {
segments.push("crate");
}
}
Expand Down
5 changes: 3 additions & 2 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use fm::{FileManager, FileMap};
use fxhash::FxHashMap as HashMap;
use lsp_types::{DiagnosticTag, Url};
use noirc_driver::{check_crate, file_manager_with_stdlib};
use noirc_driver::check_crate;
use noirc_errors::{DiagnosticKind, FileDiagnostic};

use crate::types::{
Expand Down Expand Up @@ -120,7 +120,8 @@ pub(crate) fn process_workspace_for_noir_document(
ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string())
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();

insert_all_files_for_workspace_into_file_manager(
state,
&workspace,
Expand Down
5 changes: 2 additions & 3 deletions tooling/lsp/src/requests/hover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use fm::FileMap;
use lsp_types::{Hover, HoverContents, HoverParams, MarkupContent, MarkupKind};
use noirc_frontend::{
ast::Visibility,
graph::CrateId,
hir::def_map::ModuleId,
hir_def::{stmt::HirPattern, traits::Trait},
macros_api::{NodeInterner, StructId},
Expand Down Expand Up @@ -376,9 +375,9 @@ fn format_parent_module_from_module_id(
}
}

// We don't record module attriubtes for the root module,
// We don't record module attributes for the root module,
// so we handle that case separately
if let CrateId::Root(_) = module.krate {
if module.krate.is_root() {
segments.push(&args.crate_name);
};

Expand Down
4 changes: 2 additions & 2 deletions tooling/lsp/src/requests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use lsp_types::{
WorkDoneProgressOptions,
};
use nargo_fmt::Config;
use noirc_driver::file_manager_with_stdlib;

use noirc_frontend::graph::CrateId;
use noirc_frontend::hir::def_map::CrateDefMap;
use noirc_frontend::{graph::Dependency, macros_api::NodeInterner};
Expand Down Expand Up @@ -432,7 +432,7 @@ where
ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find package for file")
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(
state,
&workspace,
Expand Down
6 changes: 2 additions & 4 deletions tooling/lsp/src/requests/profile_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ use async_lsp::{ErrorCode, ResponseError};
use nargo::ops::report_errors;
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_artifacts::debug::DebugArtifact;
use noirc_driver::{
file_manager_with_stdlib, CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_driver::{CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING};
use noirc_errors::{debug_info::OpCodesCount, Location};

use crate::{
Expand Down Expand Up @@ -53,7 +51,7 @@ fn on_profile_run_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(
state,
&workspace,
Expand Down
6 changes: 2 additions & 4 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ use crate::insert_all_files_for_workspace_into_file_manager;
use async_lsp::{ErrorCode, ResponseError};
use nargo::ops::{run_test, TestStatus};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING};
use noirc_frontend::hir::FunctionNameMatch;

use crate::{
Expand Down Expand Up @@ -48,7 +46,7 @@ fn on_test_run_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(
state,
&workspace,
Expand Down
4 changes: 2 additions & 2 deletions tooling/lsp/src/requests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::insert_all_files_for_workspace_into_file_manager;
use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use lsp_types::{LogMessageParams, MessageType};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{check_crate, file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING};
use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING};

use crate::{
get_package_tests_in_crate, parse_diff,
Expand Down Expand Up @@ -50,7 +50,7 @@ fn on_tests_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(
state,
&workspace,
Expand Down
18 changes: 18 additions & 0 deletions tooling/nargo/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ use std::{
slice,
};

use fm::FileManager;
use noirc_driver::file_manager_with_stdlib;

use crate::{
constants::{CONTRACT_DIR, EXPORT_DIR, PROOFS_DIR, TARGET_DIR},
package::Package,
Expand Down Expand Up @@ -46,6 +49,21 @@ impl Workspace {
pub fn export_directory_path(&self) -> PathBuf {
self.root_dir.join(EXPORT_DIR)
}

/// Returns a new `FileManager` for the root directory of this workspace.
/// If the root directory is not the standard library, the standard library
/// is added to the returned `FileManager`.
pub fn new_file_manager(&self) -> FileManager {
if self.is_stdlib() {
FileManager::new(&self.root_dir)
} else {
file_manager_with_stdlib(&self.root_dir)
}
}

fn is_stdlib(&self) -> bool {
self.members.len() == 1 && self.members[0].name.to_string() == "std"
}
}

pub enum IntoIter<'a, T> {
Expand Down
5 changes: 2 additions & 3 deletions tooling/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ use nargo::{
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_abi::{AbiParameter, AbiType, MAIN_RETURN_NAME};
use noirc_driver::{
check_crate, compute_function_abi, file_manager_with_stdlib, CompileOptions,
NOIR_ARTIFACT_VERSION_STRING,
check_crate, compute_function_abi, CompileOptions, NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_frontend::{
graph::{CrateId, CrateName},
Expand Down Expand Up @@ -52,7 +51,7 @@ pub(crate) fn run(args: CheckCommand, config: NargoConfig) -> Result<(), CliErro
Some(NOIR_ARTIFACT_VERSION_STRING.to_string()),
)?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

Expand Down
4 changes: 2 additions & 2 deletions tooling/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use nargo::package::Package;
use nargo::workspace::Workspace;
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::DEFAULT_EXPRESSION_WIDTH;
use noirc_driver::NOIR_ARTIFACT_VERSION_STRING;
use noirc_driver::{file_manager_with_stdlib, DEFAULT_EXPRESSION_WIDTH};
use noirc_driver::{CompilationResult, CompileOptions, CompiledContract};

use noirc_frontend::graph::CrateName;
Expand Down Expand Up @@ -114,7 +114,7 @@ pub(super) fn compile_workspace_full(
workspace: &Workspace,
compile_options: &CompileOptions,
) -> Result<(), CliError> {
let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

Expand Down
5 changes: 2 additions & 3 deletions tooling/nargo_cli/src/cli/export_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ use nargo::workspace::Workspace;
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
compile_no_check, file_manager_with_stdlib, CompileOptions, CompiledProgram,
NOIR_ARTIFACT_VERSION_STRING,
compile_no_check, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING,
};

use noirc_frontend::graph::CrateName;
Expand Down Expand Up @@ -54,7 +53,7 @@ pub(crate) fn run(args: ExportCommand, config: NargoConfig) -> Result<(), CliErr
Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()),
)?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

Expand Down
4 changes: 2 additions & 2 deletions tooling/nargo_cli/src/cli/fmt_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{fs::DirEntry, path::Path};
use clap::Args;
use nargo::{insert_all_files_for_workspace_into_file_manager, ops::report_errors};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING};
use noirc_driver::NOIR_ARTIFACT_VERSION_STRING;
use noirc_errors::CustomDiagnostic;
use noirc_frontend::{hir::def_map::parse_file, parser::ParserError};

Expand All @@ -29,7 +29,7 @@ pub(crate) fn run(args: FormatCommand, config: NargoConfig) -> Result<(), CliErr
Some(NOIR_ARTIFACT_VERSION_STRING.to_string()),
)?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);

let config = nargo_fmt::Config::read(&config.program_dir)
Expand Down
6 changes: 2 additions & 4 deletions tooling/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ use nargo::{
prepare_package,
};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING};
use noirc_frontend::{
graph::CrateName,
hir::{FunctionNameMatch, ParsedFiles},
Expand Down Expand Up @@ -65,7 +63,7 @@ pub(crate) fn run(args: TestCommand, config: NargoConfig) -> Result<(), CliError
Some(NOIR_ARTIFACT_VERSION_STRING.to_string()),
)?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
let mut workspace_file_manager = workspace.new_file_manager();
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

Expand Down

0 comments on commit 8e8e97c

Please sign in to comment.