feat: warn on trait method visibility (#6923)

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);

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)
                 }

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]

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);

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);
         }
 

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<UnresolvedTraitConstraint>,
     pub(super) body: Option<BlockExpression>,
+    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 ");

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();

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),

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 {  

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 {}

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);
     }