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

Terse diagnostic for never type fallback lint warning involving try operator and placeholder in path #132358

Open
hniksic opened this issue Oct 30, 2024 · 5 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. L-dependency_on_unit_never_type_fallback Lint: dependency_on_unit_never_type_fallback T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hniksic
Copy link
Contributor

hniksic commented Oct 30, 2024

Compiling the following function warns in Rust 1.81 and later:

pub fn foo() -> std::io::Result<()> {
    [1, 2, 3]
        .into_iter()
        .map(|_| -> std::io::Result<_> { Ok(()) })
        .collect::<std::io::Result<_>>()?;
    Ok(())
}

I expected it to compile without warnings, as it did with previous Rust versions. It currently produces this warning:

warning: this function depends on never type fallback being `()`
 --> src/lib.rs:1:1
  |
1 | pub fn foo() -> std::io::Result<()> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #123748 <https://github.com/rust-lang/rust/issues/123748>
  = help: specify the types explicitly
note: in edition 2024, the requirement `!: FromIterator<()>` will fail
 --> src/lib.rs:5:20
  |
5 |         .collect::<std::io::Result<_>>()?;
  |                    ^^^^^^^^^^^^^^^^^^
  = note: `#[warn(dependency_on_unit_never_type_fallback)]` on by default

Playground

What is particularly perplexing about this warning is that I don't understand how the never type is even involved, as nothing in the code appears to diverge.

The current workaround is to add let () = ... before the iteration, which compiles without warnings:

// no warning
pub fn foo() -> std::io::Result<()> {
    let () = [1, 2, 3]
        .into_iter()
        .map(|_| -> std::io::Result<_> { Ok(()) })
        .collect::<std::io::Result<_>>()?;
    Ok(())
}

While this workaround works, it feels unnecessary and it's hard to explain and justify.

@hniksic hniksic added the C-bug Category: This is a bug. label Oct 30, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 30, 2024
@jieyouxu jieyouxu added the F-never_type `#![feature(never_type)]` label Oct 30, 2024
@fmease fmease added L-dependency_on_unit_never_type_fallback Lint: dependency_on_unit_never_type_fallback T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed F-never_type `#![feature(never_type)]` needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 30, 2024
@fmease
Copy link
Member

fmease commented Oct 30, 2024

Triage: Works as intended. Edit: Let me explain.

@fmease fmease added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. labels Oct 30, 2024
@fmease
Copy link
Member

fmease commented Oct 30, 2024

The "culprit" is the _ inside .collect::<std::io::Result<_>>()? and the "trigger" is the try operator ?.

What is particularly perplexing about this warning is that I don't understand how the never type is even involved, as nothing in the code appears to diverge.

The try operator ? diverges on (here) Err. For Result, you can think of x? as expanding to the following1:

match x { Ok(x) => x, Err(e) => return Err(e.into()), }

Notice the return which clearly diverges :)

So Rust <=2021 the aforementioned _ defaults/falls back to ()2 because one the match arms diverges and nothing else constrains it, so the whole line means .collect::<std::io::Result<()>>()?.

In Rust >=2024 we want to fall back to ! in "these" cases (see #123748 for more details). However, as you can imagine ! doesn't implement FromIterator<()> contrary to () meaning your code won't compile in the 2024 edition (and onward). That's why this lint triggers. You can observe this by setting the edition to 2024 on the playground you've linked.

Footnotes

  1. However the actual desugaring looks different and is more general. You can see it yourself by clicking the button labeled HIR on your playground.

  2. Note that it's not due to foo's return type std::io::Result<()> that it defaults to ().

@fmease
Copy link
Member

fmease commented Oct 30, 2024

Instead of adding let () = / () = before [1, 2, 3], you can just make the following change (which looks more legible to me):

-         .collect::<std::io::Result<_>>()?;
+         .collect::<std::io::Result<()>>()?;

@hniksic
Copy link
Contributor Author

hniksic commented Oct 30, 2024

Thanks for the detailed explanation and a nicer workaround. I cannot help but get an impression that, when this decision was being made, insufficient weight was given to the fact that a frequent and useful construct was now becoming an error. But I suppose that ship has sailed.

The proposed change is indeed much better than the let () = ... approach, and is what I'm now using in my code.

Finally, I'd like to point out that the diagnostic is really inadequate. I am very far from being a Rust beginner, and I was totally stumped by it. Speaking of "never type fallbacking to ()" seemed like a bug because the never type is usually associated with panicking and aborting, neither of which is not present here. A new or intermediate user will probably have no idea how to proceed given this diagnostic. If there is no such issue already, I'd like to seriously suggest that the compiler detect this issue in ? and provide a diagnostic more focused on that case.

@compiler-errors
Copy link
Member

Here's a more minimal example:

pub fn foo() -> Result<(), ()> {
    match Ok(Default::default()) {
        Ok(k) => k,
        Err(e) => return Err(e),
    };
    Ok(())
}

I believe we can probably get a machine-applicable suggestion to change _ to (), and to turn Trait::method() into <() as Trait>::method(), and to annotate the type of variables to : () where possible. That should cover 99.9% of the cases, I would expect.

@fmease fmease added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-diagnostics Area: Messages for errors, warnings, and lints D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. and removed C-discussion Category: Discussion or questions that doesn't represent real issues. labels Oct 31, 2024
@fmease fmease changed the title Spurious never type fallback warning Terse lint diagnostic on never type fallback warning involving try operator an placeholder in path Oct 31, 2024
@fmease fmease changed the title Terse lint diagnostic on never type fallback warning involving try operator an placeholder in path Terse diagnostic for never type fallback lint warning involving try operator an placeholder in path Oct 31, 2024
@fmease fmease changed the title Terse diagnostic for never type fallback lint warning involving try operator an placeholder in path Terse diagnostic for never type fallback lint warning involving try operator and placeholder in path Oct 31, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 1, 2024
…k-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? `@WaffleLapkin`

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
jieyouxu added a commit to jieyouxu/rust that referenced this issue Nov 1, 2024
…k-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? ``@WaffleLapkin``

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 1, 2024
…k-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? ```@WaffleLapkin```

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 2, 2024
Rollup merge of rust-lang#132383 - compiler-errors:never-type-fallback-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? ```@WaffleLapkin```

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. L-dependency_on_unit_never_type_fallback Lint: dependency_on_unit_never_type_fallback T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants