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

Do not emit note in coerce if loop iterates at least once #122679

Closed

Conversation

gurry
Copy link
Contributor

@gurry gurry commented Mar 18, 2024

Fixes #122561

The fix is a bit conservative. It covers only the cases when it is easy to deduce the loop iteration count such as when the expr iterated over is a range or an array literal. It does not cover more involved cases such as when it is a local, a vec or a method call.

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 Mar 18, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I don't think we should be suppressing this without replacing it with a better error message and suggestion.

For example, we should explain why Rust doesn't know that 0..10 won't loop at least once, and perhaps we should suggest adding an unreachable!("...") after the loop?

@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 Mar 18, 2024
@gurry
Copy link
Contributor Author

gurry commented Mar 19, 2024

I don't think we should be suppressing this without replacing it with a better error message and suggestion.

For example, we should explain why Rust doesn't know that 0..10 won't loop at least once, and perhaps we should suggest adding an unreachable!("...") after the loop?

Agreed. I'll make the changes.

@bors
Copy link
Contributor

bors commented Mar 22, 2024

☔ The latest upstream changes (presumably #120926) made this pull request unmergeable. Please resolve the merge conflicts.

@gurry gurry force-pushed the 122561-bad-note-non-zero-loop-iters branch from e2c6942 to 0d52c10 Compare March 25, 2024 07:27
@rust-log-analyzer

This comment has been minimized.

@gurry
Copy link
Contributor Author

gurry commented Mar 25, 2024

Actually, the for loop failing to type check has nothing to do with iteration count. All the three functions shown below, for instance, fail with the exact same type mismatch error even though they have different iteration counts - infinite, finite but non-zero and zero:

fn infinite() -> bool {
   for _i in 0.. {
       return false;
   }
}

fn finite_atleast_once() -> bool {
   for _i in 0..3 {
       return false;
   }
}

fn zero_times() -> bool {
   for _i in 0..0 {
       return false;
   }
}

(Playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=49c635d409f3f5bd70031d89d183edf4)

The error actually stems from the for loop desugaring. This is what the bodies of all the above functions desugar to:

fn func()  -> bool {
        {
                let _t =
                    match #[lang = "into_iter"](#[lang = "Range"]{
                                        start: ...,
                                        end: ...,}) {
                            mut iter =>
                                loop {
                                        match #[lang = "next"](&mut iter) {
                                                #[lang = "None"] {} => break,
                                                #[lang = "Some"] {  0: i } => { return false; }
                                            }
                                    },
                        };
                _t
            }
    }

As you can see, the type of the tail expr _t here in will always be () and therefore we will always get the type mismatch error between () and bool irrespective of iteration count.

Given that, the current note, the function expects a value to always be returned, but loops might run zero times, is wrong and misleading. The right fix here therefore is to remove this note (and the related notes) and replace it with something that correctly identifies the problem without being too jargon-y for the user.

I will open a different PR with that change and close this one.

@gurry gurry closed this Mar 25, 2024
@gurry gurry force-pushed the 122561-bad-note-non-zero-loop-iters branch from 0d52c10 to fa374a8 Compare March 25, 2024 08:15
@gurry gurry deleted the 122561-bad-note-non-zero-loop-iters branch March 25, 2024 08:20
@gurry
Copy link
Contributor Author

gurry commented Mar 27, 2024

I will open a different PR with that change and close this one.

Here's the new PR: #123125

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
…iters-2, r=estebank

Remove suggestion about iteration count in coerce

Fixes rust-lang#122561

The iteration count-centric suggestion was implemented in PR rust-lang#100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang#122679 (comment))

This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value.

It should also fix rust-lang#100285 by simply making it redundant.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
…iters-2, r=estebank

Remove suggestion about iteration count in coerce

Fixes rust-lang#122561

The iteration count-centric suggestion was implemented in PR rust-lang#100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang#122679 (comment))

This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value.

It should also fix rust-lang#100285 by simply making it redundant.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 11, 2024
…r=estebank

Remove suggestion about iteration count in coerce

Fixes #122561

The iteration count-centric suggestion was implemented in PR #100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang/rust#122679 (comment))

This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value.

It should also fix #100285 by simply making it redundant.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request May 18, 2024
…r=estebank

Remove suggestion about iteration count in coerce

Fixes #122561

The iteration count-centric suggestion was implemented in PR #100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang/rust#122679 (comment))

This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value.

It should also fix #100285 by simply making it redundant.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 24, 2024
…r=estebank

Remove suggestion about iteration count in coerce

Fixes #122561

The iteration count-centric suggestion was implemented in PR #100094, but it was based on the wrong assumption that the type mismatch error depends on the number of times the loop iterates. As it turns out, that is not true (see this comment for details: rust-lang/rust#122679 (comment))

This PR attempts to remedy the situation by changing the suggestion from the one centered on iteration count to a simple suggestion to add a return value.

It should also fix #100285 by simply making it redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Invalid note: "the function expects a value to always be returned, but loops might run zero times"
5 participants