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

unconditional_recursion lint doesn't work with struct update syntax #78474

Closed
keiichiw opened this issue Oct 28, 2020 · 5 comments · Fixed by #92889
Closed

unconditional_recursion lint doesn't work with struct update syntax #78474

keiichiw opened this issue Oct 28, 2020 · 5 comments · Fixed by #92889
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@keiichiw
Copy link

Code

I tried this code which causes a stack overflow due to a recursive call of Default::default() I mistakenly added.

struct S {
    x: i32,
    y: i32,
}

fn f() -> i32 {
    42
}

impl Default for S {
    fn default() -> S {
        S {
            x: f(),
            ..Default::default() // cause stack overflow
        }
    }
}

fn main() {
    let _ = S::default();
}

I expected to see the unconditional_recursion warning. However, the compiler didn't complain it.

Version it worked on

It most recently worked on: 1.43.0 showed the warning. (ref. the compiler explorer)

Version with regression

The warning isn't shown on 1.44.0 or later, including the current stable 1.47.0.

@jonas-schievink jonas-schievink added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 28, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 28, 2020
@jonas-schievink
Copy link
Contributor

I think this was caused by #70822, but it looks like expected behavior: If f diverges, the self-recursion never happens.

@jonas-schievink
Copy link
Contributor

Though maybe the lint should assume that functions returning an inhabited type won't diverge (since otherwise they should be marked -> !). I can definitely see why people would write code like this, so linting on it makes sense.

@lcnr
Copy link
Contributor

lcnr commented Oct 28, 2020

I personally feel that we should lint for functions if they don't return an uninhabited type.
Even if someone wants to exit recursion by unwinding I do feel that a lint is still appropriate here
as it is really bad style imo.

@keiichiw
Copy link
Author

keiichiw commented Oct 28, 2020

Thanks for the quick response. Yeah, it'd be natural to expect a function whose return type is not ! to return eventually.

Probably, we might want to have the following rules?:

  1. Lint for self-recursion unless a function returning an uninhabited type is called in advance.
  2. If a non-diverge function is called before a self-recursion, show a message like "if this f diverges, please annotate it." in addition to the unconditional_recursion warning.

1 would help my case and 2 would help the case of #54444. (2 is not necessary, though.)

@spastorino
Copy link
Member

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

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. C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants