Remove suggestion about iteration count in coerce

[gurry]

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: #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.

[rustbot]

r? @estebank

rustbot has assigned @estebank.
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

[bors]

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

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[gurry]

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

Rebased

[gurry]

Some changes occurred in src/tools/cargo

cc @ehuss

This was due to a bad commit, which I've fixed now.

[estebank]

I'm not entirely on board with completely removing the previous message, because I feel that we'd be giving an obscure message still. Instead I would add logic to it in order to detect a few cases:

1. when the range is known to never run, mention that and modify the other messages to account for it so they make sense
2. when there's loop { return val; } with no conditions, to explain that "rustc is not smart enough to understand that val will always be returned" (although ideally we'd have a lang semantic change to account for that so that we don't error at all in that case)
3. when there's a loop { if condition { return val; } } with the current explanation (tweaked to make it clearer that it's not just the loop might not run even once, but also that it might run multiple times but never reach the return before the loop breaks)

[estebank]

I don't think that this reversal is reasonable, because the explanation for this case is exactly right: for i in 0..0 will never iterate, right?

[gurry]

Yes, for i in 0..0 will never iterate, but that is not what is causing the error since for i in 0..100 which definitely iterates still results in the same error (see #123125 (comment)).

Since the error is not due to iteration count, the reversal is reasonable IMHO.

[estebank]

Same here. We should even be detecting explicitly known to never iterate ranges, where the start and the end of a .. range are the same, because the entire thing is for sure dead code.

[gurry]

Yes, it would be a good idea to warn the user that for i in 0..0 { ... } is dead code. Maybe I can do that in a different PR.

[gurry]

I'm not entirely on board with completely removing the previous message, because I feel that we'd be giving an obscure message still. Instead I would add logic to it in order to detect a few cases:

I agree that we should provide more context. However, any notes and explanations that talk about the loop iteration count are flawed IMHO because the error has nothing to do with loop iteration count.

For example, below snippet in which the loop runs zero times triggers the type mismatch error:

fn test() -> bool {
    for i in 0..0 {
        return true;
    }
}

but so does this one in which the loop runs 5 times:

fn test() -> bool {
    for i in 0..5 {
        return true;
    }
}

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

The reality is that the error actually stems from the peculiar structure of the for loop desugaring not the whether the loop iterates or not (see this comment for details: #122679 (comment))

In light of this underlying reality, the current set of notes (like "the function expects a value to always be returned, but loops might run zero times") are incorrect and somewhat misleading because they seem to attribute the error to the loop iteration count.

If we must add any explanation, it should be about the for loop desugaring which is the actual cause. However, I couldn't find any good way to word it which would not get into the compiler internals. If you can suggest a user friendly text I'd love to add that.

1. when the range is known to never run, mention that and modify the other messages to account for it so they make sense

I actually had done just that in my now abandoned PR #122679, wherein I computed the actual iteration count for the loop and showed different notes depending on that, but then I realised no amount sophistication in suggestions centered on loop iteration count actually help here because the cause of the error is entirely something else.

2. when there's loop { return val; } with no conditions, to explain that "rustc is not smart enough to understand that val will always be returned" (although ideally we'd have a lang semantic change to account for that so that we don't error at all in that case)
3. when there's a loop { if condition { return val; } } with the current explanation (tweaked to make it clearer that it's not just the loop might not run even once, but also that it might run multiple times but never reach the return before the loop breaks)

Oddly enough, the plain vanilla loop actually doesn't trigger the type mismatch error. Both cases 2. and 3. you mentioned compile without any error as you can see in this Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c2bc903c88d86cd88cb23e9b88f99d88). So the question of adding any explanation for them does not arise.

[estebank]

Thanks! The comment on the other PR gives appropriate context. Now I'm trying to think what the appropriate explanation of the situation might be. It honestly might be reasonable to explain "yes, this desugars to something else that evaluates to (), sorry".

[gurry]

It honestly might be reasonable to explain "yes, this desugars to something else that evaluates to (), sorry".

"Desugar" may not be a term well understood outside of compiler engineering. Maybe something along the lines of, "for loops always evaluate to type ()".

[gurry]

It honestly might be reasonable to explain "yes, this desugars to something else that evaluates to (), sorry".

"Desugar" may not be a term well understood outside of compiler engineering. Maybe something along the lines of, "for loops always evaluate to type ()".

I have now added the note "(for|while) loops evaluate to unit type ()".

@rustbot label -S-waiting-on-author

[estebank]

Here we are providing effectively the same suggestion twice. We need to find a way to avoid one or the other (or ideally, unify the logic to a single place, particularly because the pre-existing suggestion gives you the actual value to use in this case).

[gurry]

Done.

The suggestion is now invoked from emit_type_mismatch_suggestions() in demand.rs which ensures it will not be called if the other suggestion has already been emitted.

[gurry]

@estebank please review

[estebank]

@bors r+

[bors]

📌 Commit fa73e37 has been approved by estebank

It is now in the queue for this repository.

[bors]

⌛ Testing commit fa73e37 with merge b16d83c...

[bors]

💔 Test failed - checks-actions

[gurry]

Build had failed due to a merge conflict. Rebased now. Please re-approve @estebank

[bors]

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

[gurry]

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

Rebased

[gurry]

@estebank Failing build fixed. Please review again

[Dylan-DPC]

@bors r=estebank

[bors]

📌 Commit 6289ed8 has been approved by estebank

It is now in the queue for this repository.

[bors]

🌲 The tree is currently closed for pull requests below priority 9999. This pull request will be tested once the tree is reopened.

[bors]

⌛ Testing commit 6289ed8 with merge 06e88c3...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 06e88c3 to master...

[rust-timer]

Finished benchmarking commit (06e88c3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.8%	[3.8%, 3.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-5.2%	[-5.2%, -5.2%]	1
All ❌✅ (primary)	1.5%	[-0.7%, 3.8%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.151s -> 673.364s (-0.41%)
Artifact size: 315.84 MiB -> 315.81 MiB (-0.01%)