Skip to content

Commit

Permalink
chore!: Require types of globals to be specified (#6592)
Browse files Browse the repository at this point in the history
  • Loading branch information
michaeljklein authored Nov 22, 2024
1 parent 3c361c9 commit 8ff4efd
Show file tree
Hide file tree
Showing 50 changed files with 251 additions and 102 deletions.
66 changes: 66 additions & 0 deletions compiler/noirc_frontend/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,14 @@ impl GenericTypeArgs {
pub fn is_empty(&self) -> bool {
self.ordered_args.is_empty() && self.named_args.is_empty()
}

fn contains_unspecified(&self) -> bool {
let ordered_args_contains_unspecified =
self.ordered_args.iter().any(|ordered_arg| ordered_arg.contains_unspecified());
let named_args_contains_unspecified =
self.named_args.iter().any(|(_name, named_arg)| named_arg.contains_unspecified());
ordered_args_contains_unspecified || named_args_contains_unspecified
}
}

impl From<Vec<GenericTypeArg>> for GenericTypeArgs {
Expand Down Expand Up @@ -375,6 +383,10 @@ impl UnresolvedType {
let typ = UnresolvedTypeData::Named(path, generic_type_args, true);
UnresolvedType { typ, span }
}

pub(crate) fn contains_unspecified(&self) -> bool {
self.typ.contains_unspecified()
}
}

impl UnresolvedTypeData {
Expand All @@ -395,6 +407,47 @@ impl UnresolvedTypeData {
pub fn with_span(&self, span: Span) -> UnresolvedType {
UnresolvedType { typ: self.clone(), span }
}

fn contains_unspecified(&self) -> bool {
match self {
UnresolvedTypeData::Array(typ, length) => {
typ.contains_unspecified() || length.contains_unspecified()
}
UnresolvedTypeData::Slice(typ) => typ.contains_unspecified(),
UnresolvedTypeData::Expression(expr) => expr.contains_unspecified(),
UnresolvedTypeData::String(length) => length.contains_unspecified(),
UnresolvedTypeData::FormatString(typ, length) => {
typ.contains_unspecified() || length.contains_unspecified()
}
UnresolvedTypeData::Parenthesized(typ) => typ.contains_unspecified(),
UnresolvedTypeData::Named(path, args, _is_synthesized) => {
// '_' is unspecified
let path_is_wildcard = path.is_wildcard();
let an_arg_is_unresolved = args.contains_unspecified();
path_is_wildcard || an_arg_is_unresolved
}
UnresolvedTypeData::TraitAsType(_path, args) => args.contains_unspecified(),
UnresolvedTypeData::MutableReference(typ) => typ.contains_unspecified(),
UnresolvedTypeData::Tuple(args) => args.iter().any(|arg| arg.contains_unspecified()),
UnresolvedTypeData::Function(args, ret, env, _unconstrained) => {
let args_contains_unspecified = args.iter().any(|arg| arg.contains_unspecified());
args_contains_unspecified
|| ret.contains_unspecified()
|| env.contains_unspecified()
}
UnresolvedTypeData::Unspecified => true,

UnresolvedTypeData::FieldElement
| UnresolvedTypeData::Integer(_, _)
| UnresolvedTypeData::Bool
| UnresolvedTypeData::Unit
| UnresolvedTypeData::Quoted(_)
| UnresolvedTypeData::AsTraitPath(_)
| UnresolvedTypeData::Resolved(_)
| UnresolvedTypeData::Interned(_)
| UnresolvedTypeData::Error => false,
}
}
}

#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash, PartialOrd, Ord)]
Expand Down Expand Up @@ -494,6 +547,19 @@ impl UnresolvedTypeExpression {
| BinaryOpKind::Modulo
)
}

fn contains_unspecified(&self) -> bool {
match self {
// '_' is unspecified
UnresolvedTypeExpression::Variable(path) => path.is_wildcard(),
UnresolvedTypeExpression::BinaryOperation(lhs, _op, rhs, _span) => {
lhs.contains_unspecified() || rhs.contains_unspecified()
}
UnresolvedTypeExpression::Constant(_, _) | UnresolvedTypeExpression::AsTraitPath(_) => {
false
}
}
}
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
Expand Down
7 changes: 7 additions & 0 deletions compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ use crate::token::{SecondaryAttribute, Token};
/// for an identifier that already failed to parse.
pub const ERROR_IDENT: &str = "$error";

/// This is used to represent an UnresolvedTypeData::Unspecified in a Path
pub const WILDCARD_TYPE: &str = "_";

#[derive(Debug, PartialEq, Eq, Clone)]
pub struct Statement {
pub kind: StatementKind,
Expand Down Expand Up @@ -483,6 +486,10 @@ impl Path {
self.segments.first().cloned().map(|segment| segment.ident)
}

pub(crate) fn is_wildcard(&self) -> bool {
self.to_ident().map(|ident| ident.0.contents) == Some(WILDCARD_TYPE.to_string())
}

pub fn is_empty(&self) -> bool {
self.segments.is_empty() && self.kind == PathKind::Plain
}
Expand Down
9 changes: 9 additions & 0 deletions compiler/noirc_frontend/src/elaborator/statements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,17 @@ impl<'context> Elaborator<'context> {
) -> (HirStatement, Type) {
let expr_span = let_stmt.expression.span;
let (expression, expr_type) = self.elaborate_expression(let_stmt.expression);
let type_contains_unspecified = let_stmt.r#type.contains_unspecified();
let annotated_type = self.resolve_inferred_type(let_stmt.r#type);

// Require the top-level of a global's type to be fully-specified
if type_contains_unspecified && global_id.is_some() {
let span = expr_span;
let expected_type = annotated_type.clone();
let error = ResolverError::UnspecifiedGlobalType { span, expected_type };
self.push_err(error);
}

let definition = match global_id {
None => DefinitionKind::Local(Some(expression)),
Some(id) => DefinitionKind::Global(id),
Expand Down
3 changes: 1 addition & 2 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
ast::{
AsTraitPath, BinaryOpKind, GenericTypeArgs, Ident, IntegerBitSize, Path, PathKind,
Signedness, UnaryOp, UnresolvedGeneric, UnresolvedGenerics, UnresolvedType,
UnresolvedTypeData, UnresolvedTypeExpression,
UnresolvedTypeData, UnresolvedTypeExpression, WILDCARD_TYPE,
},
hir::{
comptime::{Interpreter, Value},
Expand Down Expand Up @@ -40,7 +40,6 @@ use crate::{
use super::{lints, path_resolution::PathResolutionItem, Elaborator};

pub const SELF_TYPE_NAME: &str = "Self";
pub const WILDCARD_TYPE: &str = "_";

pub(super) struct TraitPathResolution {
pub(super) method: TraitMethod,
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 @@ -101,6 +101,8 @@ pub enum ResolverError {
JumpOutsideLoop { is_break: bool, span: Span },
#[error("Only `comptime` globals can be mutable")]
MutableGlobal { span: Span },
#[error("Globals must have a specified type")]
UnspecifiedGlobalType { span: Span, expected_type: Type },
#[error("Self-referential structs are not supported")]
SelfReferentialStruct { span: Span },
#[error("#[no_predicates] attribute is only allowed on constrained functions")]
Expand Down Expand Up @@ -431,6 +433,13 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
*span,
)
},
ResolverError::UnspecifiedGlobalType { span, expected_type } => {
Diagnostic::simple_error(
"Globals must have a specified type".to_string(),
format!("Inferred type is `{expected_type}`"),
*span,
)
},
ResolverError::SelfReferentialStruct { span } => {
Diagnostic::simple_error(
"Self-referential structs are not supported".into(),
Expand Down
71 changes: 65 additions & 6 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1300,11 +1300,17 @@ fn lambda$f1(mut env$l1: (Field)) -> Field {
#[test]
fn deny_cyclic_globals() {
let src = r#"
global A = B;
global B = A;
global A: u32 = B;
global B: u32 = A;
fn main() {}
"#;
assert_eq!(get_program_errors(src).len(), 1);

let errors = get_program_errors(src);
assert_eq!(errors.len(), 1);
assert!(matches!(
errors[0].0,
CompilationError::ResolverError(ResolverError::DependencyCycle { .. })
));
}

#[test]
Expand Down Expand Up @@ -3210,10 +3216,10 @@ fn as_trait_path_syntax_no_impl() {
}

#[test]
fn infer_globals_to_u32_from_type_use() {
fn dont_infer_globals_to_u32_from_type_use() {
let src = r#"
global ARRAY_LEN = 3;
global STR_LEN = 2;
global STR_LEN: _ = 2;
global FMT_STR_LEN = 2;
fn main() {
Expand All @@ -3223,6 +3229,59 @@ fn infer_globals_to_u32_from_type_use() {
}
"#;

let errors = get_program_errors(src);
assert_eq!(errors.len(), 3);
assert!(matches!(
errors[0].0,
CompilationError::ResolverError(ResolverError::UnspecifiedGlobalType { .. })
));
assert!(matches!(
errors[1].0,
CompilationError::ResolverError(ResolverError::UnspecifiedGlobalType { .. })
));
assert!(matches!(
errors[2].0,
CompilationError::ResolverError(ResolverError::UnspecifiedGlobalType { .. })
));
}

#[test]
fn dont_infer_partial_global_types() {
let src = r#"
pub global ARRAY: [Field; _] = [0; 3];
pub global NESTED_ARRAY: [[Field; _]; 3] = [[]; 3];
pub global STR: str<_> = "hi";
pub global NESTED_STR: [str<_>] = &["hi"];
pub global FMT_STR: fmtstr<_, _> = f"hi {ARRAY}";
pub global TUPLE_WITH_MULTIPLE: ([str<_>], [[Field; _]; 3]) = (&["hi"], [[]; 3]);
fn main() { }
"#;

let errors = get_program_errors(src);
assert_eq!(errors.len(), 6);
for (error, _file_id) in errors {
assert!(matches!(
error,
CompilationError::ResolverError(ResolverError::UnspecifiedGlobalType { .. })
));
}
}

#[test]
fn u32_globals_as_sizes_in_types() {
let src = r#"
global ARRAY_LEN: u32 = 3;
global STR_LEN: u32 = 2;
global FMT_STR_LEN: u32 = 2;
fn main() {
let _a: [u32; ARRAY_LEN] = [1, 2, 3];
let _b: str<STR_LEN> = "hi";
let _c: fmtstr<FMT_STR_LEN, _> = f"hi";
}
"#;

let errors = get_program_errors(src);
assert_eq!(errors.len(), 0);
}
Expand Down Expand Up @@ -3686,7 +3745,7 @@ fn allows_struct_with_generic_infix_type_as_main_input_3() {
x: [u64; N * 2],
}
global N = 9;
global N: u32 = 9;
fn main(_x: Foo<N * 2>) {}
"#;
Expand Down
6 changes: 3 additions & 3 deletions compiler/noirc_frontend/src/tests/unused_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,8 @@ fn errors_on_unused_type_alias() {
#[test]
fn warns_on_unused_global() {
let src = r#"
global foo = 1;
global bar = 1;
global foo: u32 = 1;
global bar: Field = 1;
fn main() {
let _ = bar;
Expand All @@ -216,7 +216,7 @@ fn does_not_warn_on_unused_global_if_it_has_an_abi_attribute() {
let src = r#"
contract foo {
#[abi(notes)]
global bar = 1;
global bar: u64 = 1;
}
fn main() {}
Expand Down
16 changes: 8 additions & 8 deletions docs/docs/noir/concepts/globals.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ sidebar_position: 8
## Globals


Noir supports global variables. The global's type can be inferred by the compiler entirely:
Noir supports global variables. The global's type must be specified by the user:

```rust
global N = 5; // Same as `global N: Field = 5`
global N: Field = 5;

global TUPLE = (3, 2);
global TUPLE: (Field, Field) = (3, 2);

fn main() {
assert(N == 5);
Expand All @@ -28,7 +28,7 @@ fn main() {
Globals can be defined as any expression, so long as they don't depend on themselves - otherwise there would be a dependency cycle! For example:

```rust
global T = foo(T); // dependency error
global T: u32 = foo(T); // dependency error
```

:::
Expand All @@ -47,7 +47,7 @@ fn main(y : [Field; N]) {
A global from another module can be imported or referenced externally like any other name:

```rust
global N = 20;
global N: Field = 20;

fn main() {
assert(my_submodule::N != N);
Expand All @@ -62,7 +62,7 @@ When a global is used, Noir replaces the name with its definition on each occurr
This means globals defined using function calls will repeat the call each time they're used:

```rust
global RESULT = foo();
global RESULT: [Field; 100] = foo();

fn foo() -> [Field; 100] { ... }
```
Expand All @@ -78,5 +78,5 @@ 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;
```
pub global N: u32 = 5;
```
12 changes: 6 additions & 6 deletions noir_stdlib/src/bigint.nr
Original file line number Diff line number Diff line change
@@ -1,27 +1,27 @@
use crate::cmp::Eq;
use crate::ops::{Add, Div, Mul, Sub};

global bn254_fq = &[
global bn254_fq: [u8] = &[
0x47, 0xFD, 0x7C, 0xD8, 0x16, 0x8C, 0x20, 0x3C, 0x8d, 0xca, 0x71, 0x68, 0x91, 0x6a, 0x81, 0x97,
0x5d, 0x58, 0x81, 0x81, 0xb6, 0x45, 0x50, 0xb8, 0x29, 0xa0, 0x31, 0xe1, 0x72, 0x4e, 0x64, 0x30,
];
global bn254_fr = &[
global bn254_fr: [u8] = &[
1, 0, 0, 240, 147, 245, 225, 67, 145, 112, 185, 121, 72, 232, 51, 40, 93, 88, 129, 129, 182, 69,
80, 184, 41, 160, 49, 225, 114, 78, 100, 48,
];
global secpk1_fr = &[
global secpk1_fr: [u8] = &[
0x41, 0x41, 0x36, 0xD0, 0x8C, 0x5E, 0xD2, 0xBF, 0x3B, 0xA0, 0x48, 0xAF, 0xE6, 0xDC, 0xAE, 0xBA,
0xFE, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
];
global secpk1_fq = &[
global secpk1_fq: [u8] = &[
0x2F, 0xFC, 0xFF, 0xFF, 0xFE, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
];
global secpr1_fq = &[
global secpr1_fq: [u8] = &[
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0xFF, 0xFF, 0xFF, 0xFF,
];
global secpr1_fr = &[
global secpr1_fr: [u8] = &[
81, 37, 99, 252, 194, 202, 185, 243, 132, 158, 23, 167, 173, 250, 230, 188, 255, 255, 255, 255,
255, 255, 255, 255, 0, 0, 0, 0, 255, 255, 255, 255,
];
Expand Down
4 changes: 2 additions & 2 deletions noir_stdlib/src/collections/map.nr
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use crate::option::Option;
// We use load factor alpha_max = 0.75.
// Upon exceeding it, assert will fail in order to inform the user
// about performance degradation, so that he can adjust the capacity.
global MAX_LOAD_FACTOR_NUMERATOR = 3;
global MAX_LOAD_FACTOR_DEN0MINATOR = 4;
global MAX_LOAD_FACTOR_NUMERATOR: u32 = 3;
global MAX_LOAD_FACTOR_DEN0MINATOR: u32 = 4;

/// `HashMap<Key, Value, MaxLen, Hasher>` is used to efficiently store and look up key-value pairs.
///
Expand Down
Loading

0 comments on commit 8ff4efd

Please sign in to comment.