Skip to content

Commit

Permalink
feat: Reject programs with unconditional recursion (#6292)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Related to #4828 
Followup to #6291

## Summary\*

Added a new `ResolutionError::UnconditionalRecursion` which should
result in a warning or compilation error where the function body
recurses into the defining function with no alternative way to return.

## Additional Context


## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] 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
aakoshh authored Oct 24, 2024
1 parent c891ffd commit 00c5c51
Show file tree
Hide file tree
Showing 5 changed files with 238 additions and 5 deletions.
82 changes: 79 additions & 3 deletions compiler/noirc_frontend/src/elaborator/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ use crate::{
type_check::TypeCheckError,
},
hir_def::{
expr::{HirExpression, HirIdent, HirLiteral},
expr::{HirBlockExpression, HirExpression, HirIdent, HirLiteral},
function::FuncMeta,
stmt::HirStatement,
},
node_interner::{
DefinitionId, DefinitionKind, ExprId, FuncId, FunctionModifiers, NodeInterner,
},
node_interner::NodeInterner,
node_interner::{DefinitionKind, ExprId, FuncId, FunctionModifiers},
Type,
};

Expand Down Expand Up @@ -264,3 +266,77 @@ pub(crate) fn overflowing_int(
fn func_meta_name_ident(func: &FuncMeta, modifiers: &FunctionModifiers) -> Ident {
Ident(Spanned::from(func.name.location.span, modifiers.name.clone()))
}

/// Check that a recursive function *can* return without endlessly calling itself.
pub(crate) fn unbounded_recursion<'a>(
interner: &'a NodeInterner,
func_id: FuncId,
func_name: impl FnOnce() -> &'a str,
func_span: Span,
body_id: ExprId,
) -> Option<ResolverError> {
if !can_return_without_recursing(interner, func_id, body_id) {
Some(ResolverError::UnconditionalRecursion {
name: func_name().to_string(),
span: func_span,
})
} else {
None
}
}

/// Check if an expression will end up calling a specific function.
fn can_return_without_recursing(interner: &NodeInterner, func_id: FuncId, expr_id: ExprId) -> bool {
let check = |e| can_return_without_recursing(interner, func_id, e);

let check_block = |block: HirBlockExpression| {
block.statements.iter().all(|stmt_id| match interner.statement(stmt_id) {
HirStatement::Let(s) => check(s.expression),
HirStatement::Assign(s) => check(s.expression),
HirStatement::Expression(e) => check(e),
HirStatement::Semi(e) => check(e),
// Rust doesn't seem to check the for loop body (it's bounds might mean it's never called).
HirStatement::For(e) => check(e.start_range) && check(e.end_range),
HirStatement::Constrain(_)
| HirStatement::Comptime(_)
| HirStatement::Break
| HirStatement::Continue
| HirStatement::Error => true,
})
};

match interner.expression(&expr_id) {
HirExpression::Ident(ident, _) => {
if ident.id == DefinitionId::dummy_id() {
return true;
}
let definition = interner.definition(ident.id);
if let DefinitionKind::Function(id) = definition.kind {
func_id != id
} else {
true
}
}
HirExpression::Block(b) => check_block(b),
HirExpression::Prefix(e) => check(e.rhs),
HirExpression::Infix(e) => check(e.lhs) && check(e.rhs),
HirExpression::Index(e) => check(e.collection) && check(e.index),
HirExpression::MemberAccess(e) => check(e.lhs),
HirExpression::Call(e) => check(e.func) && e.arguments.iter().cloned().all(check),
HirExpression::MethodCall(e) => check(e.object) && e.arguments.iter().cloned().all(check),
HirExpression::Cast(e) => check(e.lhs),
HirExpression::If(e) => {
check(e.condition) && (check(e.consequence) || e.alternative.map(check).unwrap_or(true))
}
HirExpression::Tuple(e) => e.iter().cloned().all(check),
HirExpression::Unsafe(b) => check_block(b),
// Rust doesn't check the lambda body (it might not be called).
HirExpression::Lambda(_)
| HirExpression::Literal(_)
| HirExpression::Constructor(_)
| HirExpression::Quote(_)
| HirExpression::Unquote(_)
| HirExpression::Comptime(_)
| HirExpression::Error => true,
}
}
14 changes: 14 additions & 0 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,20 @@ impl<'context> Elaborator<'context> {
self.check_for_unused_variables_in_scope_tree(func_scope_tree);
}

// Check that the body can return without calling the function.
if let FunctionKind::Normal | FunctionKind::Recursive = kind {
self.run_lint(|elaborator| {
lints::unbounded_recursion(
elaborator.interner,
id,
|| elaborator.interner.definition_name(func_meta.name.id),
func_meta.name.location.span,
hir_func.as_expr(),
)
.map(Into::into)
});
}

let meta = self
.interner
.func_meta
Expand Down
11 changes: 11 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ pub enum ResolverError {
UnusedVariable { ident: Ident },
#[error("Unused {}", item.item_type())]
UnusedItem { ident: Ident, item: UnusedItem },
#[error("Unconditional recursion")]
UnconditionalRecursion { name: String, span: Span },
#[error("Could not find variable in this scope")]
VariableNotDeclared { name: String, span: Span },
#[error("path is not an identifier")]
Expand Down Expand Up @@ -213,6 +215,15 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
diagnostic.unnecessary = true;
diagnostic
}
ResolverError::UnconditionalRecursion { name, span} => {
let mut diagnostic = Diagnostic::simple_warning(
format!("function `{name}` cannot return without recursing"),
"function cannot return without recursing".to_string(),
*span,
);
diagnostic.unnecessary = true;
diagnostic
}
ResolverError::VariableNotDeclared { name, span } => Diagnostic::simple_error(
format!("cannot find `{name}` in this scope "),
"not found in this scope".to_string(),
Expand Down
132 changes: 131 additions & 1 deletion compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ pub(crate) fn get_program_errors(src: &str) -> Vec<(CompilationError, FileId)> {
fn assert_no_errors(src: &str) {
let errors = get_program_errors(src);
if !errors.is_empty() {
panic!("Expected no errors, got: {:?}", errors);
panic!("Expected no errors, got: {:?}; src = {src}", errors);
}
}

Expand Down Expand Up @@ -3390,6 +3390,136 @@ fn arithmetic_generics_rounding_fail() {
assert_eq!(errors.len(), 1);
}

#[test]
fn unconditional_recursion_fail() {
let srcs = vec![
r#"
fn main() {
main()
}
"#,
r#"
fn main() -> pub bool {
if main() { true } else { false }
}
"#,
r#"
fn main() -> pub bool {
if true { main() } else { main() }
}
"#,
r#"
fn main() -> pub u64 {
main() + main()
}
"#,
r#"
fn main() -> pub u64 {
1 + main()
}
"#,
r#"
fn main() -> pub bool {
let _ = main();
true
}
"#,
r#"
fn main(a: u64, b: u64) -> pub u64 {
main(a + b, main(a, b))
}
"#,
r#"
fn main() -> pub u64 {
foo(1, main())
}
fn foo(a: u64, b: u64) -> u64 {
a + b
}
"#,
r#"
fn main() -> pub u64 {
let (a, b) = (main(), main());
a + b
}
"#,
r#"
fn main() -> pub u64 {
let mut sum = 0;
for i in 0 .. main() {
sum += i;
}
sum
}
"#,
];

for src in srcs {
let errors = get_program_errors(src);
assert!(
!errors.is_empty(),
"expected 'unconditional recursion' error, got nothing; src = {src}"
);

for (error, _) in errors {
let CompilationError::ResolverError(ResolverError::UnconditionalRecursion { .. }) =
error
else {
panic!("Expected an 'unconditional recursion' error, got {:?}; src = {src}", error);
};
}
}
}

#[test]
fn unconditional_recursion_pass() {
let srcs = vec![
r#"
fn main() {
if false { main(); }
}
"#,
r#"
fn main(i: u64) -> pub u64 {
if i == 0 { 0 } else { i + main(i-1) }
}
"#,
// Only immediate self-recursion is detected.
r#"
fn main() {
foo();
}
fn foo() {
bar();
}
fn bar() {
foo();
}
"#,
// For loop bodies are not checked.
r#"
fn main() -> pub u64 {
let mut sum = 0;
for _ in 0 .. 10 {
sum += main();
}
sum
}
"#,
// Lambda bodies are not checked.
r#"
fn main() {
let foo = || main();
foo();
}
"#,
];

for src in srcs {
assert_no_errors(src);
}
}

#[test]
fn uses_self_in_import() {
let src = r#"
Expand Down
4 changes: 3 additions & 1 deletion noir_stdlib/src/meta/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,8 @@ mod tests {
let _: MyOtherStruct = MyOtherStruct { my_other_field: 2 };
let _ = derive_do_nothing(crate::panic::panic(f""));
let _ = derive_do_nothing_alt(crate::panic::panic(f""));
remove_unused_warnings();
if false {
remove_unused_warnings();
}
}
}

0 comments on commit 00c5c51

Please sign in to comment.