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

chore!: Require types of globals to be specified #6592

Merged
merged 6 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
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 @@
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 @@
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 @@
| 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 Expand Up @@ -534,7 +600,7 @@
Self::Public => write!(f, "pub"),
Self::Private => write!(f, "priv"),
Self::CallData(id) => write!(f, "calldata{id}"),
Self::ReturnData => write!(f, "returndata"),

Check warning on line 603 in compiler/noirc_frontend/src/ast/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (returndata)
}
}
}
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 @@
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 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 Expand Up @@ -419,7 +418,7 @@
// TODO(https://github.com/noir-lang/noir/issues/6238):
// support non-u32 generics here
if !kind.unifies(&Kind::u32()) {
let error = TypeCheckError::EvaluatedGlobalIsntU32 {

Check warning on line 421 in compiler/noirc_frontend/src/elaborator/types.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
expected_kind: Kind::u32().to_string(),
expr_kind: kind.to_string(),
expr_span: path.span(),
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 (RHS inferred to have type `{expected_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(
format!("Globals must have a specified type (RHS inferred to have type `{expected_type}`)"),
String::new(),
*span,
michaeljklein marked this conversation as resolved.
Show resolved Hide resolved
)
},
ResolverError::SelfReferentialStruct { span } => {
Diagnostic::simple_error(
"Self-referential structs are not supported".into(),
Expand Down
75 changes: 71 additions & 4 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1304,7 +1304,21 @@
global B = A;
fn main() {}
"#;
assert_eq!(get_program_errors(src).len(), 1);

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::DependencyCycle { .. })
));
michaeljklein marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
Expand Down Expand Up @@ -1996,7 +2010,7 @@
}

// TODO(https://github.com/noir-lang/noir/issues/6238):
// The EvaluatedGlobalIsntU32 warning is a stopgap

Check warning on line 2013 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
// (originally from https://github.com/noir-lang/noir/issues/6125)
#[test]
fn numeric_generic_field_larger_than_u32() {
Expand All @@ -2013,7 +2027,7 @@
assert_eq!(errors.len(), 2);
assert!(matches!(
errors[0].0,
CompilationError::TypeError(TypeCheckError::EvaluatedGlobalIsntU32 { .. }),

Check warning on line 2030 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
));
assert!(matches!(
errors[1].0,
Expand All @@ -2022,7 +2036,7 @@
}

// TODO(https://github.com/noir-lang/noir/issues/6238):
// The EvaluatedGlobalIsntU32 warning is a stopgap

Check warning on line 2039 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
// (originally from https://github.com/noir-lang/noir/issues/6126)
#[test]
fn numeric_generic_field_arithmetic_larger_than_u32() {
Expand Down Expand Up @@ -2051,7 +2065,7 @@

assert!(matches!(
errors[0].0,
CompilationError::TypeError(TypeCheckError::EvaluatedGlobalIsntU32 { .. }),

Check warning on line 2068 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
));

assert!(matches!(
Expand Down Expand Up @@ -2187,7 +2201,7 @@
assert_eq!(errors.len(), 3);

// TODO(https://github.com/noir-lang/noir/issues/6238):
// The EvaluatedGlobalIsntU32 warning is a stopgap

Check warning on line 2204 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (Isnt)
assert!(matches!(
errors[0].0,
CompilationError::TypeError(TypeCheckError::EvaluatedGlobalIsntU32 { .. }),
Expand Down Expand Up @@ -3210,10 +3224,10 @@
}

#[test]
fn infer_globals_to_u32_from_type_use() {
fn dont_infer_globals_to_u32_from_type_use() {

Check warning on line 3227 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (dont)
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 +3237,59 @@
}
"#;

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() {

Check warning on line 3257 in compiler/noirc_frontend/src/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (dont)
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 +3753,7 @@
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
Loading