rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.42k stars 12.59k forks source link

Making a mistake in a match can lead to infinite recursion without any hint to what is going on #130376

Open JomerDev opened 3 weeks ago

JomerDev commented 3 weeks ago

Code

pub struct SubMenu;

impl SubMenu {
    pub fn test(&self) {
    }
}

pub enum Item {
    Back,
    SubMenu(SubMenu)
}

impl Item {
    pub fn test(&self) {
        match self {
            Item::Back => (),
            // This recurses because I messed up the match
            // I really want Item::SubMenu(x) but there is 
            // no warning at all that this can cause an issue
            sub => sub.test()
        }
    }
}

fn main() {
    let itm = Item::SubMenu(SubMenu{});
    itm.test();
}

Current output

(Running in rust playground)
Compiling playground v0.0.1 (/playground)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.46s

(Clippy output in rust playground)
Checking playground v0.0.1 (/playground)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.88s

Desired output

Anything that tells the user that there is an infinite loop waiting to happen

Something like:
sub => sub.test()
       ^^^^^^^^^^ this will recurse infinitely

Rationale and extra context

I made a mistake while writing the match seen above. I really wanted Item::SubMenu(sub) => sub.test() but mistakenly thought sub => sub.test() will be enough. While the error was of course made by me, after I found it I was surprised that neither rustc, clippy nor rust-analyzer will warn me that this will cause infinite recursion. I ran the code on an embedded target which made the error search very hard, since any debug connection died when the stack overflowed without any error message, leading to a two day search for the cause

Other cases

No response

Rust Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-pc-windows-msvc
release: 1.81.0
LLVM version: 18.1.7

(The exact same issue/missing diagnostic is also true on the beta and nightly channels)

Anything else?

No response

gurry commented 3 weeks ago

Reduced:

fn test(x: bool) {
    if x {
        test(true);
    }
}

fn main() {
    test(true);
}

@rustbot claim

gurry commented 2 weeks ago

The current recursion analysis is fairly basic and is simply is not capable of catching the problem reported in this issue.

It does not take into account the arguments passed by the caller and emits warnings based on just one thing: whether all branches in the function recurse or not. If they do a warning is emitted but even if one of them does not recurse, no warning is emitted. That is pretty much it.

To illustrate, the following function will produce a warning under the current analysis because all branches in it recurse:

fn test(x: bool) {
    if x {
        test(true);
    } else {
        test(false);
   }
}

but the following will not because the else branch does not recurse:

fn test(x: bool) {
    if x {
        test(true);
    } else {
        return;
   }
}

Note that the analysis completely ignores the fact that the else branch will never be taken if the caller passes true for x, which shows that it does take into account the values of the arguments and how they are used in the function.

A more capable analysis that can catch the reported issue will have to be implemented based on some kind of data flow I'm guessing. However, one reason it may not yet have been implemented is that it may be slower.