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.

Sign up for GitHub

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

Jump to bottom

#[test]: Point at return type if Termination bound is unsatisfied #103445

Merged
merged 1 commit into from
Nov 11, 2022
Merged

#[test]: Point at return type if Termination bound is unsatisfied #103445

merged 1 commit into from
Nov 11, 2022

Conversation

fmease
Copy link
Member

@fmease fmease commented Oct 23, 2022 •
edited
Loading

Together with #103142 (already merged) this fully fixes #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

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-libtest Area: #[test] related labels Oct 23, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2022
@fmease fmease marked this pull request as ready for review October 23, 2022 17:37
@fmease fmease changed the title test attr: point at return type if Termination bound unsatisfied #[test]: Point at return type if Termination bound is unsatisfied Oct 23, 2022
@estebank
Copy link
Contributor

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'd love to try that out before merging this, particularly to see if this could potentially cause any perf regressions (but I don't think our perf suite will touch tests, though).

I'm ok with merging this as is, but as you pointed out it doesn't feel as principled as we could be. Would you mind trying your alternatively proposed method? If you hit a wall or it doesn't pan out, we can merge this.

@fmease
Copy link
Member Author

fmease commented Oct 24, 2022

Would you mind trying your alternatively proposed method?

Will do.

I suspect though that we're gonna end up with two emitted diagnostics in the end, one from the Assert type and one from the function which I can't just get rid of. Let's see.

@fmease
Copy link
Member Author

fmease commented Oct 26, 2022

Okay, so here is the alternative approach using an Assert type. Sadly, as predicted, we emit 2 diagnostics. Not sure how to suppress the other one (even inlining assert_test_result wouldn't solve that).

@fmease
Copy link
Member Author

fmease commented Nov 3, 2022

Re

If you hit a wall or it doesn't pan out, we can merge this.

Should I try seeking something else entirely (whatever that might be) or it is okay-ish to be merged?

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2022

📌 Commit 449a440 has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2022
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 11, 2022
@Manishearth
Rollup merge of rust-lang#103445 - fmease:fix-50291, r=estebank
8048504 
`#[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 Manishearth mentioned this pull request Nov 11, 2022
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 11, 2022
@Manishearth
Rollup merge of rust-lang#103445 - fmease:fix-50291, r=estebank
1e88697 
`#[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 Manishearth mentioned this pull request Nov 11, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 11, 2022
@Dylan-DPC
Rollup merge of rust-lang#103445 - fmease:fix-50291, r=estebank
22725de 
`#[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 Dylan-DPC mentioned this pull request Nov 11, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Nov 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2022
@bors
Auto merge of rust-lang#104289 - Dylan-DPC:rollup-v7wei2t, r=Dylan-DPC
7d85104 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#100633 (Consider `#[must_use]` annotation on `async fn` as also affecting the `Future::Output`)
 - rust-lang#103445 (`#[test]`: Point at return type if `Termination` bound is unsatisfied)
 - rust-lang#103924 (Fix broken link in description of error code E0706)
 - rust-lang#104146 (Retry binding TCP Socket in remote-test-server)
 - rust-lang#104169 (Migrate `:target` rules to use CSS variables)
 - rust-lang#104202 (Fix ICE rust-lang#103748)
 - rust-lang#104216 (Don't ICE on operator trait methods with generic methods)
 - rust-lang#104217 (Display help message when fluent arg was referenced incorrectly)
 - rust-lang#104245 (Reduce default configuration's dependency upon static libstdcpp library (rust-lang#103606))

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bc9567f into rust-lang:master Nov 11, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 11, 2022
@fmease fmease deleted the fix-50291 branch November 12, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@estebank estebank

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-libtest Area: #[test] related S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.

error message when unit test type does not implement Termination is ungreat
5 participants
@fmease @estebank @bors @rust-highfive @rustbot