don't gratuitously error on tests returning Result with lifetime

[zackmdavis]

(Personal note: I'm not dead; I just didn't have time for Rust for, um, seventeen months, because I had to fight a religious war over the philosophy of language in my local apocalyptic robot cult. I won, sort of. Um, I think according to the letter of RFC 2689, I was supposed to be demoted from compiler team contributors to alumni status on inactivity grounds, but it doesn't look like anyone noticed??)

---

Sander Maijers reports that #[test] functions that return a Result with a named lifetime fail to compile with a "functions used as tests must have signature fn() -> ()" error message—but that can't really be right, because #[test] functions that return a Result with a static lifetime are accepted!

It turns out that this error message dates all the way back to April 2013's 5f1a90e ("Issue 4391: rustc should not silently skip tests with erroneous signature."). But after RFC 1937, we actually do accept non-unit return types in tests. So the check to which this 2013 error message has survived attached, looking to see if the generic params are empty, can be deleted: non-empty generic params aren't necessarily an error anymore. Having been deleted, the match expression simplifies down to a conditional.

This resolves #55228, and is of historical interest to #4391.

r? @estebank

[estebank]

Nice to see you back!

Wouldn't this change also incorrectly allow #[test] fn foo<T: Bar>() -> <T as Bar>::Baz { <T as Bar>::Baz }? I think it is correct for us to force people to use 'static for the presented case, to reduce confusion for newcomers. In that case, then we should probably just update the wording of the error.

[zackmdavis]

@estebank

Wouldn't this change also incorrectly allow #[test] fn foo<T: Bar>() -> <T as Bar>::Baz { <T as Bar>::Baz }?

I don't think so. When I tried fleshing out your proposed counterexample into something that "gets closer to" compiling, I ended up with:

struct Quux;

trait Bar {
    type Baz;

    fn rah(&self) -> Self::Baz;
}

impl Bar for Quux {
    type Baz = String;

    fn rah(&self) -> Self::Baz {
        "rah".to_owned()
    }
}

#[test]
fn foo<T: Bar<Baz = String>>() -> <T as Bar>::Baz {
    let q = Quux{};
    <Quux as Bar>::rah(&q)
}

which fails:

error[E0282]: type annotations needed
  --> estebank_counter.rs:18:1
   |
18 | / fn foo<T: Bar<Baz = String>>() -> <T as Bar>::Baz {
19 | |     let q = Quux{};
20 | |     <Quux as Bar>::rah(&q)
21 | | }
   | |_^ cannot infer type for type parameter `T` declared on the function `assert_test_result`
   |
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

I think it is correct for us to force people to use 'static for the presented case, to reduce confusion for newcomers.

I think this is ideally Clippy's job rather than the compiler's? Certainly, it's weird to define a test with the return type signature Result<(), &'a str>, but it is legal Rust, isn't it? (You're allowed to name a lifetime that's actually &'static.)

[estebank]

I could argue that the current output for the type parameter case is better and this would be a slight regression:

error: functions used as tests must have signature fn() -> ()
  --> src/lib.rs:18:1
   |
18 | / fn foo<T: Bar<Baz = String>>() -> <T as Bar>::Baz {
19 | |     let q = Quux{};
20 | |     <Quux as Bar>::rah(&q)
21 | | }
   | |_^

Could we instead change this to check for generics.params.all(|p| is_lt(p)) instead? That way we continue giving the current error if someone writes #[test] fn foo<T>() but start allowing #[test] fn bar<'a>() -> &'a ().

[zackmdavis]

@estebank OK, how about this (force-pushed)?

[zackmdavis]

(fixed tidy)

[estebank]

CC @rust-lang/lang to let people know of what's being changed here.

#[test] fn foo<'a>() -> &'a () is now allowed with this PR, which will of course be equivalent to #[test] fn foo() -> &'static (). I believe it makes sense to merge it, but want to make sure t-lang has a chance to at least look at it before merge. Can you pick it up on your next team meeting? After that, r=me.

[Mark-Simulacrum]

The language team discussed this in the last meeting.

We felt uncertain about this particular change, in particular, because it felt like the motivation was not clearly communicated - since the lifetime must be 'static regardless, in all cases, it seems like we'd potentially confuse users but not actually gain much value. Could you say more about the motivation here? It seems like improving the diagnostic to not complain about a non-unit return type makes sense, but it's not clear that allowing more cases does.

[bors]

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

[Mark-Simulacrum]

I'm going to go ahead and close this; it's still blocked on the motivation question raised in #80934 (comment). Feel free to reopen and/or poke on Zulip if you'd like to pick this back up again.