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

Compiler panic when you make a recursive call which does not terminate #4828

Closed
pventuzelo opened this issue Apr 17, 2024 · 4 comments · Fixed by #6291
Closed

Compiler panic when you make a recursive call which does not terminate #4828

pventuzelo opened this issue Apr 17, 2024 · 4 comments · Fixed by #6291
Assignees
Labels
bug Something isn't working

Comments

@pventuzelo
Copy link

Aim

We (https://github.com/FuzzingLabs) found that the compiler panics when you make a recursive call without condition, even if a return call makes you go out of the loop.a

Expected Behavior

No panic should occur.

Bug

When a function makes a recursive call after a return statement, the compiler considers the recursive call, which can lead to a panic crash as it treats it as an infinite loop. As you can see in the code example, the rec1 function has a return statement before the recursive call, preventing any potential looping. However, the compiler still perceives it as an infinite recursive function.

rec2 is another function that might exhibit the same behavior, but in this case, the compiler doesn’t panic. This seems to be because the compiler never enters the else block. The same phenomenon occurs with rec3.

The issue occurs with the latest compiler version on the master branch. The error message is as follows:

The application panicked (crashed).
Message:  Attempted to recur more than 1000 times during function inlining.
Location: compiler/noirc_evaluator/src/ssa/opt/inlining.rs:167

To Reproduce

/* Function with a recursive call which should end by returning 1, but the compiler treats it as an infinite loop. */
fn rec1() -> u8 {
    if true {
        1
    }
    rec1()
}

/* Function with a recursive call under a condition, so the compiler handles it correctly even though the result should be the same as rec1. */
fn rec2() -> u8 {
    if true {
        1
    } else {
        rec2()
    }
}

/* Another function with the same behavior that doesn't cause the compiler to panic. */
fn rec3() -> u8 {
    if false {
        rec3()
    }
    1
}

fn main() {
    rec1();
    rec2();
    rec3();
}

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@pventuzelo pventuzelo added the bug Something isn't working label Apr 17, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 17, 2024
@TomAFrench
Copy link
Member

TomAFrench commented Apr 17, 2024

/* Function with a recursive call which should end by returning 1, but the compiler treats it as an infinite loop. */
fn rec1() -> u8 {
    if true {
        1
    }
    rec1()
}

This function isn't doing what the comment is suggesting. The if statement doesn't cause the function rec1 to return so the compiler is correct to treat this as an infinite loop.

We should have a nicer error for this than a panic however.

@AFredefon
Copy link

/* Function with a recursive call which should end by returning 1, but the compiler treats it as an infinite loop. */
fn rec1() -> u8 {
    if true {
        1
    }
    rec1()
}

This function isn't doing what the comment is suggesting. The if statement doesn't cause the function rec1 to return so the compiler is correct to treat this as an infinite loop.

We should have a nicer error for this than a panic however.

I had omitted the fact that functions only return the last instruction. I assumed it worked like in Rust, where an instruction not followed by a semicolon is treated as a return. Sorry for the misunderstanding.

@jfecher
Copy link
Contributor

jfecher commented Apr 17, 2024

I had omitted the fact that functions only return the last instruction. I assumed it worked like in Rust, where an instruction not followed by a semicolon is treated as a return. Sorry for the misunderstanding.

It does work the same as in Rust. In Rust and Noir an instruction not followed by a semicolon is not a return from the function, it just yields the value of that expression. The same code in Rust does however issue an error that the result of an if with no else is expected to be a unit value though and suggests maybe a return should be added. We have a similar warning (without the helpful extra bit about adding return) in some cases, but not in an if with no else. Adding rust's error/warning would be helpful here.

@jfecher jfecher changed the title Compiler panic when you make a recursive call after a return call Compiler panic when you make a recursive call which does not terminate Apr 19, 2024
@jfecher
Copy link
Contributor

jfecher commented Apr 19, 2024

Since this error applies to any recursive function and doesn't require a return call, I'm going to treat this issue as the general issue for improving the "Attempted to recur more than 1000 times during function inlining" error and am closing #4843.

Minimal repro for this issue is

fn main() {
    main();
}

github-merge-queue bot pushed a commit that referenced this issue Oct 21, 2024
…#6291)

# Description

## Problem\*

Resolves #4828 

## Summary\*

Given a program like this:
```
fn main() {
  main();
}
```

Changed this panic message:

```
Attempted to recur more than 1000 times during function inlining.
```

Into this:
```
Attempted to recur more than 1000 times during inlining function 'main': acir(inline) fn main f0 {
  b0():
    call f0()
    return 
}
```

I included the ACIR to help figure out which function is the culprit, in
case the name is not enough.

## Additional Context

I added a test to show that there is no compilation error or warning for
the example program. It would be nice to issue a warning about
unconditional recursion like Rust does. I'll try to do that in a
followup PR.


## 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.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Oct 21, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 24, 2024
# 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants