From 39794e77ca530e5451e9ee6152388731c79600c3 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:25:40 +0000 Subject: [PATCH 1/2] feat: add `nargo check --remove-unused-imports` to prune unused imports Adds a compiler-level rewrite that removes all imports reported unused by the elaborator in one go, including complex composite forms: pruning individual entries from bracketed lists (`use foo::{bar, baz}` -> `use foo::bar`), collapsing nested lists, handling aliases and `self` imports, and deleting fully-unused `use` items along with their line. The core lives in `noirc_frontend::remove_unused_imports` and matches imports by the same `(Ident, Location)` key the usage tracker records. The LSP's "Remove unused import" quick fix now reuses this shared core (gaining `self`-import support) instead of its own tree-pruning copy. `nargo check --remove-unused-imports` rewrites only files belonging to the package's own crate, and warnings for the imports it just removed are filtered out of the report. Removal is deliberately a single round, not a fixpoint: an import only consumed by another `use` statement's path is revealed unused only after re-elaboration, so a second run may remove more (documented by a dedicated test). --- compiler/noirc_frontend/src/lib.rs | 1 + .../src/remove_unused_imports.rs | 268 +++++++++++ compiler/noirc_frontend/src/tests.rs | 1 + .../src/tests/remove_unused_imports.rs | 431 ++++++++++++++++++ cspell.json | 1 + .../code_action/remove_unused_import.rs | 163 +++---- tooling/nargo_cli/src/cli/check_cmd.rs | 161 ++++++- 7 files changed, 918 insertions(+), 108 deletions(-) create mode 100644 compiler/noirc_frontend/src/remove_unused_imports.rs create mode 100644 compiler/noirc_frontend/src/tests/remove_unused_imports.rs diff --git a/compiler/noirc_frontend/src/lib.rs b/compiler/noirc_frontend/src/lib.rs index 38a180b50f6..60c3d1e23d6 100644 --- a/compiler/noirc_frontend/src/lib.rs +++ b/compiler/noirc_frontend/src/lib.rs @@ -24,6 +24,7 @@ pub mod monomorphization; pub mod node_interner; pub mod ownership; pub mod parser; +pub mod remove_unused_imports; pub mod resolve_locations; pub mod shared; pub mod usage_tracker; diff --git a/compiler/noirc_frontend/src/remove_unused_imports.rs b/compiler/noirc_frontend/src/remove_unused_imports.rs new file mode 100644 index 00000000000..2d5209b9cab --- /dev/null +++ b/compiler/noirc_frontend/src/remove_unused_imports.rs @@ -0,0 +1,268 @@ +//! Rewrites source code to remove imports that the elaborator determined are unused. +//! +//! The entry point is [remove_unused_imports], which takes a file's source text, its parsed +//! AST and the set of unused import names (as reported by +//! [UsageTracker::unused_imports][crate::usage_tracker::UsageTracker::unused_imports]) and +//! returns the source with those imports pruned. Composite `use` trees are rewritten rather +//! than deleted: `use foo::{bar, baz};` with `baz` unused becomes `use foo::bar;`, and a +//! `use` whose imports are all unused is deleted entirely (along with its line, when the +//! line contains nothing else). +//! +//! Unused imports are identified by the imported name and its exact source location — the +//! same `(Ident, Location)` key the usage tracker records — so two imports of the same name +//! never alias each other. +//! +//! Pruned `use` trees are re-rendered with [UseTree]'s `Display` impl, which produces a +//! single-line canonical form. Callers that want the result to respect a formatting style +//! should run the formatter afterwards. +//! +//! Removal is deliberately a single round, not a fixpoint: it removes exactly the imports +//! the elaborator reported for the given compilation. An import whose only consumer is +//! another `use` statement's path (resolving a path marks its first segment as used) is only +//! reported unused — and thus only removed — after the consuming `use` has been removed and +//! the program re-elaborated. + +use std::collections::HashSet; +use std::ops::Range; + +use noirc_errors::Location; + +use crate::ast::{Ident, ItemVisibility, Path, UseTree, UseTreeKind}; +use crate::parser::{Item, ItemKind, ParsedModule}; + +/// Returns `source` with all imports in `unused_imports` removed, or `None` if there was +/// nothing to remove. `parsed_module` must be the result of parsing `source`. +pub fn remove_unused_imports( + source: &str, + parsed_module: &ParsedModule, + unused_imports: &HashSet<(Ident, Location)>, +) -> Option { + if unused_imports.is_empty() { + return None; + } + + let is_unused = |ident: &Ident| unused_imports.contains(&(ident.clone(), ident.location())); + + let mut replacements = Vec::new(); + let mut deletions = Vec::new(); + collect_edits(&parsed_module.items, &is_unused, &mut replacements, &mut deletions); + if replacements.is_empty() && deletions.is_empty() { + return None; + } + + // Deleted `use` items take their whole line with them. Runs of adjacent deleted lines are + // merged so the blank-line handling in `swallow_extra_blank_line` sees the run as a unit. + deletions = deletions + .into_iter() + .map(|range| extend_range_over_line(source, range)) + .collect::>(); + deletions.sort_by_key(|range| range.start); + let mut merged_deletions: Vec> = Vec::new(); + for deletion in deletions { + match merged_deletions.last_mut() { + Some(last) if deletion.start <= last.end => last.end = last.end.max(deletion.end), + _ => merged_deletions.push(deletion), + } + } + + let mut edits: Vec<(Range, String)> = merged_deletions + .into_iter() + .map(|range| (swallow_extra_blank_line(source, range), String::new())) + .chain(replacements) + .collect(); + + // Apply the edits back to front so earlier edits' byte offsets stay valid. + edits.sort_by_key(|(range, _)| range.start); + let mut new_source = source.to_string(); + for (range, replacement) in edits.into_iter().rev() { + new_source.replace_range(range, &replacement); + } + Some(new_source) +} + +/// Walks `items` (recursing into inline submodules) and records one edit per `use` item that +/// contains at least one unused import: a replacement when some imports remain, a deletion of +/// the item's span when none do. +fn collect_edits( + items: &[Item], + is_unused: &dyn Fn(&Ident) -> bool, + replacements: &mut Vec<(Range, String)>, + deletions: &mut Vec>, +) { + for item in items { + match &item.kind { + ItemKind::Import(use_tree, visibility) => { + let (new_use_tree, removed_count) = + use_tree_without_unused_imports(use_tree, is_unused); + if removed_count == 0 { + continue; + } + + let span = item.location.span; + let range = span.start() as usize..span.end() as usize; + match new_use_tree { + Some(use_tree) => { + let replacement = if *visibility == ItemVisibility::Private { + format!("use {use_tree};") + } else { + format!("{visibility} use {use_tree};") + }; + replacements.push((range, replacement)); + } + None => deletions.push(range), + } + } + ItemKind::Submodules(submodule) => { + collect_edits(&submodule.contents.items, is_unused, replacements, deletions); + } + _ => (), + } + } +} + +/// Returns a copy of `use_tree` with all unused imports removed, along with the number of +/// removed imports. Returns `None` for the tree if every import in it is unused. +/// +/// `is_unused` is called with the [Ident] that the import binds: the alias if there is one, +/// the last path segment otherwise, and the segment before `self` for `self` imports — the +/// same ident whose location the usage tracker records. +pub fn use_tree_without_unused_imports( + use_tree: &UseTree, + is_unused: &dyn Fn(&Ident) -> bool, +) -> (Option, usize) { + use_tree_without_unused_imports_impl(use_tree, None, is_unused) +} + +fn use_tree_without_unused_imports_impl( + use_tree: &UseTree, + parent_prefix_last_ident: Option<&Ident>, + is_unused: &dyn Fn(&Ident) -> bool, +) -> (Option, usize) { + // The name a `self` import binds is the last segment of the accumulated prefix: for + // `use foo::{self, ...}` the leaf's own prefix is empty and the name is the outer `foo`. + let prefix_last_ident = + use_tree.prefix.segments.last().map(|segment| &segment.ident).or(parent_prefix_last_ident); + + match &use_tree.kind { + UseTreeKind::Path(name, alias) => { + let binding = alias.as_ref().or(if name.as_str() == "self" { + prefix_last_ident + } else { + Some(name) + }); + if binding.is_some_and(is_unused) { (None, 1) } else { (Some(use_tree.clone()), 0) } + } + UseTreeKind::List(use_trees) => { + let mut new_use_trees: Vec = Vec::new(); + let mut removed_count = 0; + + for use_tree in use_trees { + let (new_use_tree, count) = + use_tree_without_unused_imports_impl(use_tree, prefix_last_ident, is_unused); + if let Some(new_use_tree) = new_use_tree { + new_use_trees.push(new_use_tree); + } + removed_count += count; + } + + let new_use_tree = if new_use_trees.is_empty() { + None + } else if new_use_trees.len() == 1 { + Some(merge_prefix_into_tree( + &use_tree.prefix, + new_use_trees.remove(0), + use_tree.location, + )) + } else { + Some(UseTree { + prefix: use_tree.prefix.clone(), + kind: UseTreeKind::List(new_use_trees), + location: use_tree.location, + }) + }; + + (new_use_tree, removed_count) + } + } +} + +/// Unwraps the braces around a list that was left with a single entry, turning +/// `foo::{bar::baz}` into `foo::bar::baz`. +fn merge_prefix_into_tree(prefix: &Path, use_tree: UseTree, location: Location) -> UseTree { + let mut prefix = prefix.clone(); + prefix.segments.extend(use_tree.prefix.segments); + + // `self` is only valid inside a bracketed list, so `foo::{self}` must become `foo` + // (and `foo::{self as f}` must become `foo as f`), not `foo::self`. + if let UseTreeKind::Path(name, alias) = &use_tree.kind + && name.as_str() == "self" + && let Some(last_segment) = prefix.segments.pop() + { + return UseTree { + prefix, + kind: UseTreeKind::Path(last_segment.ident, alias.clone()), + location, + }; + } + + UseTree { prefix, kind: use_tree.kind, location } +} + +/// Given the byte range of a `use` item that is going to be deleted, extends the range over +/// the item's whole line (indentation and line terminator included) so no blank line is left +/// behind. Returns the range unchanged if the line contains anything else. +fn extend_range_over_line(source: &str, range: Range) -> Range { + let bytes = source.as_bytes(); + + // Walk backwards over the line's indentation; bail out if the item doesn't start the line. + let mut start = range.start; + while start > 0 && matches!(bytes[start - 1], b' ' | b'\t') { + start -= 1; + } + if start != 0 && bytes[start - 1] != b'\n' { + return range; + } + + // Walk forwards over trailing whitespace and the line terminator; bail out if the line + // has other content after the item. + let mut end = range.end; + while end < bytes.len() && matches!(bytes[end], b' ' | b'\t') { + end += 1; + } + if end < bytes.len() && bytes[end] == b'\r' { + end += 1; + } + if end < bytes.len() { + if bytes[end] != b'\n' { + return range; + } + end += 1; + } + + start..end +} + +/// Deleting a run of whole lines that sat between two blank lines (or between the start of +/// the file and a blank line) would leave a doubled-up blank line behind. When that is the +/// case, extends the range over the following blank line so a single blank line remains. +fn swallow_extra_blank_line(source: &str, range: Range) -> Range { + let Range { start, mut end } = range; + let bytes = source.as_bytes(); + + let previous_line_is_blank = + start == 0 || start == 1 || (start >= 2 && bytes[start - 2] == b'\n'); + if previous_line_is_blank { + let mut blank_end = end; + while blank_end < bytes.len() && matches!(bytes[blank_end], b' ' | b'\t') { + blank_end += 1; + } + if blank_end < bytes.len() && bytes[blank_end] == b'\r' { + blank_end += 1; + } + if blank_end < bytes.len() && bytes[blank_end] == b'\n' { + end = blank_end + 1; + } + } + + start..end +} diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index e3f17fe16da..0e99545c8e5 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -22,6 +22,7 @@ mod name_shadowing; mod numeric_generics; mod oracles; mod references; +mod remove_unused_imports; mod runtime; mod structs; mod traits; diff --git a/compiler/noirc_frontend/src/tests/remove_unused_imports.rs b/compiler/noirc_frontend/src/tests/remove_unused_imports.rs new file mode 100644 index 00000000000..03505b323ea --- /dev/null +++ b/compiler/noirc_frontend/src/tests/remove_unused_imports.rs @@ -0,0 +1,431 @@ +//! Tests for [crate::remove_unused_imports], which rewrites source code to prune imports +//! that the elaborator determined are unused. + +use std::collections::HashSet; + +use noirc_errors::Location; + +use crate::ast::Ident; +use crate::remove_unused_imports::remove_unused_imports; +use crate::test_utils::get_program; + +/// Compiles `src`, collects every unused import from the usage tracker, and returns `src` +/// rewritten with those imports removed. Returns `None` if there was nothing to remove. +fn source_without_unused_imports(src: &str) -> Option { + let (parsed_module, context, _errors) = get_program(src); + let unused_imports: HashSet<(Ident, Location)> = context + .usage_tracker + .unused_imports() + .values() + .flat_map(|imports| imports.keys().cloned()) + .collect(); + remove_unused_imports(src, &parsed_module, &unused_imports) +} + +#[test] +fn removes_unused_imports_from_composite_bracketed_use() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} + pub mod baz { + pub fn qux() {} + pub fn corge() {} + } +} + +use foo::{bar, spam, baz::{qux, corge}}; + +fn main() { + bar(); + qux(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + pub mod baz { + pub fn qux() {} + pub fn corge() {} + } + } + + use foo::{bar, baz::qux}; + + fn main() { + bar(); + qux(); + } + "); +} + +#[test] +fn collapses_bracketed_list_with_single_remaining_import() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +use foo::{bar, spam}; + +fn main() { + bar(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + use foo::bar; + + fn main() { + bar(); + } + "); +} + +#[test] +fn collapses_nested_bracketed_lists() { + let src = r#"mod foo { + pub mod bar { + pub fn qux() {} + pub fn corge() {} + } +} + +use foo::{bar::{qux, corge}}; + +fn main() { + corge(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub mod bar { + pub fn qux() {} + pub fn corge() {} + } + } + + use foo::bar::corge; + + fn main() { + corge(); + } + "); +} + +#[test] +fn removes_entire_use_item_when_all_imports_unused() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +use foo::{bar, spam}; + +fn main() {} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + fn main() {} + "); +} + +#[test] +fn removes_unused_aliased_import() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +use foo::{bar as b, spam as s}; + +fn main() { + b(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + use foo::bar as b; + + fn main() { + b(); + } + "); +} + +#[test] +fn removes_unused_self_import_from_list() { + let src = r#"mod foo { + pub fn bar() {} +} + +use foo::{self, bar}; + +fn main() { + bar(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + } + + use foo::bar; + + fn main() { + bar(); + } + "); +} + +#[test] +fn removes_unused_imports_in_nested_modules_in_one_go() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +mod qux { + use super::foo::{bar, spam}; + + pub fn corge() { + bar(); + } +} + +use foo::spam; + +fn main() { + qux::corge(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + mod qux { + use super::foo::bar; + + pub fn corge() { + bar(); + } + } + + fn main() { + qux::corge(); + } + "); +} + +#[test] +fn removes_adjacent_fully_unused_use_items_without_leaving_blank_lines() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +use foo::bar; +use foo::spam; + +fn main() {} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + fn main() {} + "); +} + +#[test] +fn collapsing_to_self_import_drops_the_self_segment() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +use foo::{self, spam}; + +fn main() { + foo::bar(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + use foo; + + fn main() { + foo::bar(); + } + "); +} + +#[test] +fn rewrites_multi_line_use_item() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} + pub fn qux() {} +} + +use foo::{ + bar, + spam, + qux, +}; + +fn main() { + bar(); + qux(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + pub fn qux() {} + } + + use foo::{bar, qux}; + + fn main() { + bar(); + qux(); + } + "); +} + +/// Removal is a single round of a fixpoint, not the fixpoint itself. Resolving an import's +/// path marks its first segment as used, so an import whose only consumer is another `use` +/// statement is *not* reported unused until that other `use` has been removed and the +/// program re-elaborated. Each round therefore removes one level of such a cascade. +#[test] +fn import_used_only_by_a_removed_import_is_only_removed_on_a_second_run() { + let src = r#"mod foo { + pub mod bar { + pub fn qux() {} + } +} + +use foo::bar; +use bar::qux; + +fn main() {} +"#; + // The first round only removes `use bar::qux;`: at this point `use foo::bar;` is + // considered used, because resolving the path `bar::qux` referenced it. + let after_first_run = + source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(after_first_run, @r" + mod foo { + pub mod bar { + pub fn qux() {} + } + } + + use foo::bar; + + fn main() {} + "); + + // Re-elaborating the pruned source reveals that `use foo::bar;` is now unused too. + let after_second_run = + source_without_unused_imports(&after_first_run).expect("expected imports to be removed"); + insta::assert_snapshot!(after_second_run, @r" + mod foo { + pub mod bar { + pub fn qux() {} + } + } + + fn main() {} + "); + + // The fixpoint is reached: a third round has nothing left to remove. + assert_eq!(source_without_unused_imports(&after_second_run), None); +} + +#[test] +fn returns_none_when_there_are_no_unused_imports() { + let src = r#"mod foo { + pub fn bar() {} +} + +use foo::bar; + +fn main() { + bar(); +} +"#; + assert_eq!(source_without_unused_imports(src), None); +} + +#[test] +fn preserves_visibility_when_rewriting_use_item() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +mod qux { + pub(crate) use super::foo::{bar, spam}; + + pub fn corge() { + spam(); + } +} + +fn main() { + qux::corge(); +} +"#; + let result = source_without_unused_imports(src).expect("expected imports to be removed"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + mod qux { + pub(crate) use super::foo::spam; + + pub fn corge() { + spam(); + } + } + + fn main() { + qux::corge(); + } + "); +} diff --git a/cspell.json b/cspell.json index b36fb839691..2f900531bcf 100644 --- a/cspell.json +++ b/cspell.json @@ -80,6 +80,7 @@ "concat", "cond", "constrainedness", + "corge", "cpus", "cranelift", "critesjosh", diff --git a/tooling/lsp/src/requests/code_action/remove_unused_import.rs b/tooling/lsp/src/requests/code_action/remove_unused_import.rs index d811e7f45f4..996085c7b29 100644 --- a/tooling/lsp/src/requests/code_action/remove_unused_import.rs +++ b/tooling/lsp/src/requests/code_action/remove_unused_import.rs @@ -1,13 +1,11 @@ -use std::collections::{HashMap, HashSet}; - use async_lsp::lsp_types::TextEdit; use fm::FileId; use noirc_errors::{Location, Span}; use noirc_frontend::{ ParsedModule, - ast::{Ident, ItemVisibility, UseTree, UseTreeKind}, - hir::def_map::Namespace, + ast::{Ident, ItemVisibility, UseTree}, parser::{Item, ItemKind}, + remove_unused_imports::use_tree_without_unused_imports, }; use crate::byte_span_to_range; @@ -33,106 +31,42 @@ impl CodeActionFinder<'_> { return; } - if has_unused_import(use_tree, unused_imports) { - let byte_span = span.start() as usize..span.end() as usize; - let Some(range) = byte_span_to_range(self.files, self.file, byte_span) else { - return; - }; - - let (use_tree, removed_count) = - use_tree_without_unused_import(use_tree, unused_imports); - let (title, new_text) = match use_tree { - Some(use_tree) => ( - if removed_count == 1 { - "Remove unused import".to_string() - } else { - "Remove unused imports".to_string() - }, - use_tree_to_string(use_tree, visibility, self.nesting), - ), - None => ("Remove the whole `use` item".to_string(), "".to_string()), - }; - - let text_edit = TextEdit { range, new_text }; - self.unused_imports_text_edits.push(text_edit.clone()); - - let code_action = self.new_quick_fix(title, text_edit); - self.code_actions.push(code_action); + // The map is keyed by `(name, location)` because two distinct `use`s can import the same + // name (into different namespaces), so the location is matched too — otherwise a used + // import would be reported unused just for sharing a name with an unused one. + let is_unused = |ident: &Ident| { + unused_imports + .keys() + .any(|(name, location)| name == ident && *location == ident.location()) + }; + + let (use_tree, removed_count) = use_tree_without_unused_imports(use_tree, &is_unused); + if removed_count == 0 { + return; } - } -} -/// Whether `ident` names an unused import. The map is keyed by `(name, location)` because two -/// distinct `use`s can import the same name (into different namespaces), so the location is matched -/// too — otherwise a used import would be reported unused just for sharing a name with an unused one. -fn is_unused_import( - ident: &Ident, - unused_imports: &HashMap<(Ident, Location), HashSet>, -) -> bool { - unused_imports.keys().any(|(name, location)| name == ident && *location == ident.location()) -} + let byte_span = span.start() as usize..span.end() as usize; + let Some(range) = byte_span_to_range(self.files, self.file, byte_span) else { + return; + }; -fn has_unused_import( - use_tree: &UseTree, - unused_imports: &HashMap<(Ident, Location), HashSet>, -) -> bool { - match &use_tree.kind { - UseTreeKind::Path(name, alias) => { - let ident = alias.as_ref().unwrap_or(name); - is_unused_import(ident, unused_imports) - } - UseTreeKind::List(use_trees) => { - use_trees.iter().any(|use_tree| has_unused_import(use_tree, unused_imports)) - } - } -} + let (title, new_text) = match use_tree { + Some(use_tree) => ( + if removed_count == 1 { + "Remove unused import".to_string() + } else { + "Remove unused imports".to_string() + }, + use_tree_to_string(use_tree, visibility, self.nesting), + ), + None => ("Remove the whole `use` item".to_string(), "".to_string()), + }; -/// Returns a new `UseTree` with all the unused imports removed, and the number of removed imports. -fn use_tree_without_unused_import( - use_tree: &UseTree, - unused_imports: &HashMap<(Ident, Location), HashSet>, -) -> (Option, usize) { - match &use_tree.kind { - UseTreeKind::Path(name, alias) => { - let ident = alias.as_ref().unwrap_or(name); - if is_unused_import(ident, unused_imports) { - (None, 1) - } else { - (Some(use_tree.clone()), 0) - } - } - UseTreeKind::List(use_trees) => { - let mut new_use_trees: Vec = Vec::new(); - let mut total_count = 0; - - for use_tree in use_trees { - let (new_use_tree, count) = - use_tree_without_unused_import(use_tree, unused_imports); - if let Some(new_use_tree) = new_use_tree { - new_use_trees.push(new_use_tree); - } - total_count += count; - } - - let new_use_tree = if new_use_trees.is_empty() { - None - } else if new_use_trees.len() == 1 { - let new_use_tree = new_use_trees.remove(0); - - let mut prefix = use_tree.prefix.clone(); - prefix.segments.extend(new_use_tree.prefix.segments); - - Some(UseTree { prefix, kind: new_use_tree.kind, location: use_tree.location }) - } else { - Some(UseTree { - prefix: use_tree.prefix.clone(), - kind: UseTreeKind::List(new_use_trees), - location: use_tree.location, - }) - }; - - (new_use_tree, total_count) - } + let text_edit = TextEdit { range, new_text }; + self.unused_imports_text_edits.push(text_edit.clone()); + + let code_action = self.new_quick_fix(title, text_edit); + self.code_actions.push(code_action); } } @@ -297,6 +231,35 @@ mod tests { assert_code_action(title, src, expected); } + #[test] + fn test_removes_unused_self_import() { + let title = "Remove unused import"; + + let src = r#" + mod moo { + pub fn bar() {} + } + use moo::{se>| Result<(), CliErr package, &args.compile_options, args.overwrite, + args.remove_unused_imports, )?; } Ok(()) @@ -92,9 +111,33 @@ fn check_package( package: &Package, compile_options: &CompileOptions, overwrite: bool, -) -> Result<(), CompileError> { + remove_unused_imports: bool, +) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, compile_options)?; + + if remove_unused_imports { + let mut result = check_crate(&mut context, crate_id, compile_options); + + // Only rewrite sources when the check succeeded: with compilation errors present the + // usage tracker's picture of the program is not reliable. Warnings for the imports + // that were just removed are dropped so they aren't reported for code that no longer + // exists. + if let Ok((_, warnings)) = &mut result { + let removed_import_locations = + remove_unused_imports_from_package(&context, crate_id, parsed_files)?; + remove_fixed_import_warnings(warnings, &removed_import_locations); + } + + report_errors( + result, + &context.file_manager, + &context.parsed_files, + compile_options.deny_warnings, + compile_options.silence_warnings, + )?; + } else { + check_crate_and_report_errors(&mut context, crate_id, compile_options)?; + } if package.is_library() || package.is_contract() { // Libraries do not have ABIs while contracts have many, so we cannot generate a `Prover.toml` file. @@ -115,8 +158,74 @@ fn check_package( Ok(()) } else { - Err(CompileError::MissingMainFunction(package.name.clone())) + Err(CompileError::MissingMainFunction(package.name.clone()).into()) + } +} + +/// Rewrites the package's source files on disk, pruning every import the frontend reported as +/// unused. Only files belonging to `crate_id` are touched, so dependencies are never modified. +/// Returns the locations of the imports that were removed. +fn remove_unused_imports_from_package( + context: &Context, + crate_id: CrateId, + parsed_files: &ParsedFiles, +) -> Result, CliError> { + let mut unused_imports_per_file: HashMap> = HashMap::new(); + for (module_id, unused_imports) in context.usage_tracker.unused_imports() { + if module_id.krate != crate_id { + continue; + } + for (ident, location) in unused_imports.keys() { + unused_imports_per_file + .entry(location.file) + .or_default() + .insert((ident.clone(), *location)); + } + } + + // Sort by path so files are reported in a deterministic order. + let mut file_ids: Vec = unused_imports_per_file.keys().copied().collect(); + file_ids.sort_by_key(|file_id| context.file_manager.path(*file_id).map(Path::to_path_buf)); + + let mut removed_import_locations = HashSet::new(); + for file_id in file_ids { + let unused_imports = &unused_imports_per_file[&file_id]; + let Some((parsed_module, _)) = parsed_files.get(&file_id) else { + continue; + }; + let Some(source) = context.file_manager.fetch_file(file_id) else { + continue; + }; + let Some(new_source) = remove_unused_imports(source, parsed_module, unused_imports) else { + continue; + }; + let Some(path) = context.file_manager.path(file_id) else { + continue; + }; + std::fs::write(path, new_source).map_err(|error| { + CliError::Generic(format!("Failed to write {}: {error}", path.display())) + })?; + println!("Removed unused imports from {}", path.display()); + removed_import_locations.extend(unused_imports.iter().map(|(_ident, location)| *location)); } + + Ok(removed_import_locations) +} + +/// Removes from `warnings` the unused-import warnings that were just fixed: the ones whose +/// label points at an import that was removed from the source. Other diagnostics (warnings at +/// other locations, and anything that is not a warning) are left untouched. +fn remove_fixed_import_warnings( + warnings: &mut Vec, + removed_import_locations: &HashSet, +) { + warnings.retain(|diagnostic| { + !(diagnostic.is_warning() + && diagnostic + .secondaries + .iter() + .any(|label| removed_import_locations.contains(&label.location))) + }); } /// Generates the contents of a toml file with fields for each of the passed parameters. @@ -161,10 +270,46 @@ fn create_input_toml_template( #[cfg(test)] mod tests { + use std::collections::HashSet; + + use fm::FileId; use insta::assert_snapshot; use noirc_abi::{AbiParameter, AbiType, AbiVisibility, Sign}; + use noirc_errors::{CustomDiagnostic, Location, Span}; - use super::create_input_toml_template; + use super::{create_input_toml_template, remove_fixed_import_warnings}; + + #[test] + fn removes_only_warnings_at_removed_import_locations() { + let file = FileId::dummy(); + let removed_location = Location::new(Span::from(10..13), file); + let other_location = Location::new(Span::from(20..23), file); + + let fixed_warning = CustomDiagnostic::simple_warning( + "unused import foo".to_string(), + "unused import".to_string(), + removed_location, + ); + let unrelated_warning = CustomDiagnostic::simple_warning( + "unused variable x".to_string(), + "unused variable".to_string(), + other_location, + ); + // Not a warning, so it must survive even though it points at a removed location. + let error_at_removed_location = CustomDiagnostic::simple_error( + "some error".to_string(), + "error".to_string(), + removed_location, + ); + + let mut warnings = vec![fixed_warning, unrelated_warning, error_at_removed_location]; + let removed_import_locations = HashSet::from([removed_location]); + remove_fixed_import_warnings(&mut warnings, &removed_import_locations); + + let messages: Vec<&str> = + warnings.iter().map(|diagnostic| diagnostic.message.as_str()).collect(); + assert_eq!(messages, vec!["unused variable x", "some error"]); + } #[test] fn valid_toml_template() { From f70660ff6f6c28c98b92671b654146a1f3f6d886 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 22 Jul 2026 15:43:21 +0000 Subject: [PATCH 2/2] feat: generalize to `nargo check --fix`, also dropping unnecessary `mut` Renames the flag from --remove-unused-imports to --fix and generalizes the frontend rewrite (now `noirc_frontend::fix`) to apply any warning fix that is a pure removal. Alongside unused imports it now drops `mut` modifiers the elaborator reported as unnecessary, located via the typed `VariableDoesNotNeedToBeMutable` errors that a new `check_crate_returning_frontend_errors` driver entry point exposes. The fix collector is now an AST visitor, since `mut` patterns live inside function bodies rather than at item level. The elaborator does not currently warn for never-mutated `mut` parameters; a characterization test documents that boundary. --- compiler/noirc_driver/src/lib.rs | 17 +- .../src/{remove_unused_imports.rs => fix.rs} | 159 +++++++++++------- compiler/noirc_frontend/src/lib.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 2 +- .../{remove_unused_imports.rs => fix.rs} | 144 +++++++++++++--- .../code_action/remove_unused_import.rs | 2 +- tooling/nargo_cli/src/cli/check_cmd.rs | 101 ++++++----- 7 files changed, 290 insertions(+), 137 deletions(-) rename compiler/noirc_frontend/src/{remove_unused_imports.rs => fix.rs} (61%) rename compiler/noirc_frontend/src/tests/{remove_unused_imports.rs => fix.rs} (61%) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 10692d499d5..a782d2435df 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -36,6 +36,7 @@ use noirc_evaluator::ssa::{ }; use noirc_frontend::elaborator::{FrontendOptions, UnstableFeature}; use noirc_frontend::error_reporting::function_locations_in_parsed_module; +use noirc_frontend::hir::def_collector::dc_crate::CompilationError; use noirc_frontend::hir::def_map::{CrateDefMap, ModuleDefId, ModuleId}; use noirc_frontend::hir::{Context, ParsedFiles}; use noirc_frontend::monomorphization::{ @@ -451,6 +452,17 @@ pub fn check_crate( crate_id: CrateId, options: &CompileOptions, ) -> CompilationResult<()> { + check_crate_returning_frontend_errors(context, crate_id, options).0 +} + +/// Like [check_crate], but also returns the typed frontend errors the diagnostics were +/// rendered from, for callers that need to act on specific error kinds (e.g. `nargo check +/// --fix` locating the code a fixable warning points at). +pub fn check_crate_returning_frontend_errors( + context: &mut Context, + crate_id: CrateId, + options: &CompileOptions, +) -> (CompilationResult<()>, Vec) { if options.disable_comptime_printing { context.disable_comptime_printing(); } @@ -470,11 +482,12 @@ pub fn check_crate( }) .collect(); - if has_errors(&warnings_and_errors, options.deny_warnings) { + let result = if has_errors(&warnings_and_errors, options.deny_warnings) { Err(warnings_and_errors) } else { Ok(((), warnings_and_errors)) - } + }; + (result, diagnostics) } pub fn compute_function_abi( diff --git a/compiler/noirc_frontend/src/remove_unused_imports.rs b/compiler/noirc_frontend/src/fix.rs similarity index 61% rename from compiler/noirc_frontend/src/remove_unused_imports.rs rename to compiler/noirc_frontend/src/fix.rs index 2d5209b9cab..f87fc46dca8 100644 --- a/compiler/noirc_frontend/src/remove_unused_imports.rs +++ b/compiler/noirc_frontend/src/fix.rs @@ -1,51 +1,66 @@ -//! Rewrites source code to remove imports that the elaborator determined are unused. +//! Rewrites source code to apply fixes for warnings whose fix is a pure removal: the code the +//! elaborator warned about is deleted or simplified, never added to. //! -//! The entry point is [remove_unused_imports], which takes a file's source text, its parsed -//! AST and the set of unused import names (as reported by -//! [UsageTracker::unused_imports][crate::usage_tracker::UsageTracker::unused_imports]) and -//! returns the source with those imports pruned. Composite `use` trees are rewritten rather -//! than deleted: `use foo::{bar, baz};` with `baz` unused becomes `use foo::bar;`, and a -//! `use` whose imports are all unused is deleted entirely (along with its line, when the -//! line contains nothing else). +//! The entry point is [apply_fixes], which takes a file's source text, its parsed AST and the +//! set of [Fixes] to apply, and returns the rewritten source. The supported fixes are: //! -//! Unused imports are identified by the imported name and its exact source location — the -//! same `(Ident, Location)` key the usage tracker records — so two imports of the same name -//! never alias each other. +//! - **Unused imports** (as reported by +//! [UsageTracker::unused_imports][crate::usage_tracker::UsageTracker::unused_imports]). +//! Composite `use` trees are rewritten rather than deleted: `use foo::{bar, baz};` with +//! `baz` unused becomes `use foo::bar;`, and a `use` whose imports are all unused is +//! deleted entirely (along with its line, when the line contains nothing else). Unused +//! imports are identified by the imported name and its exact source location — the same +//! `(Ident, Location)` key the usage tracker records — so two imports of the same name +//! never alias each other. +//! - **Unnecessary `mut` modifiers**, identified by the location of the binding's +//! identifier (the location `ResolverError::VariableDoesNotNeedToBeMutable` reports). The +//! `mut` keyword is deleted and the binding is left untouched. //! //! Pruned `use` trees are re-rendered with [UseTree]'s `Display` impl, which produces a //! single-line canonical form. Callers that want the result to respect a formatting style //! should run the formatter afterwards. //! -//! Removal is deliberately a single round, not a fixpoint: it removes exactly the imports -//! the elaborator reported for the given compilation. An import whose only consumer is -//! another `use` statement's path (resolving a path marks its first segment as used) is only +//! Fixing is deliberately a single round, not a fixpoint: it applies exactly the fixes the +//! elaborator reported for the given compilation. An import whose only consumer is another +//! `use` statement's path (resolving a path marks its first segment as used) is only //! reported unused — and thus only removed — after the consuming `use` has been removed and //! the program re-elaborated. use std::collections::HashSet; use std::ops::Range; -use noirc_errors::Location; +use noirc_errors::{Location, Span}; -use crate::ast::{Ident, ItemVisibility, Path, UseTree, UseTreeKind}; -use crate::parser::{Item, ItemKind, ParsedModule}; +use crate::ast::{Ident, ItemVisibility, Path, Pattern, UseTree, UseTreeKind, Visitor}; +use crate::parser::ParsedModule; -/// Returns `source` with all imports in `unused_imports` removed, or `None` if there was -/// nothing to remove. `parsed_module` must be the result of parsing `source`. -pub fn remove_unused_imports( - source: &str, - parsed_module: &ParsedModule, - unused_imports: &HashSet<(Ident, Location)>, -) -> Option { - if unused_imports.is_empty() { - return None; +/// The warnings to fix, identified the same way the elaborator reports them. +#[derive(Debug, Default)] +pub struct Fixes { + /// Unused imports, keyed by the imported name and its location — the same key + /// [UsageTracker::unused_imports][crate::usage_tracker::UsageTracker::unused_imports] + /// records. + pub unused_imports: HashSet<(Ident, Location)>, + /// Locations of binding identifiers whose `mut` modifier is unnecessary. + pub unnecessary_muts: HashSet, +} + +impl Fixes { + pub fn is_empty(&self) -> bool { + self.unused_imports.is_empty() && self.unnecessary_muts.is_empty() } +} - let is_unused = |ident: &Ident| unused_imports.contains(&(ident.clone(), ident.location())); +/// Returns `source` with all of `fixes` applied, or `None` if there was nothing to fix. +/// `parsed_module` must be the result of parsing `source`. +pub fn apply_fixes(source: &str, parsed_module: &ParsedModule, fixes: &Fixes) -> Option { + if fixes.is_empty() { + return None; + } - let mut replacements = Vec::new(); - let mut deletions = Vec::new(); - collect_edits(&parsed_module.items, &is_unused, &mut replacements, &mut deletions); + let mut collector = FixCollector { fixes, replacements: Vec::new(), deletions: Vec::new() }; + parsed_module.accept(&mut collector); + let FixCollector { replacements, mut deletions, .. } = collector; if replacements.is_empty() && deletions.is_empty() { return None; } @@ -80,43 +95,59 @@ pub fn remove_unused_imports( Some(new_source) } -/// Walks `items` (recursing into inline submodules) and records one edit per `use` item that -/// contains at least one unused import: a replacement when some imports remain, a deletion of -/// the item's span when none do. -fn collect_edits( - items: &[Item], - is_unused: &dyn Fn(&Ident) -> bool, - replacements: &mut Vec<(Range, String)>, - deletions: &mut Vec>, -) { - for item in items { - match &item.kind { - ItemKind::Import(use_tree, visibility) => { - let (new_use_tree, removed_count) = - use_tree_without_unused_imports(use_tree, is_unused); - if removed_count == 0 { - continue; - } +/// Walks the AST recording one edit per fixable warning: a replacement for a `use` item that +/// keeps some of its imports, a deletion of the whole item's span for a `use` item with no +/// imports left. +struct FixCollector<'a> { + fixes: &'a Fixes, + replacements: Vec<(Range, String)>, + deletions: Vec>, +} - let span = item.location.span; - let range = span.start() as usize..span.end() as usize; - match new_use_tree { - Some(use_tree) => { - let replacement = if *visibility == ItemVisibility::Private { - format!("use {use_tree};") - } else { - format!("{visibility} use {use_tree};") - }; - replacements.push((range, replacement)); - } - None => deletions.push(range), - } - } - ItemKind::Submodules(submodule) => { - collect_edits(&submodule.contents.items, is_unused, replacements, deletions); +impl Visitor for FixCollector<'_> { + fn visit_import(&mut self, use_tree: &UseTree, span: Span, visibility: ItemVisibility) -> bool { + let is_unused = + |ident: &Ident| self.fixes.unused_imports.contains(&(ident.clone(), ident.location())); + + let (new_use_tree, removed_count) = use_tree_without_unused_imports(use_tree, &is_unused); + if removed_count == 0 { + return false; + } + + let range = span.start() as usize..span.end() as usize; + match new_use_tree { + Some(use_tree) => { + let replacement = if visibility == ItemVisibility::Private { + format!("use {use_tree};") + } else { + format!("{visibility} use {use_tree};") + }; + self.replacements.push((range, replacement)); } - _ => (), + None => self.deletions.push(range), } + + false + } + + fn visit_mutable_pattern( + &mut self, + pattern: &Pattern, + span: Span, + is_synthesized: bool, + ) -> bool { + // `span` covers the whole `mut x` pattern while the identifier starts after the + // `mut`, so deleting up to the identifier deletes exactly the modifier. Synthesized + // patterns (e.g. desugared `self`) have no `mut` token in the source to delete. + if !is_synthesized + && let Pattern::Identifier(ident) = pattern + && self.fixes.unnecessary_muts.contains(&ident.location()) + { + let range = span.start() as usize..ident.location().span.start() as usize; + self.replacements.push((range, String::new())); + } + + true } } diff --git a/compiler/noirc_frontend/src/lib.rs b/compiler/noirc_frontend/src/lib.rs index 60c3d1e23d6..441349f8f19 100644 --- a/compiler/noirc_frontend/src/lib.rs +++ b/compiler/noirc_frontend/src/lib.rs @@ -16,6 +16,7 @@ pub mod ast; pub mod debug; pub mod elaborator; pub mod error_reporting; +pub mod fix; pub mod graph; pub mod lexer; pub mod locations; @@ -24,7 +25,6 @@ pub mod monomorphization; pub mod node_interner; pub mod ownership; pub mod parser; -pub mod remove_unused_imports; pub mod resolve_locations; pub mod shared; pub mod usage_tracker; diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 0e99545c8e5..f77ba1084e8 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -12,6 +12,7 @@ mod entry_point_size; mod enums; mod expand; mod expressions; +mod fix; mod functions; mod globals; mod imports; @@ -22,7 +23,6 @@ mod name_shadowing; mod numeric_generics; mod oracles; mod references; -mod remove_unused_imports; mod runtime; mod structs; mod traits; diff --git a/compiler/noirc_frontend/src/tests/remove_unused_imports.rs b/compiler/noirc_frontend/src/tests/fix.rs similarity index 61% rename from compiler/noirc_frontend/src/tests/remove_unused_imports.rs rename to compiler/noirc_frontend/src/tests/fix.rs index 03505b323ea..e8ade2c1eb6 100644 --- a/compiler/noirc_frontend/src/tests/remove_unused_imports.rs +++ b/compiler/noirc_frontend/src/tests/fix.rs @@ -1,25 +1,36 @@ -//! Tests for [crate::remove_unused_imports], which rewrites source code to prune imports -//! that the elaborator determined are unused. +//! Tests for [crate::fix], which rewrites source code to apply removal-only fixes for the +//! warnings the elaborator reported (unused imports, unnecessary `mut`). use std::collections::HashSet; use noirc_errors::Location; -use crate::ast::Ident; -use crate::remove_unused_imports::remove_unused_imports; +use crate::fix::{Fixes, apply_fixes}; +use crate::hir::def_collector::dc_crate::CompilationError; +use crate::hir::resolution::errors::ResolverError; use crate::test_utils::get_program; -/// Compiles `src`, collects every unused import from the usage tracker, and returns `src` -/// rewritten with those imports removed. Returns `None` if there was nothing to remove. -fn source_without_unused_imports(src: &str) -> Option { - let (parsed_module, context, _errors) = get_program(src); - let unused_imports: HashSet<(Ident, Location)> = context +/// Compiles `src`, collects every fixable warning (unused imports from the usage tracker, +/// unnecessary `mut`s from the reported errors), and returns `src` rewritten with those +/// fixes applied. Returns `None` if there was nothing to fix. +fn fixed_source(src: &str) -> Option { + let (parsed_module, context, errors) = get_program(src); + let unused_imports = context .usage_tracker .unused_imports() .values() .flat_map(|imports| imports.keys().cloned()) .collect(); - remove_unused_imports(src, &parsed_module, &unused_imports) + let unnecessary_muts: HashSet = errors + .iter() + .filter_map(|error| match error { + CompilationError::ResolverError(ResolverError::VariableDoesNotNeedToBeMutable { + ident, + }) => Some(ident.location()), + _ => None, + }) + .collect(); + apply_fixes(src, &parsed_module, &Fixes { unused_imports, unnecessary_muts }) } #[test] @@ -40,7 +51,7 @@ fn main() { qux(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -73,7 +84,7 @@ fn main() { bar(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -103,7 +114,7 @@ fn main() { corge(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub mod bar { @@ -131,7 +142,7 @@ use foo::{bar, spam}; fn main() {} "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -155,7 +166,7 @@ fn main() { b(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -182,7 +193,7 @@ fn main() { bar(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -217,7 +228,7 @@ fn main() { qux::corge(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -250,7 +261,7 @@ use foo::spam; fn main() {} "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -274,7 +285,7 @@ fn main() { foo::bar(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -308,7 +319,7 @@ fn main() { qux(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} @@ -344,8 +355,7 @@ fn main() {} "#; // The first round only removes `use bar::qux;`: at this point `use foo::bar;` is // considered used, because resolving the path `bar::qux` referenced it. - let after_first_run = - source_without_unused_imports(src).expect("expected imports to be removed"); + let after_first_run = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(after_first_run, @r" mod foo { pub mod bar { @@ -359,8 +369,7 @@ fn main() {} "); // Re-elaborating the pruned source reveals that `use foo::bar;` is now unused too. - let after_second_run = - source_without_unused_imports(&after_first_run).expect("expected imports to be removed"); + let after_second_run = fixed_source(&after_first_run).expect("expected imports to be removed"); insta::assert_snapshot!(after_second_run, @r" mod foo { pub mod bar { @@ -372,7 +381,88 @@ fn main() {} "); // The fixpoint is reached: a third round has nothing left to remove. - assert_eq!(source_without_unused_imports(&after_second_run), None); + assert_eq!(fixed_source(&after_second_run), None); +} + +#[test] +fn removes_unnecessary_mut_from_let_binding() { + let src = r#"fn main() { + let mut x = 1; + assert(x == 1); +} +"#; + let result = fixed_source(src).expect("expected the `mut` to be removed"); + insta::assert_snapshot!(result, @r" + fn main() { + let x = 1; + assert(x == 1); + } + "); +} + +/// The elaborator does not currently report `VariableDoesNotNeedToBeMutable` for function +/// parameters, so there is nothing to fix here. If it ever starts to, this test will fail +/// and should be flipped to assert the `mut` is removed — the fix machinery already handles +/// patterns wherever they appear. +#[test] +fn does_not_change_never_mutated_mut_function_parameter() { + let src = r#"fn foo(mut x: Field) -> Field { + x +} + +fn main() { + assert(foo(1) == 1); +} +"#; + assert_eq!(fixed_source(src), None); +} + +#[test] +fn removes_unnecessary_mut_from_tuple_pattern_binding() { + let src = r#"fn main() { + let (mut a, b) = (1, 2); + assert(a + b == 3); +} +"#; + let result = fixed_source(src).expect("expected the `mut` to be removed"); + insta::assert_snapshot!(result, @r" + fn main() { + let (a, b) = (1, 2); + assert(a + b == 3); + } + "); +} + +#[test] +fn fixes_unused_import_and_unnecessary_mut_in_one_go() { + let src = r#"mod foo { + pub fn bar() {} + pub fn spam() {} +} + +use foo::{bar, spam}; + +fn main() { + let mut x = 1; + assert(x == 1); + bar(); +} +"#; + let result = fixed_source(src).expect("expected fixes to be applied"); + insta::assert_snapshot!(result, @r" + mod foo { + pub fn bar() {} + pub fn spam() {} + } + + use foo::bar; + + fn main() { + let x = 1; + assert(x == 1); + bar(); + } + "); } #[test] @@ -387,7 +477,7 @@ fn main() { bar(); } "#; - assert_eq!(source_without_unused_imports(src), None); + assert_eq!(fixed_source(src), None); } #[test] @@ -409,7 +499,7 @@ fn main() { qux::corge(); } "#; - let result = source_without_unused_imports(src).expect("expected imports to be removed"); + let result = fixed_source(src).expect("expected imports to be removed"); insta::assert_snapshot!(result, @r" mod foo { pub fn bar() {} diff --git a/tooling/lsp/src/requests/code_action/remove_unused_import.rs b/tooling/lsp/src/requests/code_action/remove_unused_import.rs index 996085c7b29..6a81b36fccd 100644 --- a/tooling/lsp/src/requests/code_action/remove_unused_import.rs +++ b/tooling/lsp/src/requests/code_action/remove_unused_import.rs @@ -4,8 +4,8 @@ use noirc_errors::{Location, Span}; use noirc_frontend::{ ParsedModule, ast::{Ident, ItemVisibility, UseTree}, + fix::use_tree_without_unused_imports, parser::{Item, ItemKind}, - remove_unused_imports::use_tree_without_unused_imports, }; use crate::byte_span_to_range; diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index 2d7fc43cf58..d92aa70da45 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -18,14 +18,16 @@ use nargo::{ use nargo_toml::PackageSelection; use noir_artifact_cli::fs::artifact::write_to_file; use noirc_abi::{AbiParameter, AbiType, MAIN_RETURN_NAME}; -use noirc_driver::{CompileOptions, check_crate, compute_function_abi}; +use noirc_driver::{ + CompileOptions, check_crate, check_crate_returning_frontend_errors, compute_function_abi, +}; use noirc_errors::{CustomDiagnostic, Location}; use noirc_frontend::{ - ast::Ident, + fix::{Fixes, apply_fixes}, graph::CrateId, - hir::{Context, ParsedFiles}, + hir::resolution::errors::ResolverError, + hir::{Context, ParsedFiles, def_collector::dc_crate::CompilationError as FrontendError}, monomorphization::monomorphize, - remove_unused_imports::remove_unused_imports, }; use super::{LockType, PackageOptions, WorkspaceCommand}; @@ -48,11 +50,12 @@ pub(crate) struct CheckCommand { #[clap(long, hide = true)] show_program_hash: bool, - /// Rewrite the package's source files, removing any imports that are unused. - /// Removing an import can reveal further unused imports (ones only referenced by the - /// removed import); run the command again to remove those too. + /// Rewrite the package's source files, fixing any warnings whose fix is a pure removal: + /// unused imports are removed and unnecessary `mut` modifiers are dropped. + /// Applying a fix can reveal further fixable warnings (e.g. an import only referenced by + /// a removed import); run the command again to fix those too. #[clap(long, hide = true)] - remove_unused_imports: bool, + fix: bool, } impl WorkspaceCommand for CheckCommand { @@ -97,7 +100,7 @@ pub(crate) fn run(args: CheckCommand, workspace: Workspace) -> Result<(), CliErr package, &args.compile_options, args.overwrite, - args.remove_unused_imports, + args.fix, )?; } Ok(()) @@ -111,21 +114,21 @@ fn check_package( package: &Package, compile_options: &CompileOptions, overwrite: bool, - remove_unused_imports: bool, + fix: bool, ) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - if remove_unused_imports { - let mut result = check_crate(&mut context, crate_id, compile_options); + if fix { + let (mut result, frontend_errors) = + check_crate_returning_frontend_errors(&mut context, crate_id, compile_options); // Only rewrite sources when the check succeeded: with compilation errors present the - // usage tracker's picture of the program is not reliable. Warnings for the imports - // that were just removed are dropped so they aren't reported for code that no longer - // exists. + // frontend's picture of the program is not reliable. Warnings for the code that was + // just fixed are dropped so they aren't reported for code that no longer exists. if let Ok((_, warnings)) = &mut result { - let removed_import_locations = - remove_unused_imports_from_package(&context, crate_id, parsed_files)?; - remove_fixed_import_warnings(warnings, &removed_import_locations); + let fixed_locations = + apply_fixes_to_package(&context, crate_id, parsed_files, &frontend_errors)?; + remove_fixed_warnings(warnings, &fixed_locations); } report_errors( @@ -162,41 +165,59 @@ fn check_package( } } -/// Rewrites the package's source files on disk, pruning every import the frontend reported as -/// unused. Only files belonging to `crate_id` are touched, so dependencies are never modified. -/// Returns the locations of the imports that were removed. -fn remove_unused_imports_from_package( +/// Rewrites the package's source files on disk, applying every removal-only fix the frontend +/// reported: unused imports are pruned and unnecessary `mut` modifiers are dropped. Only +/// files belonging to `crate_id` are touched, so dependencies are never modified. Returns the +/// locations of the fixed warnings (the imports that were removed, the bindings whose `mut` +/// was dropped). +fn apply_fixes_to_package( context: &Context, crate_id: CrateId, parsed_files: &ParsedFiles, + frontend_errors: &[FrontendError], ) -> Result, CliError> { - let mut unused_imports_per_file: HashMap> = HashMap::new(); + let mut fixes_per_file: HashMap = HashMap::new(); + for (module_id, unused_imports) in context.usage_tracker.unused_imports() { if module_id.krate != crate_id { continue; } for (ident, location) in unused_imports.keys() { - unused_imports_per_file + fixes_per_file .entry(location.file) .or_default() + .unused_imports .insert((ident.clone(), *location)); } } + let crate_files = context.crate_files(&crate_id); + for error in frontend_errors { + if let FrontendError::ResolverError(ResolverError::VariableDoesNotNeedToBeMutable { + ident, + }) = error + { + let location = ident.location(); + if crate_files.contains(&location.file) { + fixes_per_file.entry(location.file).or_default().unnecessary_muts.insert(location); + } + } + } + // Sort by path so files are reported in a deterministic order. - let mut file_ids: Vec = unused_imports_per_file.keys().copied().collect(); + let mut file_ids: Vec = fixes_per_file.keys().copied().collect(); file_ids.sort_by_key(|file_id| context.file_manager.path(*file_id).map(Path::to_path_buf)); - let mut removed_import_locations = HashSet::new(); + let mut fixed_locations = HashSet::new(); for file_id in file_ids { - let unused_imports = &unused_imports_per_file[&file_id]; + let fixes = &fixes_per_file[&file_id]; let Some((parsed_module, _)) = parsed_files.get(&file_id) else { continue; }; let Some(source) = context.file_manager.fetch_file(file_id) else { continue; }; - let Some(new_source) = remove_unused_imports(source, parsed_module, unused_imports) else { + let Some(new_source) = apply_fixes(source, parsed_module, fixes) else { continue; }; let Some(path) = context.file_manager.path(file_id) else { @@ -205,26 +226,24 @@ fn remove_unused_imports_from_package( std::fs::write(path, new_source).map_err(|error| { CliError::Generic(format!("Failed to write {}: {error}", path.display())) })?; - println!("Removed unused imports from {}", path.display()); - removed_import_locations.extend(unused_imports.iter().map(|(_ident, location)| *location)); + println!("Fixed {}", path.display()); + fixed_locations.extend(fixes.unused_imports.iter().map(|(_ident, location)| *location)); + fixed_locations.extend(fixes.unnecessary_muts.iter().copied()); } - Ok(removed_import_locations) + Ok(fixed_locations) } -/// Removes from `warnings` the unused-import warnings that were just fixed: the ones whose -/// label points at an import that was removed from the source. Other diagnostics (warnings at +/// Removes from `warnings` the ones that were just fixed: the ones whose label points at code +/// that was rewritten (a removed import, a dropped `mut`). Other diagnostics (warnings at /// other locations, and anything that is not a warning) are left untouched. -fn remove_fixed_import_warnings( +fn remove_fixed_warnings( warnings: &mut Vec, - removed_import_locations: &HashSet, + fixed_locations: &HashSet, ) { warnings.retain(|diagnostic| { !(diagnostic.is_warning() - && diagnostic - .secondaries - .iter() - .any(|label| removed_import_locations.contains(&label.location))) + && diagnostic.secondaries.iter().any(|label| fixed_locations.contains(&label.location))) }); } @@ -277,7 +296,7 @@ mod tests { use noirc_abi::{AbiParameter, AbiType, AbiVisibility, Sign}; use noirc_errors::{CustomDiagnostic, Location, Span}; - use super::{create_input_toml_template, remove_fixed_import_warnings}; + use super::{create_input_toml_template, remove_fixed_warnings}; #[test] fn removes_only_warnings_at_removed_import_locations() { @@ -304,7 +323,7 @@ mod tests { let mut warnings = vec![fixed_warning, unrelated_warning, error_at_removed_location]; let removed_import_locations = HashSet::from([removed_location]); - remove_fixed_import_warnings(&mut warnings, &removed_import_locations); + remove_fixed_warnings(&mut warnings, &removed_import_locations); let messages: Vec<&str> = warnings.iter().map(|diagnostic| diagnostic.message.as_str()).collect();