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

Add lint for direct recursive self-call of function #17899

Closed
huonw opened this issue Oct 9, 2014 · 6 comments · Fixed by #20373
Closed

Add lint for direct recursive self-call of function #17899

huonw opened this issue Oct 9, 2014 · 6 comments · Fixed by #20373
Assignees
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut.

Comments

@huonw
Copy link
Member

huonw commented Oct 9, 2014

With trait objects it is not so rare to have code like

impl Foo for Box<Foo> {
    fn bar(&self, x: uint) {
        // call the `bar` method in the vtable, could try:
        // self.bar(x);
        // (*self).bar(x);
        // (**self).bar(x);
    }
}

One might first write self.bar(x), but this will be calling that same bar, via since Box<Foo> has a "real" bar method itself. The correct procedure is (**self).bar(x). This would helped by detecting the most obvious cases of a function calling itself in a nonterminating way. The most obvious way that is likely to have low risk of false positives is detecting if the CFG of a function reaches a self-call unconditionally. e.g.

// these receive a warning:
fn foo() { foo() }

fn bar(x: uint) {
    bar(x - 1);
}

fn baz(x: uint) -> uint {
    if x == 0 {
        println!("hi")
    }
    2 * baz(x + 1)
}

// this is fine:
fn qux(x: uint) {
    if x == 0 { loop {} } // (could be `return 1`, etc. etc.)

    qux(x - 1) + 1
}

Notably, it does not matter if the arguments to the self call are different, since the call will always occur, and it also does not matter if the self-call is in tail position.

There is the possibility that someone is using infinite recursion to achieve an infinite loop, it doesn't seem crazy to recommend that they use loop instead.

A more conservative alternative would be just detecting the method call in the first example, that is, a function that consists of a single expression that is a self-call.

@huonw huonw added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Oct 9, 2014
@huonw
Copy link
Member Author

huonw commented Oct 10, 2014

I think an infinite loop written with tail recursion will be rare enough that that is not a problem.

@reem
Copy link
Contributor

reem commented Oct 10, 2014

Personally I've hit a lot of infinite recursions caused by this, a lint would help a lot, especially since it's not really obvious how to fix the problem.

@emberian
Copy link
Member

Me too!

@huonw
Copy link
Member Author

huonw commented Dec 29, 2014

I have this mostly implemented, except for some edge cases with loop {}, but the compiler doesn't seem to want to let me work out the precise method definition being called for a foo.bar() expression. (Meaning everything except the actual motivating example works.) I'm pestering anyone and everyone on IRC so we'll eventually find a solution...

@huonw
Copy link
Member Author

huonw commented Dec 30, 2014

Specifically, given

trait Foo {
    fn bar(&self); // A
}

impl Foo for uint {
    fn bar(&self) { // B
        self.bar(); // C
    }
}

I want to check if the method call C is calling method B, that is, obtain the DefId of the method called in the ExprMethodCall (if it exists, e.g. a method call on a trait object just optimisically assumes there isn't infinite recursion) and compare against the NodeId of B.

However, I can only work out how to obtain the DefId of A (via tcx.method_map and then ty::trait_item after pattern matching into the MethodTypeParam variant), and it's highly non-obvious how to correctly obtain the DefId of B. trans does obtain this info somehow, but naively copying its behaviour causes panics.

Advice, anyone?

@huonw
Copy link
Member Author

huonw commented Dec 30, 2014

Niko gave me a lot of advice on IRC.

huonw added a commit to huonw/rust that referenced this issue Jan 24, 2015
E.g. `fn foo() { foo() }`, or, more subtlely

    impl Foo for Box<Foo+'static> {
        fn bar(&self) {
            self.bar();
        }
    }

The compiler will warn and point out the points where recursion occurs,
if it determines that the function cannot return without calling itself.

Closes rust-lang#17899.
bors added a commit that referenced this issue Jan 25, 2015
E.g. `fn foo() { foo() }`, or, more subtlely

    impl Foo for Box<Foo+'static> {
        fn bar(&self) {
            self.bar();
        }
    }

The compiler will warn and point out the points where recursion occurs,
if it determines that the function cannot return without calling itself.

Closes #17899.

---

This is highly non-perfect, in particular, my wording above is quite precise, and I have some unresolved questions: This currently will warn about

```rust
fn foo() {
    if bar { loop {} }

    foo()
}
```

even though `foo` may never be called (i.e. our apparent "unconditional" recursion is actually conditional). I don't know if we should handle this case, and ones like it with `panic!()` instead of `loop` (or anything else that "returns" `!`).

However, strictly speaking, it seems to me that changing the above to not warn will require changing

```rust
fn foo() {
    while bar {}
    foo()
}
```

to also not warn since it could be that the `while` is an infinite loop and doesn't ever hit the `foo`.

I'm inclined to think we let these cases warn since true edge cases like the first one seem rare, and if they do occur they seem like they would usually be typos in the function call. (I could imagine someone accidentally having code like `fn foo() { assert!(bar()); foo() /* meant to be boo() */ }` which won't warn if the `loop` case is "fixed".)

(Part of the reason this is unresolved is wanting feedback, part of the reason is I couldn't devise a strategy that worked in all cases.)

---

The name `unconditional_self_calls` is kinda clunky; and this reconstructs the CFG for each function that is linted which may or may not be very expensive, I don't know.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants