Skip to content

Commit

Permalink
fix: Type check ACIR mutable reference passed to brillig (#4281)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #4245 

## Summary\*

This adds a type check for call expressions to check the arguments of a
call to an unconstrained function from a constrained function. As we
have two different runtime environments this makes type checking a bit
more complicated. As a followup to this PR we could try and catch the
error added here (#4280) sooner.
However, if the unconstrained function uses generics we must still fall
back to the "type check" in SSA/ACIR gen from PR #4280.

This PR now errors for the snippet in issue with the below using `nargo
check`:

<img width="900" alt="Screenshot 2024-02-06 at 3 36 27 PM"
src="https://github.com/noir-lang/noir/assets/43554004/6c037458-3182-4ea4-8470-050a1734b875">


## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
vezenovm authored Feb 7, 2024
1 parent 2e32845 commit 7e139de
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 1 deletion.
7 changes: 6 additions & 1 deletion compiler/noirc_frontend/src/hir/type_check/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ pub enum TypeCheckError {
NoMatchingImplFound { constraints: Vec<(Type, String)>, span: Span },
#[error("Constraint for `{typ}: {trait_name}` is not needed, another matching impl is already in scope")]
UnneededTraitConstraint { trait_name: String, typ: Type, span: Span },
#[error(
"Cannot pass a mutable reference from a constrained runtime to an unconstrained runtime"
)]
ConstrainedReferenceToUnconstrained { span: Span },
}

impl TypeCheckError {
Expand Down Expand Up @@ -202,7 +206,8 @@ impl From<TypeCheckError> for Diagnostic {
| TypeCheckError::AmbiguousBitWidth { span, .. }
| TypeCheckError::IntegerAndFieldBinaryOperation { span }
| TypeCheckError::OverflowingAssignment { span, .. }
| TypeCheckError::FieldModulo { span } => {
| TypeCheckError::FieldModulo { span }
| TypeCheckError::ConstrainedReferenceToUnconstrained { span } => {
Diagnostic::simple_error(error.to_string(), String::new(), span)
}
TypeCheckError::PublicReturnType { typ, span } => Diagnostic::simple_error(
Expand Down
34 changes: 34 additions & 0 deletions compiler/noirc_frontend/src/hir/type_check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ impl<'interner> TypeChecker<'interner> {
}
}

fn is_unconstrained_call(&self, expr: &ExprId) -> bool {
if let HirExpression::Ident(expr::HirIdent { id, .. }) = self.interner.expression(expr) {
if let Some(DefinitionKind::Function(func_id)) =
self.interner.try_definition(id).map(|def| &def.kind)
{
let modifiers = self.interner.function_modifiers(func_id);
return modifiers.is_unconstrained;
}
}
false
}

/// Infers a type for a given expression, and return this type.
/// As a side-effect, this function will also remember this type in the NodeInterner
/// for the given expr_id key.
Expand Down Expand Up @@ -139,6 +151,15 @@ impl<'interner> TypeChecker<'interner> {
}
HirExpression::Index(index_expr) => self.check_index_expression(expr_id, index_expr),
HirExpression::Call(call_expr) => {
// Need to setup these flags here as `self` is borrowed mutably to type check the rest of the call expression
// These flags are later used to type check calls to unconstrained functions from constrained functions
let current_func = self
.current_function
.expect("Can only have call expression inside of a function body");
let func_mod = self.interner.function_modifiers(&current_func);
let is_current_func_constrained = !func_mod.is_unconstrained;
let is_unconstrained_call = self.is_unconstrained_call(&call_expr.func);

self.check_if_deprecated(&call_expr.func);

let function = self.check_expression(&call_expr.func);
Expand All @@ -147,6 +168,19 @@ impl<'interner> TypeChecker<'interner> {
let typ = self.check_expression(arg);
(typ, *arg, self.interner.expr_span(arg))
});

for (typ, _, _) in args.iter() {
if is_current_func_constrained
&& is_unconstrained_call
&& matches!(&typ, Type::MutableReference(_))
{
self.errors.push(TypeCheckError::ConstrainedReferenceToUnconstrained {
span: self.interner.expr_span(expr_id),
});
return Type::Error;
}
}

let span = self.interner.expr_span(expr_id);
self.bind_function_type(function, args, span)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "brillig_mut_ref_from_acir"
type = "bin"
authors = [""]
compiler_version = ">=0.23.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
unconstrained fn mut_ref_identity(value: &mut Field) -> Field {
*value
}

fn main(mut x: Field, y: pub Field) {
let returned_x = mut_ref_identity(&mut x);
assert(returned_x == x);
}

0 comments on commit 7e139de

Please sign in to comment.