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

Don't suggest break through nested items #112024

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

compiler-errors
Copy link
Member

Fixes #112020

@rustbot
Copy link
Collaborator

rustbot commented May 27, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 27, 2023
@SparkyPotato
Copy link
Contributor

If I'm not mistaken, this still suggests breaking out of the loop inside a closure - not sure what hir::Node variant closures have.

In the nested item case, currently, the compiler doesn't suggest breaking out of the loop, but instead returning (but only if the type and return types match):

error[E0308]: mismatched types
 --> src/lib.rs:5:17
  |
4 | /             if true {
5 | |                 Err(1)
  | |                 ^^^^^^ expected `()`, found `Result<_, {integer}>`
6 | |             }
  | |_____________- expected this to be `()`
  |
  = note: expected unit type `()`
                  found enum `Result<_, {integer}>`
help: you might have meant to return this value
  |
5 |                 return Err(1);
  |                 ++++++       +

@compiler-errors
Copy link
Member Author

compiler-errors commented May 28, 2023

lol, @SparkyPotato I should've read my own UI test stderr. I had made this work for closures, then refactored it for items and didn't check that I broke it for closures 😭

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2023
@compiler-errors
Copy link
Member Author

In the nested item case, currently, the compiler doesn't suggest breaking out of the loop

This does suggest breaking out of the loop currently:

fn b() {
    let _ = loop {
        fn foo() -> Result<(), ()> {
            if true {
                Err(1)
                //~^ ERROR mismatched types
            }
            Err(())
        }
    };
}
error[[E0308]](https://doc.rust-lang.org/nightly/error_codes/E0308.html): mismatched types
 --> src/lib.rs:5:17
  |
4 | /             if true {
5 | |                 Err(1)
  | |                 ^^^^^^ expected `()`, found `Result<_, {integer}>`
6 | |                 //~^ ERROR mismatched types
7 | |             }
  | |_____________- expected this to be `()`
  |
  = note: expected unit type `()`
                  found enum `Result<_, {integer}>`
help: you might have meant to break the loop with this value
  |
5 |                 break Err(1);
  |                 +++++       +

But that's fixed by this PR.

In the nested item case, currently, the compiler doesn't suggest breaking out of the loop, but instead returning (but only if the type and return types match)

Can you give a more complete example? Not sure if you're asking for something additional that needs fixing, or just making an observation.

@SparkyPotato
Copy link
Contributor

SparkyPotato commented May 28, 2023

On stable:

fn test() {
    loop {
        fn x() -> Result<(), i32> {
            if true {
                Err(1)
            }
            
            Ok(())
        }
    }
}

suggests returning the value, while:

fn test() {
    loop {
        fn x() -> Result<(), ()> {
            if true {
                Err(1)
            }
            
            Ok(())
        }
    }
}

has no suggestions.

Seemingly, the let _ = changes the behavior here, while it didn't make a difference for the closure case.

@compiler-errors
Copy link
Member Author

Seemingly, the let _ = changes the behavior here, while it didn't make a difference for the closure case.

That's because the diagnostic checks that there's a let statement in some parent HIR node. I don't think that's necessarily the best heuristic, but if you'd like, you should dive deeper into other ways to make this diagnostic give wonky suggestions and possibly fix those too 😸

@compiler-errors compiler-errors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 28, 2023
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with the async blocks handled (and probably also const blocks too?)

Comment on lines +5 to +10
Err(1)
//~^ ERROR mismatched types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I would expect this to suggest return. (still better than the status quo ig)

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2023
@compiler-errors
Copy link
Member Author

Async blocks were already handled, but good catch on const blocks.

@bors r=WaffleLapkin

@bors
Copy link
Contributor

bors commented May 31, 2023

📌 Commit 0a51ab9 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108459 (rustdoc: Fix LinkReplacer link matching)
 - rust-lang#111318 (Add a distinct `OperandValue::ZeroSized` variant for ZSTs)
 - rust-lang#111892 (rustdoc: add interaction delays for tooltip popovers)
 - rust-lang#111980 (Preserve substs in opaques recorded in typeck results)
 - rust-lang#112024 (Don't suggest break through nested items)
 - rust-lang#112128 (Don't compute inlining status of mono items in advance.)
 - rust-lang#112141 (remove reference to Into in ? operator core/std docs, fix rust-lang#111655)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 02c4b4b into rust-lang:master Jun 1, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 1, 2023
@compiler-errors compiler-errors deleted the dont-break-thru-item branch August 11, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

break loop help ignores nested closures
5 participants