Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(experimental): Implement comptime mut globals #4926

Merged
merged 5 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions aztec_macros/src/utils/hir_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@
module_id,
file_id,
global.attributes.clone(),
false,
);

// Add the statement to the scope so its path can be looked up later
Expand Down Expand Up @@ -357,7 +358,7 @@
}
}

pub fn get_global_numberic_const(

Check warning on line 361 in aztec_macros/src/utils/hir_utils.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (numberic)
context: &HirContext,
const_name: &str,
) -> Result<u128, MacroError> {
Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,8 @@ impl Pattern {
pub fn name_ident(&self) -> &Ident {
match self {
Pattern::Identifier(name_ident) => name_ident,
_ => panic!("only the identifier pattern can return a name"),
Pattern::Mutable(pattern, ..) => pattern.name_ident(),
_ => panic!("Only the Identifier or Mutable patterns can return a name"),
}
}

Expand Down
11 changes: 8 additions & 3 deletions compiler/noirc_frontend/src/hir/comptime/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,14 @@ impl<'a> Interpreter<'a> {
DefinitionKind::Local(_) => self.lookup(&ident),
DefinitionKind::Global(global_id) => {
// Don't need to check let_.comptime, we can evaluate non-comptime globals too.
let let_ = self.interner.get_global_let_statement(*global_id).unwrap();
self.evaluate_let(let_)?;
self.lookup(&ident)
// Avoid resetting the value if it is already known
if let Ok(value) = self.lookup(&ident) {
Ok(value)
} else {
let let_ = self.interner.get_global_let_statement(*global_id).unwrap();
self.evaluate_let(let_)?;
self.lookup(&ident)
}
}
DefinitionKind::GenericType(type_variable) => {
let value = match &*type_variable.borrow() {
Expand Down
14 changes: 14 additions & 0 deletions compiler/noirc_frontend/src/hir/comptime/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,20 @@ fn mutating_arrays() {
assert_eq!(result, Value::U8(22));
}

#[test]
fn mutate_in_new_scope() {
let program = "fn main() -> pub u8 {
let mut x = 0;
x += 1;
{
x += 1;
}
x
}";
let result = interpret(program, vec!["main".into()]);
assert_eq!(result, Value::U8(2));
}

#[test]
fn for_loop() {
let program = "fn main() -> pub u8 {
Expand Down
5 changes: 4 additions & 1 deletion compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

use crate::ast::{
FunctionDefinition, Ident, ItemVisibility, LetStatement, ModuleDeclaration, NoirFunction,
NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl,
NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Pattern, TraitImplItem, TraitItem,
TypeImpl,
};
use crate::{
graph::CrateId,
Expand Down Expand Up @@ -109,6 +110,7 @@
self.module_id,
self.file_id,
global.attributes.clone(),
matches!(global.pattern, Pattern::Mutable { .. }),
);

// Add the statement to the scope so its path can be looked up later
Expand Down Expand Up @@ -463,6 +465,7 @@
trait_id.0.local_id,
self.file_id,
vec![],
false,
);

if let Err((first_def, second_def)) = self.def_collector.def_map.modules
Expand All @@ -478,7 +481,7 @@
}
}
TraitItem::Type { name } => {
// TODO(nickysn or alexvitkov): implement context.def_interner.push_empty_type_alias and get an id, instead of using TypeAliasId::dummy_id()

Check warning on line 484 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (nickysn)

Check warning on line 484 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (alexvitkov)
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[trait_id.0.local_id.0]
.declare_type_alias(name.clone(), TypeAliasId::dummy_id())
Expand Down
9 changes: 9 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
#[error("#[recursive] attribute is only allowed on entry points to a program")]
MisplacedRecursiveAttribute { ident: Ident },
#[error("#[abi(tag)] attribute is only allowed in contracts")]
AbiAttributeOusideContract { span: Span },

Check warning on line 80 in compiler/noirc_frontend/src/hir/resolution/errors.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Ouside)
#[error("Usage of the `#[foreign]` or `#[builtin]` function attributes are not allowed outside of the Noir standard library")]
LowLevelFunctionOutsideOfStdlib { ident: Ident },
#[error("Dependency cycle found, '{item}' recursively depends on itself: {cycle} ")]
Expand All @@ -86,6 +86,8 @@
JumpInConstrainedFn { is_break: bool, span: Span },
#[error("break/continue are only allowed within loops")]
JumpOutsideLoop { is_break: bool, span: Span },
#[error("Only `comptime` globals can be mutable")]
MutableGlobal { span: Span },
#[error("Self-referential structs are not supported")]
SelfReferentialStruct { span: Span },
#[error("#[inline(tag)] attribute is only allowed on constrained functions")]
Expand Down Expand Up @@ -346,6 +348,13 @@
*span,
)
},
ResolverError::MutableGlobal { span } => {
Diagnostic::simple_error(
"Only `comptime` globals may be mutable".into(),
String::new(),
*span,
)
},
ResolverError::SelfReferentialStruct { span } => {
Diagnostic::simple_error(
"Self-referential structs are not supported".into(),
Expand Down
11 changes: 7 additions & 4 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1222,15 +1222,18 @@ impl<'a> Resolver<'a> {
) -> HirStatement {
self.current_item = Some(DependencyId::Global(global_id));
let expression = self.resolve_expression(let_stmt.expression);
let global_id = self.interner.next_global_id();
let definition = DefinitionKind::Global(global_id);

if !self.in_contract
&& let_stmt.attributes.iter().any(|attr| matches!(attr, SecondaryAttribute::Abi(_)))
{
self.push_err(ResolverError::AbiAttributeOusideContract {
span: let_stmt.pattern.span(),
});
let span = let_stmt.pattern.span();
self.push_err(ResolverError::AbiAttributeOusideContract { span });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is an old spelling error. Could we change it to Outside everywhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this off here: #4933

}

if !let_stmt.comptime && matches!(let_stmt.pattern, Pattern::Mutable(..)) {
let span = let_stmt.pattern.span();
self.push_err(ResolverError::MutableGlobal { span });
}

HirStatement::Let(HirLetStatement {
Expand Down
10 changes: 8 additions & 2 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -653,11 +653,13 @@ impl NodeInterner {
let_statement: StmtId,
file: FileId,
attributes: Vec<SecondaryAttribute>,
mutable: bool,
) -> GlobalId {
let id = GlobalId(self.globals.len());
let location = Location::new(ident.span(), file);
let name = ident.to_string();
let definition_id = self.push_definition(name, false, DefinitionKind::Global(id), location);
let definition_id =
self.push_definition(name, mutable, DefinitionKind::Global(id), location);

self.globals.push(GlobalInfo {
id,
Expand All @@ -682,9 +684,13 @@ impl NodeInterner {
local_id: LocalModuleId,
file: FileId,
attributes: Vec<SecondaryAttribute>,
mutable: bool,
) -> GlobalId {
let statement = self.push_stmt(HirStatement::Error);
self.push_global(name, local_id, statement, file, attributes)
let span = name.span();
let id = self.push_global(name, local_id, statement, file, attributes, mutable);
self.push_statement_location(statement, span, file);
id
}

/// Intern an empty function.
Expand Down
19 changes: 15 additions & 4 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,16 +235,27 @@ fn implementation() -> impl NoirParser<TopLevelStatement> {
fn global_declaration() -> impl NoirParser<TopLevelStatement> {
let p = attributes::attributes()
.then(maybe_comp_time())
.then(spanned(keyword(Keyword::Mut)).or_not())
.then_ignore(keyword(Keyword::Global).labelled(ParsingRuleLabel::Global))
.then(ident().map(Pattern::Identifier));

let p = then_commit(p, optional_type_annotation());
let p = then_commit_ignore(p, just(Token::Assign));
let p = then_commit(p, expression());
p.validate(|((((attributes, comptime), pattern), r#type), expression), span, emit| {
let global_attributes = attributes::validate_secondary_attributes(attributes, span, emit);
LetStatement { pattern, r#type, comptime, expression, attributes: global_attributes }
})
p.validate(
|(((((attributes, comptime), mutable), mut pattern), r#type), expression), span, emit| {
let global_attributes =
attributes::validate_secondary_attributes(attributes, span, emit);

// Only comptime globals are allowed to be mutable, but we always parse the `mut`
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
// and throw the error in name resolution.
if let Some((_, mut_span)) = mutable {
let span = mut_span.merge(pattern.span());
pattern = Pattern::Mutable(Box::new(pattern), span, false);
}
LetStatement { pattern, r#type, comptime, expression, attributes: global_attributes }
},
)
.map(TopLevelStatement::Global)
}

Expand Down
9 changes: 9 additions & 0 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1284,6 +1284,15 @@ fn lambda$f1(mut env$l1: (Field)) -> Field {
}

#[test]
fn ban_mutable_globals() {
// Mutable globals are only allowed in a comptime context
let src = r#"
mut global FOO: Field = 0;
fn main() {}
"#;
assert_eq!(get_program_errors(src).len(), 1);
}

fn deny_inline_attribute_on_unconstrained() {
let src = r#"
#[inline(never)]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "comptime_mut_global"
type = "bin"
authors = [""]
compiler_version = ">=0.28.0"

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

comptime fn get_unique_id() -> Field {
let id = COUNTER;
COUNTER += 1;
id
}

fn id1() -> Field {
comptime
{
get_unique_id()
}
}

fn id2() -> Field {
comptime
{
get_unique_id()
}
}

fn main() {
// Order of comptime evaluation between functions isn't guaranteed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this guaranteed? Of course this would be a follow-up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// so we don't know if (id1 == 0 && id2 == 1) or if (id1 == 1 && id2 == 0).
// we only know they are not equal
let id1 = id1();
let id2 = id2();
assert(id1 != id2);
}