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

Warn on circular code #11032

Open
orowith2os opened this issue Jun 26, 2023 · 7 comments
Open

Warn on circular code #11032

orowith2os opened this issue Jun 26, 2023 · 7 comments
Labels
A-lint Area: New lints

Comments

@orowith2os
Copy link

orowith2os commented Jun 26, 2023

What it does

When code goes in a circular loop (fn1 calls fn 2, which then calls fn1), Clippy will emit a warning about this behavior, so the programmer won't have to wonder why their program apparently hangs.

In debug mode, it will crash with the following:

thread 'main' has overflowed its stack
fatal runtime error: stack overflow

This would be something along the lines of unconditional_recursion in rustc, unsure if these checks need to be in two separate places.

See #428 for a previous issue.

Advantage

Circular Rust code now won't compile without the programmer knowing it's circular, but can still do so if desired.

Drawbacks

None

Example

fn output1() {
    output2();
}

fn output2() {
    output1();
}

fn main() {
    output1();
}
@orowith2os orowith2os added the A-lint Area: New lints label Jun 26, 2023
@Noratrieb
Copy link
Member

Noratrieb commented Jun 26, 2023

related rust-lang/rust issue: rust-lang/rust#57965
This was implemented in rustc in rust-lang/rust#75067, but it was a significant perf regression.

@Centri3
Copy link
Member

Centri3 commented Jun 26, 2023

I think it's reasonable in the case that it's 100% irrefutable and does nothing big before it, like output1 -> output2 in the example given, anything more complex though it shouldn't be added yeah due to performance reasons

Another problem then though is whether that should even be added in the first place, as if it only lints the simplest of conditions it'll likely not be too helpful

@orowith2os
Copy link
Author

It might be useful to get a visual representation of a situation like this from the compiler, and go from there? Not sure if any tools exist for that already, but would be nice.

@Centri3
Copy link
Member

Centri3 commented Jun 26, 2023

Tools definitely do, debuggers :D But yeah if it wasn't a severe performance hazard then I'd be 100% for the compiler warning of this

@orowith2os
Copy link
Author

Oh, I meant more a GUI, like a graph. Would make it easier to see what's going in loops and what isn't, then I can maybe poke around and see if I can figure anything out (or redo all the mistakes that came before)

@orowith2os
Copy link
Author

Thoughts

Maybe a more thorough checking mode would be ideal? That way you have the normal stuff, day-to-day mistakes, and then you have more troublesome things like this.

The former would be used normally, on each compile, and the latter can be used e.g. in CI, or pre-commit hooks.

@Centri3
Copy link
Member

Centri3 commented Jun 29, 2023

As it stands currently lints must be ran even if they're always allowed, so that isn't really possible in current clippy/rustc. See rust-lang/rust#106983

If it's the same lint that would work better but just the simpler implementation would already be quite a bit slower, afaik

bors added a commit that referenced this issue Mar 13, 2024
lint when calling the blanket `Into` impl from a `From` impl

Closes #11150
```
warning: function cannot return without recursing
  --> x.rs:9:9
   |
9  | /         fn from(value: f32) -> Self {
10 | |             value.into()
11 | |         }
   | |_________^
   |
note: recursive call site
  --> x.rs:10:13
   |
10 |             value.into()
   |             ^^^^^^^^^^^^
```

I'm also thinking that we can probably generalize this lint to #11032 at some point (instead of hardcoding a bunch of impls), like how rustc's `unconditional_recursion` works, at least up to one indirect call, but this still seems useful for now :)

I've also noticed that we use `fn_def_id` in a bunch of lints and then try to get the node args of the call in a separate step, so I made a helper function that does both in one. I intend to refactor a bunch of uses of `fn_def_id` to use this later

I can add more test cases, but this is already using much of the same logic that exists for the other impls that this lint looks for (e.g. making sure that there are no conditional returns).

changelog: [`unconditional_recursion`]: emit a warning inside of `From::from` when unconditionally calling the blanket `.into()` impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

3 participants