From bb71bcb3a0eec6f76a48085ba9684cb8dd4aa27f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 2 Jan 2025 13:58:08 -0300 Subject: [PATCH] feat: warn on trait method visibility (#6923) --- .../src/hir/def_collector/dc_mod.rs | 6 +++- compiler/noirc_frontend/src/parser/errors.rs | 12 ++++--- .../noirc_frontend/src/parser/parser/impls.rs | 4 +-- .../src/parser/parser/traits.rs | 24 ++++++++++++-- tooling/lsp/src/requests/mod.rs | 4 ++- tooling/nargo_fmt/src/formatter/function.rs | 21 ++++++++++--- tooling/nargo_fmt/src/formatter/impls.rs | 4 ++- tooling/nargo_fmt/src/formatter/item.rs | 5 ++- tooling/nargo_fmt/src/formatter/trait_impl.rs | 31 +++++++++++++------ tooling/nargo_fmt/src/formatter/traits.rs | 31 +++++++++++++++---- tooling/nargo_fmt/src/lib.rs | 1 + 11 files changed, 110 insertions(+), 33 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index e7953aab5a4..8f46d5429bc 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -1222,7 +1222,11 @@ pub(crate) fn collect_trait_impl_items( for item in std::mem::take(&mut trait_impl.items) { match item.item.kind { - TraitImplItemKind::Function(impl_method) => { + TraitImplItemKind::Function(mut impl_method) => { + // Regardless of what visibility was on the source code, treat it as public + // (a warning is produced during parsing for this) + impl_method.def.visibility = ItemVisibility::Public; + let func_id = interner.push_empty_fn(); let location = Location::new(impl_method.span(), file_id); interner.push_function(func_id, &impl_method.def, module, location); diff --git a/compiler/noirc_frontend/src/parser/errors.rs b/compiler/noirc_frontend/src/parser/errors.rs index 7cb8731593b..32cacf5e4ed 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/compiler/noirc_frontend/src/parser/errors.rs @@ -71,6 +71,8 @@ pub enum ParserErrorReason { PatternInTraitFunctionParameter, #[error("Patterns aren't allowed in a trait impl's associated constants")] PatternInAssociatedConstant, + #[error("Visibility is ignored on a trait method")] + TraitVisibilityIgnored, #[error("Visibility is ignored on a trait impl method")] TraitImplVisibilityIgnored, #[error("comptime keyword is deprecated")] @@ -183,11 +185,8 @@ impl ParserError { } pub fn is_warning(&self) -> bool { - matches!( - self.reason(), - Some(ParserErrorReason::ExperimentalFeature(_)) - | Some(ParserErrorReason::MissingSafetyComment) - ) + let diagnostic: Diagnostic = self.into(); + diagnostic.is_warning() } } @@ -264,6 +263,9 @@ impl<'a> From<&'a ParserError> for Diagnostic { ParserErrorReason::ExperimentalFeature(_) => { Diagnostic::simple_warning(reason.to_string(), "".into(), error.span) } + ParserErrorReason::TraitVisibilityIgnored => { + Diagnostic::simple_warning(reason.to_string(), "".into(), error.span) + } ParserErrorReason::TraitImplVisibilityIgnored => { Diagnostic::simple_warning(reason.to_string(), "".into(), error.span) } diff --git a/compiler/noirc_frontend/src/parser/parser/impls.rs b/compiler/noirc_frontend/src/parser/parser/impls.rs index 8e6b3bae0e9..278c20e1e27 100644 --- a/compiler/noirc_frontend/src/parser/parser/impls.rs +++ b/compiler/noirc_frontend/src/parser/parser/impls.rs @@ -245,7 +245,7 @@ impl<'a> Parser<'a> { let noir_function = self.parse_function( attributes, - ItemVisibility::Public, + modifiers.visibility, modifiers.comptime.is_some(), modifiers.unconstrained.is_some(), true, // allow_self @@ -482,7 +482,7 @@ mod tests { panic!("Expected function"); }; assert_eq!(function.def.name.to_string(), "foo"); - assert_eq!(function.def.visibility, ItemVisibility::Public); + assert_eq!(function.def.visibility, ItemVisibility::Private); } #[test] diff --git a/compiler/noirc_frontend/src/parser/parser/traits.rs b/compiler/noirc_frontend/src/parser/parser/traits.rs index e03b629e9ea..6f6a9bab960 100644 --- a/compiler/noirc_frontend/src/parser/parser/traits.rs +++ b/compiler/noirc_frontend/src/parser/parser/traits.rs @@ -225,6 +225,10 @@ impl<'a> Parser<'a> { false, // allow mut ); + if modifiers.visibility != ItemVisibility::Private { + self.push_error(ParserErrorReason::TraitVisibilityIgnored, modifiers.visibility_span); + } + if !self.eat_keyword(Keyword::Fn) { self.modifiers_not_followed_by_an_item(modifiers); return None; @@ -285,7 +289,11 @@ mod tests { use crate::{ ast::{NoirTrait, NoirTraitImpl, TraitItem}, parser::{ - parser::{parse_program, tests::expect_no_errors, ParserErrorReason}, + parser::{ + parse_program, + tests::{expect_no_errors, get_single_error, get_source_with_error_span}, + ParserErrorReason, + }, ItemKind, }, }; @@ -513,7 +521,19 @@ mod tests { } #[test] - fn parse_trait_inheirtance() { + fn parse_trait_function_with_visibility() { + let src = " + trait Foo { pub fn foo(); } + ^^^ + "; + let (src, span) = get_source_with_error_span(src); + let (_module, errors) = parse_program(&src); + let error = get_single_error(&errors, span); + assert!(error.to_string().contains("Visibility is ignored on a trait method")); + } + + #[test] + fn parse_trait_inheritance() { let src = "trait Foo: Bar + Baz {}"; let noir_trait = parse_trait_no_errors(src); assert_eq!(noir_trait.bounds.len(), 2); diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 22bdda8d7d7..f7b58d7d42f 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -18,6 +18,7 @@ use nargo_fmt::Config; use noirc_frontend::graph::CrateId; use noirc_frontend::hir::def_map::CrateDefMap; +use noirc_frontend::parser::ParserError; use noirc_frontend::usage_tracker::UsageTracker; use noirc_frontend::{graph::Dependency, node_interner::NodeInterner}; use serde::{Deserialize, Serialize}; @@ -285,7 +286,8 @@ fn on_formatting_inner( if let Some(source) = state.input_files.get(&path) { let (module, errors) = noirc_frontend::parse_program(source); - if !errors.is_empty() { + let is_all_warnings = errors.iter().all(ParserError::is_warning); + if !is_all_warnings { return Ok(None); } diff --git a/tooling/nargo_fmt/src/formatter/function.rs b/tooling/nargo_fmt/src/formatter/function.rs index 8207db5e486..ca905f3dcf8 100644 --- a/tooling/nargo_fmt/src/formatter/function.rs +++ b/tooling/nargo_fmt/src/formatter/function.rs @@ -19,10 +19,11 @@ pub(super) struct FunctionToFormat { pub(super) return_visibility: Visibility, pub(super) where_clause: Vec, pub(super) body: Option, + pub(super) skip_visibility: bool, } impl<'a> Formatter<'a> { - pub(super) fn format_function(&mut self, func: NoirFunction) { + pub(super) fn format_function(&mut self, func: NoirFunction, skip_visibility: bool) { self.format_function_impl(FunctionToFormat { attributes: func.def.attributes, visibility: func.def.visibility, @@ -33,6 +34,7 @@ impl<'a> Formatter<'a> { return_visibility: func.def.return_visibility, where_clause: func.def.where_clause, body: Some(func.def.body), + skip_visibility, }); } @@ -41,7 +43,7 @@ impl<'a> Formatter<'a> { self.format_attributes(func.attributes); self.write_indentation(); - self.format_function_modifiers(func.visibility); + self.format_function_modifiers(func.visibility, func.skip_visibility); self.write_keyword(Keyword::Fn); self.write_space(); self.write_identifier(func.name); @@ -94,7 +96,11 @@ impl<'a> Formatter<'a> { } } - pub(super) fn format_function_modifiers(&mut self, visibility: ItemVisibility) { + pub(super) fn format_function_modifiers( + &mut self, + visibility: ItemVisibility, + skip_visibility: bool, + ) { // For backwards compatibility, unconstrained might come before visibility. // We'll remember this but put it after the visibility. let unconstrained = if self.is_at_keyword(Keyword::Unconstrained) { @@ -105,7 +111,14 @@ impl<'a> Formatter<'a> { false }; - self.format_item_visibility(visibility); + if skip_visibility { + // The intention here is to format the visibility into a temporary buffer that is discarded + self.chunk_formatter().chunk(|formatter| { + formatter.format_item_visibility(visibility); + }); + } else { + self.format_item_visibility(visibility); + } if unconstrained { self.write("unconstrained "); diff --git a/tooling/nargo_fmt/src/formatter/impls.rs b/tooling/nargo_fmt/src/formatter/impls.rs index 1c2c25c9200..71548dd5efa 100644 --- a/tooling/nargo_fmt/src/formatter/impls.rs +++ b/tooling/nargo_fmt/src/formatter/impls.rs @@ -38,7 +38,9 @@ impl<'a> Formatter<'a> { if !doc_comments.is_empty() { self.format_outer_doc_comments(); } - self.format_function(method); + self.format_function( + method, false, // skip visibility + ); } self.skip_comments_and_whitespace(); diff --git a/tooling/nargo_fmt/src/formatter/item.rs b/tooling/nargo_fmt/src/formatter/item.rs index 521e476fe71..3365e52ec29 100644 --- a/tooling/nargo_fmt/src/formatter/item.rs +++ b/tooling/nargo_fmt/src/formatter/item.rs @@ -58,7 +58,10 @@ impl<'a> Formatter<'a> { ItemKind::Import(use_tree, item_visibility) => { self.format_import(use_tree, item_visibility); } - ItemKind::Function(noir_function) => self.format_function(noir_function), + ItemKind::Function(noir_function) => self.format_function( + noir_function, + false, // skip visibility + ), ItemKind::Struct(noir_struct) => self.format_struct(noir_struct), ItemKind::Trait(noir_trait) => self.format_trait(noir_trait), ItemKind::TraitImpl(noir_trait_impl) => self.format_trait_impl(noir_trait_impl), diff --git a/tooling/nargo_fmt/src/formatter/trait_impl.rs b/tooling/nargo_fmt/src/formatter/trait_impl.rs index b31da8a4101..5bb9a0d0025 100644 --- a/tooling/nargo_fmt/src/formatter/trait_impl.rs +++ b/tooling/nargo_fmt/src/formatter/trait_impl.rs @@ -1,8 +1,5 @@ use noirc_frontend::{ - ast::{ - FunctionDefinition, ItemVisibility, NoirFunction, NoirTraitImpl, Pattern, TraitImplItem, - TraitImplItemKind, - }, + ast::{NoirTraitImpl, Pattern, TraitImplItem, TraitImplItemKind}, token::{Keyword, Token}, }; @@ -69,12 +66,10 @@ impl<'a> Formatter<'a> { fn format_trait_impl_item(&mut self, item: TraitImplItem) { match item.kind { TraitImplItemKind::Function(noir_function) => { - // Trait impl functions are public, but there's no `pub` keyword in the source code, - // so to format it we pass a private one. - let def = - FunctionDefinition { visibility: ItemVisibility::Private, ..noir_function.def }; - let noir_function = NoirFunction { def, ..noir_function }; - self.format_function(noir_function); + self.format_function( + noir_function, + true, // skip visibility + ); } TraitImplItemKind::Constant(name, typ, value) => { let pattern = Pattern::Identifier(name); @@ -179,6 +174,22 @@ fn foo ( ) { } assert_format(src, expected); } + #[test] + fn format_trait_impl_function_with_visibility() { + let src = " mod moo { impl Foo for Bar { + /// Some doc comment +pub fn foo ( ) { } + } }"; + let expected = "mod moo { + impl Foo for Bar { + /// Some doc comment + fn foo() {} + } +} +"; + assert_format(src, expected); + } + #[test] fn format_trait_impl_constant_without_type() { let src = " mod moo { impl Foo for Bar { diff --git a/tooling/nargo_fmt/src/formatter/traits.rs b/tooling/nargo_fmt/src/formatter/traits.rs index 1f192be471e..175dcad6170 100644 --- a/tooling/nargo_fmt/src/formatter/traits.rs +++ b/tooling/nargo_fmt/src/formatter/traits.rs @@ -113,6 +113,7 @@ impl<'a> Formatter<'a> { return_visibility: Visibility::Private, where_clause, body, + skip_visibility: true, }; self.format_function_impl(func); } @@ -236,12 +237,12 @@ mod tests { fn format_trait_with_function_without_body() { let src = " mod moo { trait Foo { /// hello - pub fn foo ( ); + fn foo ( ); } }"; let expected = "mod moo { trait Foo { /// hello - pub fn foo(); + fn foo(); } } "; @@ -252,12 +253,12 @@ mod tests { fn format_trait_with_function_with_body() { let src = " mod moo { trait Foo { /// hello - pub fn foo ( ) { 1 } + fn foo ( ) { 1 } } }"; let expected = "mod moo { trait Foo { /// hello - pub fn foo() { + fn foo() { 1 } } @@ -270,12 +271,12 @@ mod tests { fn format_trait_with_function_with_params() { let src = " mod moo { trait Foo { /// hello - pub fn foo ( x : i32 , y : Field ); + fn foo ( x : i32 , y : Field ); } }"; let expected = "mod moo { trait Foo { /// hello - pub fn foo(x: i32, y: Field); + fn foo(x: i32, y: Field); } } "; @@ -298,6 +299,24 @@ mod tests { assert_format(src, expected); } + #[test] + fn format_trait_with_function_with_visibility() { + let src = " mod moo { trait Foo { + /// hello + pub fn foo ( ) { 1 } + } }"; + let expected = "mod moo { + trait Foo { + /// hello + fn foo() { + 1 + } + } +} +"; + assert_format(src, expected); + } + #[test] fn format_multiple_traits() { let src = " trait Foo {} diff --git a/tooling/nargo_fmt/src/lib.rs b/tooling/nargo_fmt/src/lib.rs index eda77e78c7c..2b55b86e975 100644 --- a/tooling/nargo_fmt/src/lib.rs +++ b/tooling/nargo_fmt/src/lib.rs @@ -66,6 +66,7 @@ pub(crate) fn assert_format_with_config(src: &str, expected: &str, config: Confi use noirc_frontend::parser; let (parsed_module, errors) = parser::parse_program(src); + let errors: Vec<_> = errors.into_iter().filter(|error| !error.is_warning()).collect(); if !errors.is_empty() { panic!("Expected no errors, got: {:?}", errors); }