-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: add test for async-trait type mismatch #21069
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
feat: add test for async-trait type mismatch #21069
Conversation
|
| pub trait SimpleAsyncTraitModel { | ||
| fn save<'life0, 'async_trait>( | ||
| &'life0 self, | ||
| ) -> Pin<Box<dyn Future<Output = Result<SimpleAsyncTraitResult, Box<dyn core::error::Error + Send + Sync>>> + Send + 'async_trait>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't gonna work since we have no Box in our minicore. So, it won't cause mismatch even if the issue exists in the real world, as we don't emit type mismatches for unknown types. And it would be good if you could check that the same test code fails on the past version.
|
When I tested on three commits, Test file; 1.) On 2b2d9d8 (it's a commit before d1288f6) Result: 2.) On d1288f6 commit (this is the commit where the new solver trait was implemented) Result:
Result: The results are consistent showing the original issue #19957 is not actually solved yet contrasting what said in #19957 (comment) Additional test which I did on #19957 (comment) code snippet and got the similar result. |
ShoyuVanilla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks! I left a nit which fixes the CI failure
| } | ||
|
|
||
| #[test] | ||
| // #[should_panic(expected = "Unexpected type mismatches")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed, I guess? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the test will fail, this will still make the CI to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed your comment above and I only looked into the tidy test failure. Yeah, this seems not fixed yet
Edit) Well, I've tested a bit more and this seems fixed. I guess we need to adjust some lang item imports and implementations in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, adding the following impl to minicore fixes the failure 😄
impl<Ptr, U> CoerceUnsized<Pin<U>> for Pin<Ptr>
where
Ptr: CoerceUnsized<U>,
{
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShoyuVanilla done.
closes #19957 (remaining part)
Made a test to verify that issue #19957 is resolved. The test passes successfully, confirming that the async-trait type mismatch issue has been fixed by the migration to the new trait solver (commit d1288f6).
About Test
As we can't directly use
#[async_trait]in hir-ty tests (it's not available in minicore), the test simulates what the#[async_trait]macro expands to.The test verifies that no type mismatch occurs between the
Pin<Box<dyn Future>>signature and thePin<Box<impl Future>>returned by the async block.