Skip to content

Commit

Permalink
fix: Fix using lazily elaborated comptime globals (#5995)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #5991

## Summary\*

Fixes two issues:
1. The original linked issue where elaborating a function early would
cause any globals to be "locked in" in a sense.
2. The fact that we were never actually mutating underlying values in
the interpreter.

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

- [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.
  • Loading branch information
jfecher authored Sep 11, 2024
1 parent 3c3ed1e commit f6f493c
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 15 deletions.
9 changes: 0 additions & 9 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,15 +513,6 @@ impl<'context> Elaborator<'context> {
let typ = self.type_check_variable(expr, id, generics);
self.interner.push_expr_type(id, typ.clone());

// Comptime variables must be replaced with their values
if let Some(definition) = self.interner.try_definition(definition_id) {
if definition.comptime && !self.in_comptime_context() {
let mut interpreter = self.setup_interpreter();
let value = interpreter.evaluate(id);
return self.inline_comptime_value(value, span);
}
}

(id, typ)
}

Expand Down
23 changes: 17 additions & 6 deletions compiler/noirc_frontend/src/hir/comptime/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,14 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {

for scope in self.elaborator.interner.comptime_scopes.iter_mut().rev() {
if let Entry::Occupied(mut entry) = scope.entry(id) {
entry.insert(argument);
match entry.get() {
Value::Pointer(reference, true) => {
*reference.borrow_mut() = argument;
}
_ => {
entry.insert(argument);
}
}
return Ok(());
}
}
Expand Down Expand Up @@ -527,12 +534,13 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
DefinitionKind::Local(_) => self.lookup(&ident),
DefinitionKind::Global(global_id) => {
// Avoid resetting the value if it is already known
if let Ok(value) = self.lookup(&ident) {
Ok(value)
if let Some(value) = &self.elaborator.interner.get_global(*global_id).value {
Ok(value.clone())
} else {
let crate_of_global = self.elaborator.interner.get_global(*global_id).crate_id;
let global_id = *global_id;
let crate_of_global = self.elaborator.interner.get_global(global_id).crate_id;
let let_ =
self.elaborator.interner.get_global_let_statement(*global_id).ok_or_else(
self.elaborator.interner.get_global_let_statement(global_id).ok_or_else(
|| {
let location = self.elaborator.interner.expr_location(&id);
InterpreterError::VariableNotInScope { location }
Expand All @@ -542,7 +550,10 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
if let_.comptime || crate_of_global != self.crate_id {
self.evaluate_let(let_.clone())?;
}
self.lookup(&ident)

let value = self.lookup(&ident)?;
self.elaborator.interner.get_global_mut(global_id).value = Some(value.clone());
Ok(value)
}
}
DefinitionKind::GenericType(type_variable) => {
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_frontend/src/hir/comptime/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,11 @@ impl Value {
}
Value::Quoted(tokens) => HirExpression::Unquote(add_token_spans(tokens, location.span)),
Value::TypedExpr(TypedExpr::ExprId(expr_id)) => interner.expression(&expr_id),
// Only convert pointers with auto_deref = true. These are mutable variables
// and we don't need to wrap them in `&mut`.
Value::Pointer(element, true) => {
return element.unwrap_or_clone().into_hir_expression(interner, location);
}
Value::TypedExpr(TypedExpr::StmtId(..))
| Value::Expr(..)
| Value::Pointer(..)
Expand Down
10 changes: 10 additions & 0 deletions compiler/noirc_frontend/src/hir_def/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,16 @@ impl<T> Shared<T> {
pub fn borrow_mut(&self) -> std::cell::RefMut<T> {
self.0.borrow_mut()
}

pub fn unwrap_or_clone(self) -> T
where
T: Clone,
{
match Rc::try_unwrap(self.0) {
Ok(elem) => elem.into_inner(),
Err(rc) => rc.as_ref().clone().into_inner(),
}
}
}

/// A restricted subset of binary operators useable on
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "comptime_globals_regression"
type = "bin"
authors = [""]
compiler_version = ">=0.33.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
comptime mut global COUNTER = 0;

fn main() {
comptime
{
increment()
};
comptime
{
increment()
};

assert_eq(get_counter(), 2);
}

fn get_counter() -> Field {
COUNTER
}

comptime fn increment() {
COUNTER += 1;
assert_eq(get_counter(), COUNTER);
}

0 comments on commit f6f493c

Please sign in to comment.