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 diagnostics to "while loop" and "for loop" that note that it is always determined that it migh t iterate zero times. #126510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kohei316
Copy link
Contributor

close #116572
When a user writes a loop expression as a tail expression, which has a loop header always executed, and which contains return expressions like this,

fn foo() -> bool {
    while let x = 0 {
        return true
    }
}

this,

fn foo() -> bool {
    while true {
        return true
    }
}

or this,

fn foo() -> bool {
    for i in 0..10 {
        return true
    }
}

they may be misundrestanding that it evaluate to not a unit type but un argument of the return expression.
This PR adds diagnositics which help clearing such misundrestanding.

@rustbot
Copy link
Collaborator

rustbot commented Jun 15, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jun 15, 2024
@Kohei316 Kohei316 changed the title Add diagnostics to "while loop" and "for loop" that note that it is a Add diagnostics to "while loop" and "for loop" that note that it is always determined that it migh t iterate zero times. Jun 15, 2024
@rust-log-analyzer

This comment has been minimized.

loop_span,
format!("It is determined that this might iterate zero times, regardless of the {loop_header}.")
);
err.span_help(loop_span, "If you are assuming that it will iterate more than once, consider using a `loop` expression instead.");

This comment was marked as resolved.

…lways determined that it might iterate zero times.
LL + /* `bool` value */
|

error: aborting due to 1 previous error
Copy link
Contributor

Choose a reason for hiding this comment

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

A number of issues here.

  • Error messages don't start with an upper-case letter, and don't end with a '.'.
  • There is a lot of repetition here. The note and the following help both print the loop condition. The two could be merged. I am also not sure if the condition needs to be printed, given that the loop is printed above anyway.
  • "It is determined" is too wordy, something like "this loop might iterate zero times" would be better.

But there are bigger concerns, which I will discuss below.

@nnethercote
Copy link
Contributor

There's a big problem here: the error messages mentioned in #116572 have since changed greatly. E.g. for the first example in that issue it now just says "while loops evaluate to unit type ()" which I find sufficiently informative. In #116572 @Nadrieril wrote:

A workable solution would be instead to change the error message: the code that emits "this might have zero elements to iterate on" could check if the pattern is a single binding and emit a better error message if that's the case.

But because the message has changed so much, there is now no mention of zero elements, so this PR isn't following that suggestion.

#122561 was also complaining about the old message, and #123125 was the PR that changed the message in order to fix #122561. So I think this PR is not actually needed, and #116572 can now be closed. But I will ask Esteban, who reviewed #123125, for a second opinion, because this is a complicated situation and is stretching my expertise on error messages to breaking point.

r? @estebank

@rustbot rustbot assigned estebank and unassigned nnethercote Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logically identical code. One compiles, one does not
6 participants