feat: visibility for globals (#6161)

# Description

## Problem

Part of #4515

## Summary

## Additional Context

I left all of the stdlib globals as private except one which needed to
be `pub(crate)` to avoid a warning. I don't know if some of these
globals should actually be public.

## Documentation

Check one:
- [ ] No documentation needed.
- [x] 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.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

compiler/noirc_frontend/src/ast/visitor.rs

@@ -500,7 +500,7 @@ impl Item {
                 noir_trait_impl.accept(self.span, visitor);
             }
             ItemKind::Impl(type_impl) => type_impl.accept(self.span, visitor),
-            ItemKind::Global(let_statement) => {
+            ItemKind::Global(let_statement, _visibility) => {
                 if visitor.visit_global(let_statement, self.span) {
                     let_statement.accept(visitor);
                 }

compiler/noirc_frontend/src/elaborator/comptime.rs

@@ -439,11 +439,12 @@ impl<'context> Elaborator<'context> {
                     resolved_trait_generics: Vec::new(),
                 });
             }
-            TopLevelStatementKind::Global(global) => {
+            TopLevelStatementKind::Global(global, visibility) => {
                 let (global, error) = dc_mod::collect_global(
                     self.interner,
                     self.def_maps.get_mut(&self.crate_id).unwrap(),
                     Documented::new(global, item.doc_comments),
+                    visibility,
                     self.file,
                     self.local_module,
                     self.crate_id,

compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -139,15 +139,16 @@ impl<'a> ModCollector<'a> {
     fn collect_globals(
         &mut self,
         context: &mut Context,
-        globals: Vec<Documented<LetStatement>>,
+        globals: Vec<(Documented<LetStatement>, ItemVisibility)>,
         crate_id: CrateId,
     ) -> Vec<(CompilationError, fm::FileId)> {
         let mut errors = vec![];
-        for global in globals {
+        for (global, visibility) in globals {
             let (global, error) = collect_global(
                 &mut context.def_interner,
                 &mut self.def_collector.def_map,
                 global,
+                visibility,
                 self.file_id,
                 self.module_id,
                 crate_id,
@@ -515,7 +516,7 @@ impl<'a> ModCollector<'a> {
 
                         if let Err((first_def, second_def)) = self.def_collector.def_map.modules
                             [trait_id.0.local_id.0]
-                            .declare_global(name.clone(), global_id)
+                            .declare_global(name.clone(), ItemVisibility::Public, global_id)
                         {
                             let error = DefCollectorErrorKind::Duplicate {
                                 typ: DuplicateType::TraitAssociatedConst,
@@ -1160,6 +1161,7 @@ pub(crate) fn collect_global(
     interner: &mut NodeInterner,
     def_map: &mut CrateDefMap,
     global: Documented<LetStatement>,
+    visibility: ItemVisibility,
     file_id: FileId,
     module_id: LocalModuleId,
     crate_id: CrateId,
@@ -1180,7 +1182,15 @@ pub(crate) fn collect_global(
     );
 
     // Add the statement to the scope so its path can be looked up later
-    let result = def_map.modules[module_id.0].declare_global(name, global_id);
+    let result = def_map.modules[module_id.0].declare_global(name.clone(), visibility, global_id);
+
+    let parent_module_id = ModuleId { krate: crate_id, local_id: module_id };
+    interner.usage_tracker.add_unused_item(
+        parent_module_id,
+        name,
+        UnusedItem::Global(global_id),
+        visibility,
+    );
 
     let error = result.err().map(|(first_def, second_def)| {
         let err =

compiler/noirc_frontend/src/hir/def_map/module_data.rs

@@ -96,8 +96,13 @@ impl ModuleData {
         self.definitions.remove_definition(name);
     }
 
-    pub fn declare_global(&mut self, name: Ident, id: GlobalId) -> Result<(), (Ident, Ident)> {
-        self.declare(name, ItemVisibility::Public, id.into(), None)
+    pub fn declare_global(
+        &mut self,
+        name: Ident,
+        visibility: ItemVisibility,
+        id: GlobalId,
+    ) -> Result<(), (Ident, Ident)> {
+        self.declare(name, visibility, id.into(), None)
     }
 
     pub fn declare_struct(

compiler/noirc_frontend/src/parser/mod.rs

@@ -47,7 +47,7 @@ pub enum TopLevelStatementKind {
     Impl(TypeImpl),
     TypeAlias(NoirTypeAlias),
     SubModule(ParsedSubModule),
-    Global(LetStatement),
+    Global(LetStatement, ItemVisibility),
     InnerAttribute(SecondaryAttribute),
     Error,
 }
@@ -64,7 +64,7 @@ impl TopLevelStatementKind {
             TopLevelStatementKind::Impl(i) => Some(ItemKind::Impl(i)),
             TopLevelStatementKind::TypeAlias(t) => Some(ItemKind::TypeAlias(t)),
             TopLevelStatementKind::SubModule(s) => Some(ItemKind::Submodules(s)),
-            TopLevelStatementKind::Global(c) => Some(ItemKind::Global(c)),
+            TopLevelStatementKind::Global(c, visibility) => Some(ItemKind::Global(c, visibility)),
             TopLevelStatementKind::InnerAttribute(a) => Some(ItemKind::InnerAttribute(a)),
             TopLevelStatementKind::Error => None,
         }
@@ -249,7 +249,7 @@ pub struct SortedModule {
     pub trait_impls: Vec<NoirTraitImpl>,
     pub impls: Vec<TypeImpl>,
     pub type_aliases: Vec<Documented<NoirTypeAlias>>,
-    pub globals: Vec<Documented<LetStatement>>,
+    pub globals: Vec<(Documented<LetStatement>, ItemVisibility)>,
 
     /// Module declarations like `mod foo;`
     pub module_decls: Vec<Documented<ModuleDeclaration>>,
@@ -271,7 +271,7 @@ impl std::fmt::Display for SortedModule {
             write!(f, "{import}")?;
         }
 
-        for global_const in &self.globals {
+        for (global_const, _visibility) in &self.globals {
             write!(f, "{global_const}")?;
         }
 
@@ -321,7 +321,9 @@ impl ParsedModule {
                 ItemKind::TypeAlias(type_alias) => {
                     module.push_type_alias(type_alias, item.doc_comments);
                 }
-                ItemKind::Global(global) => module.push_global(global, item.doc_comments),
+                ItemKind::Global(global, visibility) => {
+                    module.push_global(global, visibility, item.doc_comments);
+                }
                 ItemKind::ModuleDecl(mod_name) => {
                     module.push_module_decl(mod_name, item.doc_comments);
                 }
@@ -354,7 +356,7 @@ pub enum ItemKind {
     TraitImpl(NoirTraitImpl),
     Impl(TypeImpl),
     TypeAlias(NoirTypeAlias),
-    Global(LetStatement),
+    Global(LetStatement, ItemVisibility),
     ModuleDecl(ModuleDeclaration),
     Submodules(ParsedSubModule),
     InnerAttribute(SecondaryAttribute),
@@ -438,8 +440,13 @@ impl SortedModule {
         self.submodules.push(Documented::new(submodule, doc_comments));
     }
 
-    fn push_global(&mut self, global: LetStatement, doc_comments: Vec<String>) {
-        self.globals.push(Documented::new(global, doc_comments));
+    fn push_global(
+        &mut self,
+        global: LetStatement,
+        visibility: ItemVisibility,
+        doc_comments: Vec<String>,
+    ) {
+        self.globals.push((Documented::new(global, doc_comments), visibility));
     }
 }
 
@@ -526,19 +533,23 @@ impl std::fmt::Display for TopLevelStatementKind {
             TopLevelStatementKind::Function(fun) => fun.fmt(f),
             TopLevelStatementKind::Module(m) => m.fmt(f),
             TopLevelStatementKind::Import(tree, visibility) => {
-                if visibility == &ItemVisibility::Private {
-                    write!(f, "use {tree}")
-                } else {
-                    write!(f, "{visibility} use {tree}")
+                if visibility != &ItemVisibility::Private {
+                    write!(f, "{visibility} ")?;
                 }
+                write!(f, "use {tree}")
             }
             TopLevelStatementKind::Trait(t) => t.fmt(f),
             TopLevelStatementKind::TraitImpl(i) => i.fmt(f),
             TopLevelStatementKind::Struct(s) => s.fmt(f),
             TopLevelStatementKind::Impl(i) => i.fmt(f),
             TopLevelStatementKind::TypeAlias(t) => t.fmt(f),
             TopLevelStatementKind::SubModule(s) => s.fmt(f),
-            TopLevelStatementKind::Global(c) => c.fmt(f),
+            TopLevelStatementKind::Global(c, visibility) => {
+                if visibility != &ItemVisibility::Private {
+                    write!(f, "{visibility} ")?;
+                }
+                c.fmt(f)
+            }
             TopLevelStatementKind::InnerAttribute(a) => write!(f, "#![{}]", a),
             TopLevelStatementKind::Error => write!(f, "error"),
         }

compiler/noirc_frontend/src/parser/parser.rs

@@ -220,6 +220,7 @@ fn implementation() -> impl NoirParser<TopLevelStatementKind> {
 /// global_declaration: 'global' ident global_type_annotation '=' literal
 fn global_declaration() -> impl NoirParser<TopLevelStatementKind> {
     let p = attributes::attributes()
+        .then(item_visibility())
         .then(maybe_comp_time())
         .then(spanned(keyword(Keyword::Mut)).or_not())
         .then_ignore(keyword(Keyword::Global).labelled(ParsingRuleLabel::Global))
@@ -229,7 +230,9 @@ fn global_declaration() -> impl NoirParser<TopLevelStatementKind> {
     let p = then_commit_ignore(p, just(Token::Assign));
     let p = then_commit(p, expression());
     p.validate(
-        |(((((attributes, comptime), mutable), mut pattern), r#type), expression), span, emit| {
+        |((((((attributes, visibility), comptime), mutable), mut pattern), r#type), expression),
+         span,
+         emit| {
             let global_attributes =
                 attributes::validate_secondary_attributes(attributes, span, emit);
 
@@ -239,10 +242,19 @@ fn global_declaration() -> impl NoirParser<TopLevelStatementKind> {
                 let span = mut_span.merge(pattern.span());
                 pattern = Pattern::Mutable(Box::new(pattern), span, false);
             }
-            LetStatement { pattern, r#type, comptime, expression, attributes: global_attributes }
+            (
+                LetStatement {
+                    pattern,
+                    r#type,
+                    comptime,
+                    expression,
+                    attributes: global_attributes,
+                },
+                visibility,
+            )
         },
     )
-    .map(TopLevelStatementKind::Global)
+    .map(|(let_statement, visibility)| TopLevelStatementKind::Global(let_statement, visibility))
 }
 
 /// submodule: 'mod' ident '{' module '}'

compiler/noirc_frontend/src/tests.rs

@@ -1376,7 +1376,9 @@ fn ban_mutable_globals() {
     // Mutable globals are only allowed in a comptime context
     let src = r#"
         mut global FOO: Field = 0;
-        fn main() {}
+        fn main() {
+            let _ = FOO; // silence FOO never used warning
+        }
     "#;
     assert_eq!(get_program_errors(src).len(), 1);
 }

compiler/noirc_frontend/src/tests/unused_items.rs

@@ -239,3 +239,26 @@ fn errors_if_type_alias_aliases_more_private_type_in_generic() {
     assert_eq!(typ, "Foo");
     assert_eq!(item, "Bar");
 }
+
+#[test]
+fn warns_on_unused_global() {
+    let src = r#"
+    global foo = 1;
+    global bar = 1;
+
+    fn main() {
+        let _ = bar;
+    }
+    "#;
+
+    let errors = get_program_errors(src);
+    assert_eq!(errors.len(), 1);
+
+    let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0
+    else {
+        panic!("Expected an unused item warning");
+    };
+
+    assert_eq!(ident.to_string(), "foo");
+    assert_eq!(item.item_type(), "global");
+}

compiler/noirc_frontend/src/usage_tracker.rs

@@ -4,7 +4,7 @@ use crate::{
     ast::{Ident, ItemVisibility},
     hir::def_map::ModuleId,
     macros_api::StructId,
-    node_interner::{FuncId, TraitId, TypeAliasId},
+    node_interner::{FuncId, GlobalId, TraitId, TypeAliasId},
 };
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
@@ -14,6 +14,7 @@ pub enum UnusedItem {
     Struct(StructId),
     Trait(TraitId),
     TypeAlias(TypeAliasId),
+    Global(GlobalId),
 }
 
 impl UnusedItem {
@@ -24,6 +25,7 @@ impl UnusedItem {
             UnusedItem::Struct(_) => "struct",
             UnusedItem::Trait(_) => "trait",
             UnusedItem::TypeAlias(_) => "type alias",
+            UnusedItem::Global(_) => "global",
         }
     }
 }

docs/docs/noir/concepts/globals.md

@@ -70,3 +70,13 @@ fn foo() -> [Field; 100] { ... }
 This is usually fine since Noir will generally optimize any function call that does not
 refer to a program input into a constant. It should be kept in mind however, if the called
 function performs side-effects like `println`, as these will still occur on each use.
+
+### Visibility
+
+By default, like functions, globals are private to the module the exist in. You can use `pub`
+to make the global public or `pub(crate)` to make it public to just its crate:
+
+```rust
+// This global is now public
+pub global N = 5;
+```

noir_stdlib/src/field/bn254.nr

@@ -4,7 +4,7 @@ use crate::runtime::is_unconstrained;
 global PLO: Field = 53438638232309528389504892708671455233;
 global PHI: Field = 64323764613183177041862057485226039389;
 
-global TWO_POW_128: Field = 0x100000000000000000000000000000000;
+pub(crate) global TWO_POW_128: Field = 0x100000000000000000000000000000000;
 
 // Decomposes a single field into two 16 byte fields.
 fn compute_decomposition(x: Field) -> (Field, Field) {

tooling/nargo_fmt/src/visitor/item.rs

@@ -276,7 +276,7 @@ impl super::FmtVisitor<'_> {
                 ItemKind::Struct(_)
                 | ItemKind::Trait(_)
                 | ItemKind::TypeAlias(_)
-                | ItemKind::Global(_)
+                | ItemKind::Global(..)
                 | ItemKind::ModuleDecl(_)
                 | ItemKind::InnerAttribute(_) => {
                     self.push_rewrite(self.slice(span).to_string(), span);