From d1c31890edceba1d8b7cc10c7a2ae22c9b6dfa40 Mon Sep 17 00:00:00 2001
From: Akosh Farkash <aakoshh@gmail.com>
Date: Fri, 11 Oct 2024 17:03:02 +0100
Subject: [PATCH] chore(refactor): Remove optional global_id from methods that
 can get it from the definition (#6283)

# Description

## Problem\*

Followup to https://github.com/noir-lang/noir/pull/6282

## Summary\*
Some methods in the `Elaborator` received a `global_id:
Option<GlobalId>` and a `definition: DefinitionKind` parameter. We found
that `Elaborator::elaborate_let` actually created `definition` to carry
the same global ID as it was given, so `global_id: Some(id)` and
`definition: DefinitionKind::Global(id)` always carried the same value.
We added an
[assertion](https://github.com/noir-lang/noir/pull/6282/commits/1d2bd36ab5fb38ebab28b8262dbf13f8020ee461)
that checked that they are never different.

The next logical step is to remove the possibility of confusion by only
carrying the ID in the definition.

## Additional Context

## 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\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
---
 compiler/noirc_frontend/src/elaborator/mod.rs        |  1 -
 compiler/noirc_frontend/src/elaborator/patterns.rs   | 12 +-----------
 compiler/noirc_frontend/src/elaborator/statements.rs |  1 -
 .../src/hir/comptime/interpreter/builtin.rs          |  1 -
 4 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs
index a9173621fc7..74847e27a99 100644
--- a/compiler/noirc_frontend/src/elaborator/mod.rs
+++ b/compiler/noirc_frontend/src/elaborator/mod.rs
@@ -760,7 +760,6 @@ impl<'context> Elaborator<'context> {
                 DefinitionKind::Local(None),
                 &mut parameter_idents,
                 true, // warn_if_unused
-                None,
             );
 
             parameters.push((pattern, typ.clone(), visibility));
diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs
index f82244d34fc..8eb1d3fee70 100644
--- a/compiler/noirc_frontend/src/elaborator/patterns.rs
+++ b/compiler/noirc_frontend/src/elaborator/patterns.rs
@@ -37,7 +37,6 @@ impl<'context> Elaborator<'context> {
             None,
             &mut Vec::new(),
             warn_if_unused,
-            None,
         )
     }
 
@@ -50,7 +49,6 @@ impl<'context> Elaborator<'context> {
         definition_kind: DefinitionKind,
         created_ids: &mut Vec<HirIdent>,
         warn_if_unused: bool,
-        global_id: Option<GlobalId>,
     ) -> HirPattern {
         self.elaborate_pattern_mut(
             pattern,
@@ -59,7 +57,6 @@ impl<'context> Elaborator<'context> {
             None,
             created_ids,
             warn_if_unused,
-            global_id,
         )
     }
 
@@ -72,7 +69,6 @@ impl<'context> Elaborator<'context> {
         mutable: Option<Span>,
         new_definitions: &mut Vec<HirIdent>,
         warn_if_unused: bool,
-        global_id: Option<GlobalId>,
     ) -> HirPattern {
         match pattern {
             Pattern::Identifier(name) => {
@@ -82,9 +78,7 @@ impl<'context> Elaborator<'context> {
                     (Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None),
                     (_, other) => other,
                 };
-                let ident = if let Some(global_id) = global_id {
-                    // Sanity check that we don't have conflicting globals.
-                    assert_eq!(definition, DefinitionKind::Global(global_id));
+                let ident = if let DefinitionKind::Global(global_id) = definition {
                     // Globals don't need to be added to scope, they're already in the def_maps
                     let id = self.interner.get_global(global_id).definition_id;
                     let location = Location::new(name.span(), self.file);
@@ -114,7 +108,6 @@ impl<'context> Elaborator<'context> {
                     Some(span),
                     new_definitions,
                     warn_if_unused,
-                    global_id,
                 );
                 let location = Location::new(span, self.file);
                 HirPattern::Mutable(Box::new(pattern), location)
@@ -146,7 +139,6 @@ impl<'context> Elaborator<'context> {
                         mutable,
                         new_definitions,
                         warn_if_unused,
-                        global_id,
                     )
                 });
                 let location = Location::new(span, self.file);
@@ -170,7 +162,6 @@ impl<'context> Elaborator<'context> {
                     mutable,
                     new_definitions,
                     warn_if_unused,
-                    global_id,
                 )
             }
         }
@@ -285,7 +276,6 @@ impl<'context> Elaborator<'context> {
                 mutable,
                 new_definitions,
                 true, // warn_if_unused
-                None,
             );
 
             if unseen_fields.contains(&field) {
diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs
index 0be71e39587..238160e5aa4 100644
--- a/compiler/noirc_frontend/src/elaborator/statements.rs
+++ b/compiler/noirc_frontend/src/elaborator/statements.rs
@@ -110,7 +110,6 @@ impl<'context> Elaborator<'context> {
             definition,
             &mut Vec::new(),
             warn_if_unused,
-            global_id,
         );
 
         let attributes = let_stmt.attributes;
diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs
index 170cd67e146..3329e9c526a 100644
--- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs
+++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs
@@ -2462,7 +2462,6 @@ fn function_def_set_parameters(
                 DefinitionKind::Local(None),
                 &mut parameter_idents,
                 true, // warn_if_unused
-                None,
             )
         });