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.

Sign up for GitHub

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

fix: Prevent mutating immutable bindings to mutable reference types #3008

Closed
wants to merge 14 commits into from
Closed

fix: Prevent mutating immutable bindings to mutable reference types #3008

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
066d8d9
Fix mut type check
jfecher Oct 5, 2023
73c401c
Merge branch 'master' into jf/fix-mut-check
kevaundray Oct 9, 2023
cac887b
add regression test
kevaundray Oct 9, 2023
a4c6d70
Merge branch 'master' into jf/fix-mut-check
kevaundray Oct 9, 2023
9fa02c9
Add regression test
jfecher Oct 9, 2023
92b773c
Update tooling/nargo_cli/tests/execution_success/slices_mutable_retur…
kevaundray Oct 9, 2023
5914fd7
remove kev's regression test
kevaundray Oct 9, 2023
f79e971
Merge branch 'master' into jf/fix-mut-check
kevaundray Oct 9, 2023
1f86b52
Merge branch 'master' into jf/fix-mut-check
kevaundray Oct 10, 2023
73590e9
Revert "Merge branch 'master' into jf/fix-mut-check"
kevaundray Oct 10, 2023
d36cdec
Revert "Merge branch 'master' into jf/fix-mut-check"
kevaundray Oct 10, 2023
9b29103
Revert "remove kev's regression test"
kevaundray Oct 10, 2023
8c1fe35
Merge branch 'master' into jf/fix-mut-check
jfecher Oct 10, 2023
584f866
Merge branch 'jf/fix-mut-check' of https://github.com/noir-lang/noir …
jfecher Oct 10, 2023
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
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/type_check/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ impl<'interner> TypeChecker<'interner> {
});

let lhs_type = self.check_expression(&index_expr.collection);
match lhs_type {
match lhs_type.follow_bindings() {
// XXX: We can check the array bounds here also, but it may be better to constant fold first
// and have ConstId instead of ExprId for constants
Type::Array(_, base_type) => *base_type,
Expand Down
68 changes: 45 additions & 23 deletions compiler/noirc_frontend/src/hir/type_check/stmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,12 @@ impl<'interner> TypeChecker<'interner> {
fn check_assign_stmt(&mut self, assign_stmt: HirAssignStatement, stmt_id: &StmtId) {
let expr_type = self.check_expression(&assign_stmt.expression);
let span = self.interner.expr_span(&assign_stmt.expression);
let (lvalue_type, new_lvalue) = self.check_lvalue(assign_stmt.lvalue, span);
let (lvalue_type, new_lvalue, mutable) = self.check_lvalue(&assign_stmt.lvalue, span);

if !mutable {
let (name, span) = self.get_lvalue_name_and_span(&assign_stmt.lvalue);
self.errors.push(TypeCheckError::VariableMustBeMutable { name, span });
}

// Must push new lvalue to the interner, we've resolved any field indices
self.interner.update_statement(stmt_id, |stmt| match stmt {
Expand All @@ -159,39 +164,52 @@ impl<'interner> TypeChecker<'interner> {
});
}

fn get_lvalue_name_and_span(&self, lvalue: &HirLValue) -> (String, Span) {
match lvalue {
HirLValue::Ident(name, _) => {
let span = name.location.span;

if let Some(definition) = self.interner.try_definition(name.id) {
(definition.name.clone(), span)
} else {
("(undeclared variable)".into(), span)
}
}
HirLValue::MemberAccess { object, .. } => self.get_lvalue_name_and_span(object),
HirLValue::Index { array, .. } => self.get_lvalue_name_and_span(array),
HirLValue::Dereference { lvalue, .. } => self.get_lvalue_name_and_span(lvalue),
}
}

/// Type check an lvalue - the left hand side of an assignment statement.
fn check_lvalue(&mut self, lvalue: HirLValue, assign_span: Span) -> (Type, HirLValue) {
fn check_lvalue(&mut self, lvalue: &HirLValue, assign_span: Span) -> (Type, HirLValue, bool) {
match lvalue {
HirLValue::Ident(ident, _) => {
let mut mutable = true;

let typ = if ident.id == DefinitionId::dummy_id() {
Type::Error
} else {
// Do we need to store TypeBindings here?
let typ = self.interner.id_type(ident.id).instantiate(self.interner).0;
let typ = typ.follow_bindings();

if let Some(definition) = self.interner.try_definition(ident.id) {
if !definition.mutable && !matches!(typ, Type::MutableReference(_)) {
self.errors.push(TypeCheckError::VariableMustBeMutable {
name: definition.name.clone(),
span: ident.location.span,
});
}
mutable = definition.mutable;
}

typ
let typ = self.interner.id_type(ident.id).instantiate(self.interner).0;
typ.follow_bindings()
};

(typ.clone(), HirLValue::Ident(ident, typ))
(typ.clone(), HirLValue::Ident(*ident, typ), mutable)
}
HirLValue::MemberAccess { object, field_name, .. } => {
let (lhs_type, object) = self.check_lvalue(*object, assign_span);
let (lhs_type, object, mut mutable) = self.check_lvalue(object, assign_span);
let mut object = Box::new(object);
let span = field_name.span();
let field_name = field_name.clone();

let object_ref = &mut object;
let mutable_ref = &mut mutable;

let (typ, field_index) = self
let (object_type, field_index) = self
.check_field_access(
&lhs_type,
&field_name.0.contents,
Expand All @@ -206,16 +224,19 @@ impl<'interner> TypeChecker<'interner> {

let lvalue = std::mem::replace(object_ref, Box::new(tmp_value));
*object_ref = Box::new(HirLValue::Dereference { lvalue, element_type });
*mutable_ref = true;
},
)
.unwrap_or((Type::Error, 0));

let field_index = Some(field_index);
(typ.clone(), HirLValue::MemberAccess { object, field_name, field_index, typ })
let typ = object_type.clone();
let lvalue = HirLValue::MemberAccess { object, field_name, field_index, typ };
(object_type, lvalue, mutable)
}
HirLValue::Index { array, index, .. } => {
let index_type = self.check_expression(&index);
let expr_span = self.interner.expr_span(&index);
let index_type = self.check_expression(index);
let expr_span = self.interner.expr_span(index);

index_type.unify(
&Type::polymorphic_integer(self.interner),
Expand All @@ -227,7 +248,7 @@ impl<'interner> TypeChecker<'interner> {
},
);

let (result, array) = self.check_lvalue(*array, assign_span);
let (result, array, mutable) = self.check_lvalue(array, assign_span);
let array = Box::new(array);

let typ = match result {
Expand All @@ -244,10 +265,10 @@ impl<'interner> TypeChecker<'interner> {
}
};

(typ.clone(), HirLValue::Index { array, index, typ })
(typ.clone(), HirLValue::Index { array, index: *index, typ }, mutable)
}
HirLValue::Dereference { lvalue, element_type: _ } => {
let (reference_type, lvalue) = self.check_lvalue(*lvalue, assign_span);
let (reference_type, lvalue, _) = self.check_lvalue(lvalue, assign_span);
let lvalue = Box::new(lvalue);

let element_type = Type::type_variable(self.interner.next_type_variable_id());
Expand All @@ -258,7 +279,8 @@ impl<'interner> TypeChecker<'interner> {
expr_span: assign_span,
});

(element_type.clone(), HirLValue::Dereference { lvalue, element_type })
// Dereferences are always mutable since we already type checked against a &mut T
(element_type.clone(), HirLValue::Dereference { lvalue, element_type }, true)
}
}
}
Expand Down