feat: add FunctionDef::set_return_visibility (#5941)

# Description

## Problem

Resolves #5902

## Summary


## Additional Context

There's a catch: if you pass `quote { hello world }` to the method, it
doesn't crash. The reason is that when we parse the visibility, if it's
not `pub` or one of the other non-private visibilities, we simply assume
it's private (in regular code then would come a type or `{` and that's
where it would fail to parse). I wanted to error in this case but
couldn't:
1. One idea I had was to check if we get a non-empty tokens stream. If
that's the case and we don't get a private visibility, it's an error.
Well, that worked, but I didn't know how to easily create a
`ParserError` for that.
2. Another idea is to have a specific parser to parse this or EOF... but
I tried a choice with `TokenKind::Token(Token::EOF)` and when it was
EOF, it said it was expecting EOF (kind of confusing).

Any ideas about the best way to do this?

We could also capture a follow-up issue for this, because merging this
unblocks some stuff.

## 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.

compiler/noirc_frontend/src/elaborator/lints.rs

@@ -1,20 +1,19 @@
 use crate::{
-    ast::FunctionKind,
+    ast::{FunctionKind, Ident},
     graph::CrateId,
     hir::{
         resolution::errors::{PubPosition, ResolverError},
         type_check::TypeCheckError,
     },
-    hir_def::expr::HirIdent,
+    hir_def::{expr::HirIdent, function::FuncMeta},
     macros_api::{
-        HirExpression, HirLiteral, NodeInterner, NoirFunction, Signedness, UnaryOp,
-        UnresolvedTypeData, Visibility,
+        HirExpression, HirLiteral, NodeInterner, NoirFunction, Signedness, UnaryOp, Visibility,
     },
-    node_interner::{DefinitionKind, ExprId, FuncId},
+    node_interner::{DefinitionKind, ExprId, FuncId, FunctionModifiers},
     Type,
 };
 
