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

wf-check generators #97183

Merged
merged 1 commit into from
Jul 19, 2022
Merged

wf-check generators #97183

merged 1 commit into from
Jul 19, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 19, 2022

fixes #90409

We should not rely on generators being well formed by construction now that they can get used via type alias impl trait (and thus users can choose generic arguments that are invalid). This can cause surprising behaviour if (definitely unsound) transmutes are used, and it's generally saner to just check for well formedness.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 19, 2022
@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

@oli-obk what's broken and how?

@oli-obk
Copy link
Contributor Author

oli-obk commented May 19, 2022

This PR checks predicates on generators at all use sites, so also if the generator is used in a different function than where it was defined (via impl trait). This breaks two async tests (see PR diff), even though they should be fine afaict. It looks like the async fn generated TAIT does not know about the predicates of the function, but it may be even subtler, not sure. If nothing is obvious to you, i'll dig in, just wanted to check whether this rang a bell for you

@cjgillot
Copy link
Contributor

I have no idea what happens. From the test outputs, this looks more like an issue of missing predicates in the param_env, doesn't it? Lowering for async closures is just an expression, with nothing smart on the generic parameters.

@cjgillot
Copy link
Contributor

It looks like the async fn generated TAIT does not know about the predicates of the function

explicit_predicates_of ignores on purpose the predicates of the function. This logic is copied from RPIT, but in that case is wrong, and should use the common case of inheriting predicates.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 2, 2022

Blocked on #97607

@oli-obk oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jun 2, 2022
@bors
Copy link
Contributor

bors commented Jun 8, 2022

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 14, 2022

#97607 has been resolved by stabilizing nll

r? rust-lang/compiler

@oli-obk oli-obk changed the title WIP: wf-check generators wf-check generators Jun 14, 2022
@bors
Copy link
Contributor

bors commented Jun 20, 2022

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

@oli-obk oli-obk removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jun 22, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 22, 2022

@rustbot review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2022
@cjgillot
Copy link
Contributor

r? rust-lang/types

@rust-highfive rust-highfive assigned jackh726 and unassigned cjgillot Jun 26, 2022
@jackh726
Copy link
Member

crater run, maybe?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 27, 2022

@bors try

@bors
Copy link
Contributor

bors commented Jun 27, 2022

⌛ Trying commit b394703ec62dd42d725e83381460e9281fdeb539 with merge 0f68263c719c986445acfdd2b05d69e200c95710...

@bors
Copy link
Contributor

bors commented Jun 27, 2022

☀️ Try build successful - checks-actions
Build commit: 0f68263c719c986445acfdd2b05d69e200c95710 (0f68263c719c986445acfdd2b05d69e200c95710)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 27, 2022

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-97183 created and queued.
🤖 Automatically detected try build 0f68263c719c986445acfdd2b05d69e200c95710
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-97183 is completed!
📊 24 regressed and 5 fixed (238437 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 29, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 29, 2022

The only real regression is monoio and that uses the type_alias_impl_trait feature gate, which may mean this is actually a soundness bug in monoio that has now surfaced. Arguably hard to abuse soundness bug, but still...

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The monoio "regression" should be added as a test.

Comment on lines +1 to +17
error[E0277]: the trait bound `B: Bar` is not satisfied
--> $DIR/future.rs:15:5
|
LL | async move { bar.bar() }
| ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Bar` is not implemented for `B`
|
note: required by a bound in `foo`
--> $DIR/future.rs:14:11
|
LL | fn foo<B: Bar>(bar: B) -> FooFuture<B> {
| ^^^ required by this bound in `foo`
help: consider restricting type parameter `B`
|
LL | type FooFuture<B: Bar> = impl Future<Output = ()>;
| +++++

error: aborting due to previous error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error seems incorrect, given that there is a B: Bar bound on foo and we throw away bounds on type aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not throw away bounds on type alias impl trait though. Performing the suggested change will actually work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's interesting!

src/test/ui/lifetimes/issue-76168-hr-outlives-2.rs Outdated Show resolved Hide resolved
Comment on lines +1 to +9
error[E0277]: the size for values of type `A` cannot be known at compilation time
--> $DIR/issue-88287.rs:35:9
|
LL | type SearchFutureTy<'f, A, B: 'f>
| - this type parameter needs to be `std::marker::Sized`
...
LL | async move { todo!() }
| ^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if it's just me, but I'm having a difficult time actually figuring out what the problem here is...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, but that's not a generator issue, closures would have the same issue.

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 10, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 11, 2022

The monoio "regression" should be added as a test.

it adds nothing new that isn't already covered by the existing tests

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 14, 2022

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 14, 2022
@bors
Copy link
Contributor

bors commented Jul 18, 2022

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

@jackh726
Copy link
Member

I'm torn on if this needs types team FCP or not. I lean towards no, since this is imo a bug fix. But also can see a case for it.

I can't imagine any reason why we wouldn't want to do this, so r=me after rebase

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 18, 2022

I'm torn on if this needs types team FCP or not.

since it only affects unstable code, I think not

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Jul 18, 2022

📌 Commit f403260 has been approved by jackh726

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 19, 2022
wf-check generators

fixes rust-lang#90409

We should not rely on generators being well formed by construction now that they can get used via type alias impl trait (and thus users can choose generic arguments that are invalid). This can cause surprising behaviour if (definitely unsound) transmutes are used, and it's generally saner to just check for well formedness.
This was referenced Jul 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#97183 (wf-check generators)
 - rust-lang#98320 (Mention first and last macro in backtrace)
 - rust-lang#99335 (Use split_once in FromStr docs)
 - rust-lang#99347 (Use `LocalDefId` in `OpaqueTypeKey`)
 - rust-lang#99392 (Fix debuginfo tests.)
 - rust-lang#99404 (Use span_bug for unexpected field projection type)
 - rust-lang#99410 (Update invalid atomic ordering lint)
 - rust-lang#99434 (Fix `Skip::next` for non-fused inner iterators)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 881e1c1 into rust-lang:master Jul 19, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 19, 2022
@oli-obk oli-obk deleted the tait_ice_async branch November 9, 2022 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

ICE with missing bound in TAIT with closures/async
7 participants