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

error message when unit test type does not implement Termination is ungreat #50291

Closed
nikomatsakis opened this issue Apr 27, 2018 · 9 comments · Fixed by #103445
Closed

error message when unit test type does not implement Termination is ungreat #50291

nikomatsakis opened this issue Apr 27, 2018 · 9 comments · Fixed by #103445
Assignees
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-diagnostics Area: Messages for errors, warnings, and lints A-libtest Area: `#[test]` / the `test` library C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 27, 2018

Edit: outstanding work is to change the span from pointing at the function body to point at the return type.

#50272 adds a test for the case where a unit test returns a value that does not implement Termination. The message currently talks about main and has an ugly multi-line span:

error[E0277]: `main` has invalid return type `std::result::Result<f32, std::num::ParseIntError>`
  --> $DIR/termination-trait-test-wrong-type.rs:18:1
   |
LL | / fn can_parse_zero_as_f32() -> Result<f32, ParseIntError> { //~ ERROR
LL | |     "0".parse()
LL | | }
   | |_^ `main` can only return types that implement `std::process::Termination`
   |
   = help: the trait `std::process::Termination` is not implemented for `std::result::Result<f32, std::num::ParseIntError>`
   = note: required by `__test::test::assert_test_result`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
@nikomatsakis nikomatsakis added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 27, 2018
@estebank
Copy link
Contributor

As long as we have the method signature around when we emit E0277, it has a field with the span for the return type, which should be used for this instead, but this will probably require some special casing this in that (already quite hairy) diagnostic code. Extending rustc_on_unimplemented could be possible, but I would advice against doing so to avoid turning that attribute into a bigger monster than it already is.

error[E0277]: `main` has invalid return type `std::result::Result<f32, std::num::ParseIntError>`
  --> $DIR/termination-trait-test-wrong-type.rs:18:1
   |
LL | fn can_parse_zero_as_f32() -> Result<f32, ParseIntError> { //~ ERROR
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't implement `std::process::Termination`
   |
   = help: the trait `std::process::Termination` is not implemented for `std::result::Result<f32, std::num::ParseIntError>`
   = note: required by `__test::test::assert_test_result`

@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Oct 2, 2018
@shepmaster
Copy link
Member

shepmaster commented Jul 10, 2019

Chiming in that this confused me as I was preparing for a Rust training session, where I expect it would confuse the students even more. Specifically, this should not talk about main at all as it's plausible to cause people to look in the wrong spot.

@estebank
Copy link
Contributor

CC #60179

@azalutsky
Copy link

Just started learning Rust and came across this error while writing some tests- I absolutely agree "ungreat" is perfect word choice.

@GoldsteinE
Copy link
Contributor

GoldsteinE commented Oct 14, 2022

It could get even more confusing when using Termination trait as a bound:

use std::process::Termination;

fn foo<T: Termination>(_: T) {}

fn main() {
    foo(1_i32);
}
error[E0277]: `main` has invalid return type `i32`
 --> src/main.rs:6:9
  |
6 |     foo(1_i32);
  |     --- ^^^^^ `main` can only return types that implement `Termination`
  |     |
  |     required by a bound introduced by this call
  |
  = help: the trait `Termination` is not implemented for `i32`
note: required by a bound in `foo`
 --> src/main.rs:3:11
  |
3 | fn foo<T: Termination>(_: T) {}
  |           ^^^^^^^^^^^ required by this bound in `foo`

Rust 1.64

@fmease
Copy link
Member

fmease commented Oct 19, 2022

This issue is now (mostly) fixed on nightly by #103142 and should be closed I think. The error message is now correct but the span is still “ugly”. I've overlooked this issue and only closed its duplicate, namely #103052.

@estebank
Copy link
Contributor

Actually, let's keep this open only for the span change :)

@estebank estebank reopened this Oct 19, 2022
@estebank estebank added A-libtest Area: `#[test]` / the `test` library D-papercut Diagnostics: An error or lint that needs small tweaks. labels Oct 19, 2022
@necimye
Copy link

necimye commented Oct 22, 2022

Just wrap the return type with a struct and implement the Termination trait for that wrapper struct
example


struct Complex(Box<dyn Fn(i32) -> i32>);
impl Termination for Complex {
    fn report(self) -> std::process::ExitCode {
        ExitCode::SUCCESS
    }
}
#[test]
#[allow(dead_code)]
fn returns_closure() -> Complex {
    Complex(Box::new(|x| x + 1))
}

@fmease
Copy link
Member

fmease commented Oct 23, 2022

@necimye Note that this GitHub issue is about the subpar error message given by the compiler which should be fixed and not about potential solutions for user code when facing this message.

@rustbot claim

Manishearth added a commit to Manishearth/rust that referenced this issue Nov 11, 2022
`#[test]`: Point at return type if `Termination` bound is unsatisfied

Together with rust-lang#103142 (already merged) this fully fixes rust-lang#50291.

I don't consider my current solution of changing a few spans “here and there” very clean since the
failed obligation is a `FunctionArgumentObligation` and we point at a type instead of a function argument.

If you agree with me on this point, I can offer to keep the spans of the existing nodes and instead inject
`let _: AssertRetTyIsTermination<$ret_ty>;` (type to be defined in `libtest`) similar to `AssertParamIsEq` etc.
used by some built-in derive-macros.

I haven't tried that approach yet though and cannot promise that it would actually work out or
be “cleaner” for that matter.

`@rustbot` label A-libtest A-diagnostics
r? `@estebank`
Manishearth added a commit to Manishearth/rust that referenced this issue Nov 11, 2022
`#[test]`: Point at return type if `Termination` bound is unsatisfied

Together with rust-lang#103142 (already merged) this fully fixes rust-lang#50291.

I don't consider my current solution of changing a few spans “here and there” very clean since the
failed obligation is a `FunctionArgumentObligation` and we point at a type instead of a function argument.

If you agree with me on this point, I can offer to keep the spans of the existing nodes and instead inject
`let _: AssertRetTyIsTermination<$ret_ty>;` (type to be defined in `libtest`) similar to `AssertParamIsEq` etc.
used by some built-in derive-macros.

I haven't tried that approach yet though and cannot promise that it would actually work out or
be “cleaner” for that matter.

``@rustbot`` label A-libtest A-diagnostics
r? ``@estebank``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Nov 11, 2022
`#[test]`: Point at return type if `Termination` bound is unsatisfied

Together with rust-lang#103142 (already merged) this fully fixes rust-lang#50291.

I don't consider my current solution of changing a few spans “here and there” very clean since the
failed obligation is a `FunctionArgumentObligation` and we point at a type instead of a function argument.

If you agree with me on this point, I can offer to keep the spans of the existing nodes and instead inject
`let _: AssertRetTyIsTermination<$ret_ty>;` (type to be defined in `libtest`) similar to `AssertParamIsEq` etc.
used by some built-in derive-macros.

I haven't tried that approach yet though and cannot promise that it would actually work out or
be “cleaner” for that matter.

```@rustbot``` label A-libtest A-diagnostics
r? ```@estebank```
@bors bors closed this as completed in bc9567f Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-diagnostics Area: Messages for errors, warnings, and lints A-libtest Area: `#[test]` / the `test` library C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants