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

fix: Error on empty function bodies #5519

Merged
merged 5 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
6 changes: 5 additions & 1 deletion compiler/noirc_frontend/src/hir/comptime/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,11 @@
self.define_pattern(parameter, typ, argument, arg_location)?;
}

let function_body = self.interner.function(&function).as_expr();
let function_body = self.interner.function(&function).try_as_expr().ok_or_else(|| {
let function = self.interner.function_name(&function).to_owned();
InterpreterError::NonComptimeFnCallInSameCrate { function, location }
})?;

let result = self.evaluate(function_body)?;

self.exit_function(previous_state);
Expand Down Expand Up @@ -161,7 +165,7 @@
}
} else {
let name = self.interner.function_name(&function);
unreachable!("Non-builtin, lowlevel or oracle builtin fn '{name}'")

Check warning on line 168 in compiler/noirc_frontend/src/hir/comptime/interpreter.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (lowlevel)
}
}

Expand Down
9 changes: 5 additions & 4 deletions compiler/noirc_frontend/src/hir/comptime/scan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ impl<'interner> Interpreter<'interner> {
return Ok(());
}

let function = self.interner.function(&function);
if let Some(function) = self.interner.function(&function).try_as_expr() {
let state = self.enter_function();
self.scan_expression(function)?;
self.exit_function(state);
}

let state = self.enter_function();
self.scan_expression(function.as_expr())?;
self.exit_function(state);
Ok(())
}

Expand Down
76 changes: 40 additions & 36 deletions compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
let declared_return_type = meta.return_type().clone();
let can_ignore_ret = meta.is_stub();

let function_body_id = &interner.function(&func_id).as_expr();
let function_body_id = interner.function(&func_id).try_as_expr();

let mut type_checker = TypeChecker::new(interner);
type_checker.current_function = Some(func_id);
Expand Down Expand Up @@ -88,45 +88,49 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
type_checker.bind_pattern(&param.0, param.1);
}

let function_last_type = type_checker.check_function_body(function_body_id);
// Check declared return type and actual return type
if !can_ignore_ret {
let (expr_span, empty_function) = function_info(type_checker.interner, function_body_id);
let func_span = type_checker.interner.expr_span(function_body_id); // XXX: We could be more specific and return the span of the last stmt, however stmts do not have spans yet
if let Type::TraitAsType(trait_id, _, generics) = &declared_return_type {
if type_checker
.interner
.lookup_trait_implementation(&function_last_type, *trait_id, generics)
.is_err()
{
let error = TypeCheckError::TypeMismatchWithSource {
expected: declared_return_type.clone(),
actual: function_last_type,
span: func_span,
source: Source::Return(expected_return_type, expr_span),
};
errors.push(error);
}
} else {
function_last_type.unify_with_coercions(
&declared_return_type,
*function_body_id,
type_checker.interner,
&mut errors,
|| {
let mut error = TypeCheckError::TypeMismatchWithSource {
if let Some(function_body_id) = function_body_id {
let function_last_type = type_checker.check_function_body(&function_body_id);

// Check declared return type and actual return type
if !can_ignore_ret {
let (expr_span, empty_function) =
function_info(type_checker.interner, &function_body_id);
let func_span = type_checker.interner.expr_span(&function_body_id); // XXX: We could be more specific and return the span of the last stmt, however stmts do not have spans yet
if let Type::TraitAsType(trait_id, _, generics) = &declared_return_type {
if type_checker
.interner
.lookup_trait_implementation(&function_last_type, *trait_id, generics)
.is_err()
{
let error = TypeCheckError::TypeMismatchWithSource {
expected: declared_return_type.clone(),
actual: function_last_type.clone(),
actual: function_last_type,
span: func_span,
source: Source::Return(expected_return_type, expr_span),
};

if empty_function {
error = error.add_context("implicitly returns `()` as its body has no tail or `return` expression");
}
error
},
);
errors.push(error);
}
} else {
function_last_type.unify_with_coercions(
&declared_return_type,
function_body_id,
type_checker.interner,
&mut errors,
|| {
let mut error = TypeCheckError::TypeMismatchWithSource {
expected: declared_return_type.clone(),
actual: function_last_type.clone(),
span: func_span,
source: Source::Return(expected_return_type, expr_span),
};

if empty_function {
error = error.add_context("implicitly returns `()` as its body has no tail or `return` expression");
}
error
},
);
}
}
}

Expand Down
7 changes: 0 additions & 7 deletions compiler/noirc_frontend/src/hir_def/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,6 @@ pub enum HirExpression {
Error,
}

impl HirExpression {
/// Returns an empty block expression
pub const fn empty_block() -> HirExpression {
HirExpression::Block(HirBlockExpression { statements: vec![] })
}
}

/// Corresponds to a variable in the source code
#[derive(Debug, Clone)]
pub struct HirIdent {
Expand Down
18 changes: 11 additions & 7 deletions compiler/noirc_frontend/src/hir_def/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,30 @@ use crate::macros_api::{BlockExpression, StructId};
use crate::node_interner::{ExprId, NodeInterner, TraitImplId};
use crate::{ResolvedGeneric, Type};

/// A Hir function is a block expression
/// with a list of statements
/// A Hir function is a block expression with a list of statements.
/// If the function has yet to be resolved, the body starts off empty (None).
#[derive(Debug, Clone)]
pub struct HirFunction(ExprId);
pub struct HirFunction(Option<ExprId>);

impl HirFunction {
pub fn empty() -> HirFunction {
HirFunction(ExprId::empty_block_id())
HirFunction(None)
}

pub const fn unchecked_from_expr(expr_id: ExprId) -> HirFunction {
HirFunction(expr_id)
HirFunction(Some(expr_id))
}

pub const fn as_expr(&self) -> ExprId {
pub fn as_expr(&self) -> ExprId {
self.0.expect("Function has yet to be elaborated, cannot get an ExprId of its body!")
}

pub fn try_as_expr(&self) -> Option<ExprId> {
self.0
}

pub fn block(&self, interner: &NodeInterner) -> HirBlockExpression {
match interner.expression(&self.0) {
match interner.expression(&self.as_expr()) {
HirExpression::Block(block_expr) => block_expr,
_ => unreachable!("ice: functions can only be block expressions"),
}
Expand Down
14 changes: 2 additions & 12 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@
/// Stores the [Location] of a [Type] reference
pub(crate) type_ref_locations: Vec<(Type, Location)>,

/// In Noir's metaprogramming, a noir type has the type `Type`. When these are spliced

Check warning on line 196 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (metaprogramming)
/// into `quoted` expressions, we preserve the original type by assigning it a unique id
/// and creating a `Token::QuotedType(id)` from this id. We cannot create a token holding
/// the actual type since types do not implement Send or Sync.
Expand Down Expand Up @@ -385,11 +385,6 @@
#[derive(Debug, Eq, PartialEq, Hash, Copy, Clone, PartialOrd, Ord)]
pub struct ExprId(Index);

impl ExprId {
pub fn empty_block_id() -> ExprId {
ExprId(Index::unsafe_zeroed())
}
}
#[derive(Debug, Eq, PartialEq, Hash, Copy, Clone)]
pub struct FuncId(Index);

Expand Down Expand Up @@ -557,7 +552,7 @@

impl Default for NodeInterner {
fn default() -> Self {
let mut interner = NodeInterner {
NodeInterner {
nodes: Arena::default(),
func_meta: HashMap::new(),
function_definition_ids: HashMap::new(),
Expand Down Expand Up @@ -597,12 +592,7 @@
reference_graph: petgraph::graph::DiGraph::new(),
reference_graph_indices: HashMap::new(),
reference_modules: HashMap::new(),
};

// An empty block expression is used often, we add this into the `node` on startup
let expr_id = interner.push_expr(HirExpression::empty_block());
assert_eq!(expr_id, ExprId::empty_block_id());
interner
}
}
}

Expand Down
Loading