Skip to content

Commit

Permalink
feat(improve): Remove scan through globals (#6282)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #5001 

## Summary\*

Removes the search for a global by local module ID and name when
elaborating a global variable declaration. Instead simply looks up the
global by the ID that we have in the definition we are elaborating. In
fact the elaboration starts from a global ID of an unresolved global.

I could be way off the mark on whether this is the right thing to do.

## Additional Context

I found a couple of things odd:
* `NodeInterner::push_global` pushes a `DefinitionKind::Global(id)` with
the exact ID that the `GlobalInfo` will have, ie. it is self-referential
(just an observation). This is called during the collection of
unresolved globals.
* `Elaborator::add_global_variable_decl` only looks for globals by
`local_id` and `name`, but _not_ by `definition`, even though at this
point we know that the `definition` must contain a `GlobalId`, otherwise
we would have stayed in `Elaborator::add_variable_decl`.
* if a global cannot be found, it calls `NodeInterner::push_definition`
with the `definition` in the parameters; since we know it contains a
`GlobalId` and that the same ID must have been inserted in `push_global`
as well, it will now be part of two definitions at different locations -
not sure exactly what this means or looks like in code

I added some assertions in the commits to check that during the tests on
CI, the `global_id` and the `definition`, and any results found by the
lookup by local+name, are always consistent, which is why in the end I
thought that if the only thing we can find is the one we can get by ID,
why not get just get it?


## 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.
  • Loading branch information
aakoshh authored Oct 11, 2024
1 parent c8a91a5 commit fd91913
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 49 deletions.
1 change: 0 additions & 1 deletion compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
56 changes: 10 additions & 46 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ impl<'context> Elaborator<'context> {
None,
&mut Vec::new(),
warn_if_unused,
None,
)
}

Expand All @@ -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,
Expand All @@ -59,7 +57,6 @@ impl<'context> Elaborator<'context> {
None,
created_ids,
warn_if_unused,
global_id,
)
}

Expand All @@ -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) => {
Expand All @@ -82,7 +78,7 @@ impl<'context> Elaborator<'context> {
(Some(_), DefinitionKind::Local(_)) => DefinitionKind::Local(None),
(_, other) => other,
};
let ident = if let Some(global_id) = 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);
Expand Down Expand Up @@ -112,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)
Expand Down Expand Up @@ -144,7 +139,6 @@ impl<'context> Elaborator<'context> {
mutable,
new_definitions,
warn_if_unused,
global_id,
)
});
let location = Location::new(span, self.file);
Expand All @@ -168,7 +162,6 @@ impl<'context> Elaborator<'context> {
mutable,
new_definitions,
warn_if_unused,
global_id,
)
}
}
Expand Down Expand Up @@ -283,7 +276,6 @@ impl<'context> Elaborator<'context> {
mutable,
new_definitions,
true, // warn_if_unused
None,
);

if unseen_fields.contains(&field) {
Expand Down Expand Up @@ -329,8 +321,8 @@ impl<'context> Elaborator<'context> {
warn_if_unused: bool,
definition: DefinitionKind,
) -> HirIdent {
if definition.is_global() {
return self.add_global_variable_decl(name, definition);
if let DefinitionKind::Global(global_id) = definition {
return self.add_global_variable_decl(name, global_id);
}

let location = Location::new(name.span(), self.file);
Expand Down Expand Up @@ -375,47 +367,19 @@ impl<'context> Elaborator<'context> {
}
}

pub fn add_global_variable_decl(
&mut self,
name: Ident,
definition: DefinitionKind,
) -> HirIdent {
let comptime = self.in_comptime_context();
pub fn add_global_variable_decl(&mut self, name: Ident, global_id: GlobalId) -> HirIdent {
let scope = self.scopes.get_mut_scope();

// This check is necessary to maintain the same definition ids in the interner. Currently, each function uses a new resolver that has its own ScopeForest and thus global scope.
// We must first check whether an existing definition ID has been inserted as otherwise there will be multiple definitions for the same global statement.
// This leads to an error in evaluation where the wrong definition ID is selected when evaluating a statement using the global. The check below prevents this error.
let mut global_id = None;
let global = self.interner.get_all_globals();
for global_info in global {
if global_info.local_id == self.local_module && global_info.ident == name {
global_id = Some(global_info.id);
}
}

let (ident, resolver_meta) = if let Some(id) = global_id {
let global = self.interner.get_global(id);
let hir_ident = HirIdent::non_trait_method(global.definition_id, global.location);
let ident = hir_ident.clone();
let resolver_meta = ResolverMeta { num_times_used: 0, ident, warn_if_unused: true };
(hir_ident, resolver_meta)
} else {
let location = Location::new(name.span(), self.file);
let name = name.0.contents.clone();
let id = self.interner.push_definition(name, false, comptime, definition, location);
let ident = HirIdent::non_trait_method(id, location);
let resolver_meta =
ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused: true };
(ident, resolver_meta)
};
let global = self.interner.get_global(global_id);
let ident = HirIdent::non_trait_method(global.definition_id, global.location);
let resolver_meta =
ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused: true };

let old_global_value = scope.add_key_value(name.0.contents.clone(), resolver_meta);
if let Some(old_global_value) = old_global_value {
self.push_err(ResolverError::DuplicateDefinition {
name: name.0.contents.clone(),
first_span: old_global_value.ident.location.span,
second_span: name.span(),
name: name.0.contents,
first_span: old_global_value.ident.location.span,
});
}
ident
Expand Down
1 change: 0 additions & 1 deletion compiler/noirc_frontend/src/elaborator/statements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ impl<'context> Elaborator<'context> {
definition,
&mut Vec::new(),
warn_if_unused,
global_id,
);

let attributes = let_stmt.attributes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2462,7 +2462,6 @@ fn function_def_set_parameters(
DefinitionKind::Local(None),
&mut parameter_idents,
true, // warn_if_unused
None,
)
});

Expand Down

0 comments on commit fd91913

Please sign in to comment.