Skip to content

Commit

Permalink
chore!: Ban nested slices (#4018)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #4017 

## Summary\*

After this PR #3979 and an
internal discussion we have decided to temporarily block nested slices.

This PR adds a check in the frontend and in the SSA against nested
slices. The check in the frontend makes sure any struct fields with a
nested slice (but without generics) are not blocked as well as any type
declarations with nested slices. In order to account for generics in
structs we also have a checked array codegen that makes sure we do not
have a nested slice.

## Additional Context

The actual nested slice code in the ACIR gen is somewhat intertwined so
I felt it would be best for a separate PR which will be a followup to
this one.

## Documentation\*

Check one:
- [] No documentation needed.
- [X] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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
vezenovm committed Jan 16, 2024
1 parent d6a16d0 commit f8a1fb7
Show file tree
Hide file tree
Showing 21 changed files with 170 additions and 542 deletions.
5 changes: 4 additions & 1 deletion compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ pub enum RuntimeError {
UnknownLoopBound { call_stack: CallStack },
#[error("Argument is not constant")]
AssertConstantFailed { call_stack: CallStack },
#[error("Nested slices are not supported")]
NestedSlice { call_stack: CallStack },
}

// We avoid showing the actual lhs and rhs since most of the time they are just 0
Expand Down Expand Up @@ -129,7 +131,8 @@ impl RuntimeError {
| RuntimeError::UnknownLoopBound { call_stack }
| RuntimeError::AssertConstantFailed { call_stack }
| RuntimeError::IntegerOutOfBounds { call_stack, .. }
| RuntimeError::UnsupportedIntegerSize { call_stack, .. } => call_stack,
| RuntimeError::UnsupportedIntegerSize { call_stack, .. }
| RuntimeError::NestedSlice { call_stack, .. } => call_stack,
}
}
}
Expand Down
20 changes: 17 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,14 @@ impl<'a> FunctionContext<'a> {

let typ = Self::convert_type(&array.typ).flatten();
Ok(match array.typ {
ast::Type::Array(_, _) => self.codegen_array(elements, typ[0].clone()),
ast::Type::Array(_, _) => {
self.codegen_array_checked(elements, typ[0].clone())?
}
ast::Type::Slice(_) => {
let slice_length =
self.builder.field_constant(array.contents.len() as u128);

let slice_contents = self.codegen_array(elements, typ[1].clone());
let slice_contents =
self.codegen_array_checked(elements, typ[1].clone())?;
Tree::Branch(vec![slice_length.into(), slice_contents])
}
_ => unreachable!(
Expand Down Expand Up @@ -231,6 +233,18 @@ impl<'a> FunctionContext<'a> {
self.codegen_array(elements, typ)
}

// Codegen an array but make sure that we do not have a nested slice
fn codegen_array_checked(
&mut self,
elements: Vec<Values>,
typ: Type,
) -> Result<Values, RuntimeError> {
if typ.is_nested_slice() {
return Err(RuntimeError::NestedSlice { call_stack: self.builder.get_call_stack() });
}
Ok(self.codegen_array(elements, typ))
}

/// Codegen an array by allocating enough space for each element and inserting separate
/// store instructions until each element is stored. The store instructions will be separated
/// by add instructions to calculate the new offset address to store to next.
Expand Down
7 changes: 7 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ pub enum ResolverError {
NonCrateFunctionCalled { name: String, span: Span },
#[error("Only sized types may be used in the entry point to a program")]
InvalidTypeForEntryPoint { span: Span },
#[error("Nested slices are not supported")]
NestedSlices { span: Span },
}

impl ResolverError {
Expand Down Expand Up @@ -304,6 +306,11 @@ impl From<ResolverError> for Diagnostic {
ResolverError::InvalidTypeForEntryPoint { span } => Diagnostic::simple_error(
"Only sized types may be used in the entry point to a program".to_string(),
"Slices, references, or any type containing them may not be used in main or a contract function".to_string(), span),
ResolverError::NestedSlices { span } => Diagnostic::simple_error(
"Nested slices are not supported".into(),
"Try to use a constant sized array instead".into(),
span,
),
}
}
}
7 changes: 6 additions & 1 deletion compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,12 @@ impl<'a> Resolver<'a> {

/// Translates an UnresolvedType to a Type
pub fn resolve_type(&mut self, typ: UnresolvedType) -> Type {
self.resolve_type_inner(typ, &mut vec![])
let span = typ.span;
let resolved_type = self.resolve_type_inner(typ, &mut vec![]);
if resolved_type.is_nested_slice() {
self.errors.push(ResolverError::NestedSlices { span: span.unwrap() });
}
resolved_type
}

pub fn resolve_type_aliases(
Expand Down
27 changes: 27 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ pub(crate) fn resolve_structs(
crate_id: CrateId,
) -> Vec<(CompilationError, FileId)> {
let mut errors: Vec<(CompilationError, FileId)> = vec![];
// This is necessary to avoid cloning the entire struct map
// when adding checks after each struct field is resolved.
let struct_ids = structs.keys().copied().collect::<Vec<_>>();

// Resolve each field in each struct.
// Each struct should already be present in the NodeInterner after def collection.
for (type_id, typ) in structs {
Expand All @@ -35,6 +39,28 @@ pub(crate) fn resolve_structs(
struct_def.generics = generics;
});
}

// Check whether the struct fields have nested slices
// We need to check after all structs are resolved to
// make sure every struct's fields is accurately set.
for id in struct_ids {
let struct_type = context.def_interner.get_struct(id);
// Only handle structs without generics as any generics args will be checked
// after monomorphization when performing SSA codegen
if struct_type.borrow().generics.is_empty() {
let fields = struct_type.borrow().get_fields(&[]);
for field in fields.iter() {
if field.1.is_nested_slice() {
errors.push((
ResolverError::NestedSlices { span: struct_type.borrow().location.span }
.into(),
struct_type.borrow().location.file,
));
}
}
}
}

errors
}

Expand All @@ -49,5 +75,6 @@ fn resolve_struct_fields(
let (generics, fields, errors) =
Resolver::new(&mut context.def_interner, &path_resolver, &context.def_maps, file_id)
.resolve_struct_fields(unresolved.struct_def);

(generics, fields, errors)
}
37 changes: 37 additions & 0 deletions compiler/noirc_frontend/src/hir_def/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,43 @@ impl Type {
| Type::Error => unreachable!("This type cannot exist as a parameter to main"),
}
}

pub(crate) fn is_nested_slice(&self) -> bool {
match self {
Type::Array(size, elem) => {
if let Type::NotConstant = size.as_ref() {
elem.as_ref().contains_slice()
} else {
false
}
}
_ => false,
}
}

fn contains_slice(&self) -> bool {
match self {
Type::Array(size, _) => matches!(size.as_ref(), Type::NotConstant),
Type::Struct(struct_typ, generics) => {
let fields = struct_typ.borrow().get_fields(generics);
for field in fields.iter() {
if field.1.contains_slice() {
return true;
}
}
false
}
Type::Tuple(types) => {
for typ in types.iter() {
if typ.contains_slice() {
return true;
}
}
false
}
_ => false,
}
}
}

/// A list of TypeVariableIds to bind to a type. Storing the
Expand Down
4 changes: 4 additions & 0 deletions docs/docs/noir/concepts/data_types/arrays.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ You can define multidimensional arrays:
let array : [[Field; 2]; 2];
let element = array[0][0];
```
However, multidimensional slices are not supported. For example, the following code will error at compile time:
```rust
let slice : [[Field]] = [];
```

## Types

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "nested_slice_declared_type"
type = "bin"
authors = [""]
compiler_version = ">=0.22.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn main(x: Field, y: pub Field) {
assert(x != y);

let slice: [[Field]] = [];
assert(slice.len() != 10);
}
7 changes: 7 additions & 0 deletions test_programs/compile_failure/nested_slice_literal/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "nested_slice_literal"
type = "bin"
authors = [""]
compiler_version = ">=0.22.0"

[dependencies]
23 changes: 23 additions & 0 deletions test_programs/compile_failure/nested_slice_literal/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
struct FooParent<T> {
parent_arr: [Field; 3],
foos: [Foo<T>],
}

struct Bar {
inner: [Field; 3],
}

struct Foo<T> {
a: Field,
b: T,
bar: Bar,
}

fn main(x: Field, y: pub Field) {
assert(x != y);

let foo = Foo { a: 7, b: [8, 9, 22].as_slice(), bar: Bar { inner: [106, 107, 108] } };
let mut slice = [foo, foo];
slice = slice.push_back(foo);
assert(slice.len() == 3);
}
7 changes: 7 additions & 0 deletions test_programs/compile_failure/nested_slice_struct/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "nested_slice_struct"
type = "bin"
authors = [""]
compiler_version = ">=0.22.0"

[dependencies]
18 changes: 18 additions & 0 deletions test_programs/compile_failure/nested_slice_struct/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
struct FooParent {
parent_arr: [Field; 3],
foos: [Foo],
}

struct Bar {
inner: [Field; 3],
}

struct Foo {
a: Field,
b: [Field],
bar: Bar,
}

fn main(x: Field, y: pub Field) {
assert(x != y);
}

This file was deleted.

This file was deleted.

6 changes: 0 additions & 6 deletions test_programs/execution_success/slice_struct_field/Nargo.toml

This file was deleted.

This file was deleted.

0 comments on commit f8a1fb7

Please sign in to comment.