-use noirc_errors::Span;
+use noirc_errors::{Span, Spanned};
 
 pub(super) fn deprecated_function(interner: &NodeInterner, expr: ExprId) -> Option<TypeCheckError> {
     let HirExpression::Ident(HirIdent { location, id, impl_kind: _ }, _) =
@@ -39,16 +38,17 @@ pub(super) fn deprecated_function(interner: &NodeInterner, expr: ExprId) -> Opti
 /// Inline attributes are only relevant for constrained functions
 /// as all unconstrained functions are not inlined and so
 /// associated attributes are disallowed.
-pub(super) fn inlining_attributes(func: &NoirFunction) -> Option<ResolverError> {
-    if func.def.is_unconstrained {
-        let attributes = func.attributes().clone();
-
-        if attributes.is_no_predicates() {
-            Some(ResolverError::NoPredicatesAttributeOnUnconstrained {
-                ident: func.name_ident().clone(),
-            })
-        } else if attributes.is_foldable() {
-            Some(ResolverError::FoldAttributeOnUnconstrained { ident: func.name_ident().clone() })
+pub(super) fn inlining_attributes(
+    func: &FuncMeta,
+    modifiers: &FunctionModifiers,
+) -> Option<ResolverError> {
+    if modifiers.is_unconstrained {
+        if modifiers.attributes.is_no_predicates() {
+            let ident = func_meta_name_ident(func, modifiers);
+            Some(ResolverError::NoPredicatesAttributeOnUnconstrained { ident })
+        } else if modifiers.attributes.is_foldable() {
+            let ident = func_meta_name_ident(func, modifiers);
+            Some(ResolverError::FoldAttributeOnUnconstrained { ident })
         } else {
             None
         }
@@ -59,24 +59,30 @@ pub(super) fn inlining_attributes(func: &NoirFunction) -> Option<ResolverError>
 
 /// Attempting to define new low level (`#[builtin]` or `#[foreign]`) functions outside of the stdlib is disallowed.
 pub(super) fn low_level_function_outside_stdlib(
-    func: &NoirFunction,
+    func: &FuncMeta,
+    modifiers: &FunctionModifiers,
     crate_id: CrateId,
 ) -> Option<ResolverError> {
     let is_low_level_function =
-        func.attributes().function.as_ref().map_or(false, |func| func.is_low_level());
+        modifiers.attributes.function.as_ref().map_or(false, |func| func.is_low_level());
     if !crate_id.is_stdlib() && is_low_level_function {
-        Some(ResolverError::LowLevelFunctionOutsideOfStdlib { ident: func.name_ident().clone() })
+        let ident = func_meta_name_ident(func, modifiers);
+        Some(ResolverError::LowLevelFunctionOutsideOfStdlib { ident })
     } else {
         None
     }
 }
 
 /// Oracle definitions (functions with the `#[oracle]` attribute) must be marked as unconstrained.
-pub(super) fn oracle_not_marked_unconstrained(func: &NoirFunction) -> Option<ResolverError> {
+pub(super) fn oracle_not_marked_unconstrained(
+    func: &FuncMeta,
+    modifiers: &FunctionModifiers,
+) -> Option<ResolverError> {
     let is_oracle_function =
-        func.attributes().function.as_ref().map_or(false, |func| func.is_oracle());
-    if is_oracle_function && !func.def.is_unconstrained {
-        Some(ResolverError::OracleMarkedAsConstrained { ident: func.name_ident().clone() })
+        modifiers.attributes.function.as_ref().map_or(false, |func| func.is_oracle());
+    if is_oracle_function && !modifiers.is_unconstrained {
+        let ident = func_meta_name_ident(func, modifiers);
+        Some(ResolverError::OracleMarkedAsConstrained { ident })
     } else {
         None
     }
@@ -106,24 +112,26 @@ pub(super) fn oracle_called_from_constrained_function(
 }
 
 /// `pub` is required on return types for entry point functions
-pub(super) fn missing_pub(func: &NoirFunction, is_entry_point: bool) -> Option<ResolverError> {
-    if is_entry_point
-        && func.return_type().typ != UnresolvedTypeData::Unit
-        && func.def.return_visibility == Visibility::Private
+pub(super) fn missing_pub(func: &FuncMeta, modifiers: &FunctionModifiers) -> Option<ResolverError> {
+    if func.is_entry_point
+        && func.return_type() != &Type::Unit
+        && func.return_visibility == Visibility::Private
     {
-        Some(ResolverError::NecessaryPub { ident: func.name_ident().clone() })
+        let ident = func_meta_name_ident(func, modifiers);
+        Some(ResolverError::NecessaryPub { ident })
     } else {
         None
     }
 }
 
 /// `#[recursive]` attribute is only allowed for entry point functions
 pub(super) fn recursive_non_entrypoint_function(
-    func: &NoirFunction,
-    is_entry_point: bool,
+    func: &FuncMeta,
+    modifiers: &FunctionModifiers,
 ) -> Option<ResolverError> {
-    if !is_entry_point && func.kind == FunctionKind::Recursive {
-        Some(ResolverError::MisplacedRecursiveAttribute { ident: func.name_ident().clone() })
+    if !func.is_entry_point && func.kind == FunctionKind::Recursive {
+        let ident = func_meta_name_ident(func, modifiers);
+        Some(ResolverError::MisplacedRecursiveAttribute { ident })
     } else {
         None
     }
@@ -163,14 +171,13 @@ pub(super) fn unconstrained_function_return(
 ///
 /// Application of `pub` to other functions is not meaningful and is a mistake.
 pub(super) fn unnecessary_pub_return(
-    func: &NoirFunction,
+    func: &FuncMeta,
+    modifiers: &FunctionModifiers,
     is_entry_point: bool,
 ) -> Option<ResolverError> {
-    if !is_entry_point && func.def.return_visibility == Visibility::Public {
-        Some(ResolverError::UnnecessaryPub {
-            ident: func.name_ident().clone(),
-            position: PubPosition::ReturnType,
-        })
+    if !is_entry_point && func.return_visibility == Visibility::Public {
+        let ident = func_meta_name_ident(func, modifiers);
+        Some(ResolverError::UnnecessaryPub { ident, position: PubPosition::ReturnType })
     } else {
         None
     }
@@ -252,3 +259,7 @@ pub(crate) fn overflowing_int(
 
     errors
 }
+
+fn func_meta_name_ident(func: &FuncMeta, modifiers: &FunctionModifiers) -> Ident {
+    Ident(Spanned::from(func.name.location.span, modifiers.name.clone()))
+}

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -26,7 +26,8 @@ use crate::{
         SecondaryAttribute, StructId,
     },
     node_interner::{
-        DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, ReferenceId, TraitId, TypeAliasId,
+        DefinitionKind, DependencyId, ExprId, FuncId, FunctionModifiers, GlobalId, ReferenceId,
+        TraitId, TypeAliasId,
     },
     token::CustomAtrribute,
     Shared, Type, TypeVariable,
@@ -417,6 +418,10 @@ impl<'context> Elaborator<'context> {
         self.trait_bounds = func_meta.trait_constraints.clone();
         self.function_context.push(FunctionContext::default());
 
+        let modifiers = self.interner.function_modifiers(&id).clone();
+
+        self.run_function_lints(&func_meta, &modifiers);
+
         self.introduce_generics_into_scope(func_meta.all_generics.clone());
 
         // The DefinitionIds for each parameter were already created in define_function_meta
@@ -731,20 +736,6 @@ impl<'context> Elaborator<'context> {
 
         let is_entry_point = self.is_entry_point_function(func, in_contract);
 
-        self.run_lint(|_| lints::inlining_attributes(func).map(Into::into));
-        self.run_lint(|_| lints::missing_pub(func, is_entry_point).map(Into::into));
-        self.run_lint(|elaborator| {
-            lints::unnecessary_pub_return(func, elaborator.pub_allowed(func, in_contract))
-                .map(Into::into)
-        });
-        self.run_lint(|_| lints::oracle_not_marked_unconstrained(func).map(Into::into));
-        self.run_lint(|elaborator| {
-            lints::low_level_function_outside_stdlib(func, elaborator.crate_id).map(Into::into)
-        });
-        self.run_lint(|_| {
-            lints::recursive_non_entrypoint_function(func, is_entry_point).map(Into::into)
-        });
-
         // Both the #[fold] and #[no_predicates] alter a function's inline type and code generation in similar ways.
         // In certain cases such as type checking (for which the following flag will be used) both attributes
         // indicate we should code generate in the same way. Thus, we unify the attributes into one flag here.
@@ -858,6 +849,23 @@ impl<'context> Elaborator<'context> {
         self.current_item = None;
     }
 
+    fn run_function_lints(&mut self, func: &FuncMeta, modifiers: &FunctionModifiers) {
+        self.run_lint(|_| lints::inlining_attributes(func, modifiers).map(Into::into));
+        self.run_lint(|_| lints::missing_pub(func, modifiers).map(Into::into));
+        self.run_lint(|_| {
+            let pub_allowed = func.is_entry_point || modifiers.attributes.is_foldable();
+            lints::unnecessary_pub_return(func, modifiers, pub_allowed).map(Into::into)
+        });
+        self.run_lint(|_| lints::oracle_not_marked_unconstrained(func, modifiers).map(Into::into));
+        self.run_lint(|elaborator| {
+            lints::low_level_function_outside_stdlib(func, modifiers, elaborator.crate_id)
+                .map(Into::into)
+        });
+        self.run_lint(|_| {
+            lints::recursive_non_entrypoint_function(func, modifiers).map(Into::into)
+        });
+    }
+
     /// Only sized types are valid to be used as main's parameters or the parameters to a contract
     /// function. If the given type is not sized (e.g. contains a slice or NamedGeneric type), an
     /// error is issued.

compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs

@@ -6,7 +6,7 @@ use std::{
 use acvm::{AcirField, FieldElement};
 use builtin_helpers::{
     block_expression_to_value, check_argument_count, check_function_not_yet_resolved,
-    check_one_argument, check_three_arguments, check_two_arguments, get_expr, get_field,
+    check_one_argument, check_three_arguments, check_two_arguments, get_bool, get_expr, get_field,
     get_format_string, get_function_def, get_module, get_quoted, get_slice, get_struct,
     get_trait_constraint, get_trait_def, get_trait_impl, get_tuple, get_type, get_typed_expr,
     get_u32, get_unresolved_type, hir_pattern_to_tokens, mutate_func_meta_type, parse,
@@ -110,6 +110,9 @@ impl<'local, 'context> Interpreter<'local, 'context> {
             "function_def_set_return_type" => {
                 function_def_set_return_type(self, arguments, location)
             }
+            "function_def_set_return_public" => {
+                function_def_set_return_public(self, arguments, location)
+            }
             "module_functions" => module_functions(self, arguments, location),
             "module_has_named_attribute" => module_has_named_attribute(self, arguments, location),
             "module_is_contract" => module_is_contract(self, arguments, location),
@@ -1935,6 +1938,25 @@ fn function_def_set_return_type(
     Ok(Value::Unit)
 }
 
+// fn set_return_public(self, public: bool)
+fn function_def_set_return_public(
+    interpreter: &mut Interpreter,
+    arguments: Vec<(Value, Location)>,
+    location: Location,
+) -> IResult<Value> {
+    let (self_argument, public) = check_two_arguments(arguments, location)?;
+
+    let func_id = get_function_def(self_argument)?;
+    check_function_not_yet_resolved(interpreter, func_id, location)?;
+
+    let public = get_bool(public)?;
+
+    let func_meta = interpreter.elaborator.interner.function_meta_mut(&func_id);
+    func_meta.return_visibility = if public { Visibility::Public } else { Visibility::Private };
+
+    Ok(Value::Unit)
+}
+
 // fn functions(self) -> [FunctionDefinition]
 fn module_functions(
     interpreter: &Interpreter,

compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs

@@ -90,6 +90,13 @@ pub(crate) fn get_array(
     }
 }
 
+pub(crate) fn get_bool((value, location): (Value, Location)) -> IResult<bool> {
+    match value {
+        Value::Bool(value) => Ok(value),
+        value => type_mismatch(value, Type::Bool, location),
+    }
+}
+
 pub(crate) fn get_slice(
     interner: &NodeInterner,
     (value, location): (Value, Location),

compiler/noirc_frontend/src/parser/mod.rs

@@ -26,7 +26,8 @@ use noirc_errors::Span;
 pub use parser::path::path_no_turbofish;
 pub use parser::traits::trait_bound;
 pub use parser::{
-    block, expression, fresh_statement, lvalue, parse_program, parse_type, pattern, top_level_items,
+    block, expression, fresh_statement, lvalue, parse_program, parse_type, pattern,
+    top_level_items, visibility,
 };
 
 #[derive(Debug, Clone)]

compiler/noirc_frontend/src/parser/parser.rs

@@ -28,7 +28,8 @@ use self::primitives::{keyword, macro_quote_marker, mutable_reference, variable}
 use self::types::{generic_type_args, maybe_comp_time};
 use attributes::{attributes, inner_attribute, validate_secondary_attributes};
 pub use types::parse_type;
-use visibility::visibility_modifier;
+use visibility::item_visibility;
+pub use visibility::visibility;
 
 use super::{
     foldl_with_span, labels::ParsingRuleLabel, parameter_name_recovery, parameter_recovery,
@@ -459,7 +460,7 @@ fn module_declaration() -> impl NoirParser<TopLevelStatement> {
 }
 
 fn use_statement() -> impl NoirParser<TopLevelStatement> {
-    visibility_modifier()
+    item_visibility()
         .then_ignore(keyword(Keyword::Use))
         .then(use_tree())
         .map(|(visibility, use_tree)| TopLevelStatement::Import(use_tree, visibility))
@@ -737,15 +738,6 @@ fn call_data() -> impl NoirParser<Visibility> {
     })
 }
 
-fn optional_visibility() -> impl NoirParser<Visibility> {
-    keyword(Keyword::Pub)
-        .map(|_| Visibility::Public)
-        .or(call_data())
-        .or(keyword(Keyword::ReturnData).map(|_| Visibility::ReturnData))
-        .or_not()
-        .map(|opt| opt.unwrap_or(Visibility::Private))
-}
-
 pub fn expression() -> impl ExprParser {
     recursive(|expr| {
         expression_with_precedence(

compiler/noirc_frontend/src/parser/parser/function.rs

@@ -1,10 +1,10 @@
 use super::{
     attributes::{attributes, validate_attributes},
-    block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_visibility,
-    parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern,
+    block, fresh_statement, ident, keyword, maybe_comp_time, nothing, parameter_name_recovery,
+    parameter_recovery, parenthesized, parse_type, pattern,
     primitives::token_kind,
     self_parameter,
-    visibility::visibility_modifier,
+    visibility::{item_visibility, visibility},
     where_clause, NoirParser,
 };
 use crate::token::{Keyword, Token, TokenKind};
@@ -79,13 +79,9 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser<NoirFunct
 ///
 /// returns (is_unconstrained, visibility) for whether each keyword was present
 fn function_modifiers() -> impl NoirParser<(bool, ItemVisibility, bool)> {
-    keyword(Keyword::Unconstrained)
-        .or_not()
-        .then(visibility_modifier())
-        .then(maybe_comp_time())
-        .map(|((unconstrained, visibility), comptime)| {
-            (unconstrained.is_some(), visibility, comptime)
-        })
+    keyword(Keyword::Unconstrained).or_not().then(item_visibility()).then(maybe_comp_time()).map(
+        |((unconstrained, visibility), comptime)| (unconstrained.is_some(), visibility, comptime),
+    )
 }
 
 pub(super) fn numeric_generic() -> impl NoirParser<UnresolvedGeneric> {
@@ -142,14 +138,12 @@ pub(super) fn generics() -> impl NoirParser<UnresolvedGenerics> {
 
 pub(super) fn function_return_type() -> impl NoirParser<(Visibility, FunctionReturnType)> {
     #[allow(deprecated)]
-    just(Token::Arrow)
-        .ignore_then(optional_visibility())
-        .then(spanned(parse_type()))
-        .or_not()
-        .map_with_span(|ret, span| match ret {
+    just(Token::Arrow).ignore_then(visibility()).then(spanned(parse_type())).or_not().map_with_span(
+        |ret, span| match ret {
             Some((visibility, (ty, _))) => (visibility, FunctionReturnType::Ty(ty)),
             None => (Visibility::Private, FunctionReturnType::Default(span)),
-        })
+        },
+    )
 }
 
 fn function_parameters<'a>(allow_self: bool) -> impl NoirParser<Vec<Param>> + 'a {
@@ -158,7 +152,7 @@ fn function_parameters<'a>(allow_self: bool) -> impl NoirParser<Vec<Param>> + 'a
     let full_parameter = pattern()
         .recover_via(parameter_name_recovery())
         .then_ignore(just(Token::Colon))
-        .then(optional_visibility())
+        .then(visibility())
         .then(typ)
         .map_with_span(|((pattern, visibility), typ), span| Param {
             visibility,