wf-check generators #97183

oli-obk · 2022-05-19T15:52:45Z

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.

cjgillot · 2022-05-19T17:23:22Z

@oli-obk what's broken and how?

oli-obk · 2022-05-19T18:04:33Z

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 · 2022-05-22T08:28:07Z

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 · 2022-05-22T08:33:26Z

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 · 2022-06-02T09:23:04Z

Blocked on #97607

bors · 2022-06-08T08:42:46Z

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

oli-obk · 2022-06-14T12:17:54Z

#97607 has been resolved by stabilizing nll

r? rust-lang/compiler

bors · 2022-06-20T13:57:48Z

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

compiler/rustc_trait_selection/src/traits/wf.rs

oli-obk · 2022-06-22T10:36:08Z

@rustbot review

cjgillot · 2022-06-26T20:26:59Z

r? rust-lang/types

jackh726 · 2022-06-26T23:31:17Z

crater run, maybe?

oli-obk · 2022-06-27T06:47:51Z

@bors try

bors · 2022-06-27T06:47:59Z

⌛ Trying commit b394703ec62dd42d725e83381460e9281fdeb539 with merge 0f68263c719c986445acfdd2b05d69e200c95710...

bors · 2022-06-27T08:21:38Z

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

oli-obk · 2022-06-27T09:39:58Z

@craterbot check

craterbot · 2022-06-27T09:40:03Z

👌 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 · 2022-06-29T07:07:30Z

🎉 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

oli-obk · 2022-06-29T07:44:55Z

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...

jackh726

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

jackh726 · 2022-07-10T18:57:26Z

src/test/ui/type-alias-impl-trait/future.stderr

+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


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

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

Oh that's interesting!

src/test/ui/lifetimes/issue-76168-hr-outlives-2.rs

jackh726 · 2022-07-10T19:07:57Z

src/test/ui/generic-associated-types/issue-88287.stderr

+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
+   |


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

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

compiler/rustc_trait_selection/src/traits/wf.rs

oli-obk · 2022-07-11T10:52:30Z

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

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

oli-obk · 2022-07-14T16:11:39Z

@rustbot review

bors · 2022-07-18T06:12:43Z

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

jackh726 · 2022-07-18T17:14:50Z

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

oli-obk · 2022-07-18T17:27:20Z

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 · 2022-07-18T17:27:21Z

📌 Commit f403260 has been approved by jackh726

It is now in the queue for this repository.

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.

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

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

This comment has been minimized.

Sign in to view

oli-obk force-pushed the tait_ice_async branch from bb0fe1a to c857c7b Compare June 1, 2022 14:35

oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 2, 2022

oli-obk force-pushed the tait_ice_async branch from c857c7b to 85bc912 Compare June 14, 2022 12:17

rust-highfive assigned cjgillot Jun 14, 2022

oli-obk changed the title ~~WIP: wf-check generators~~ wf-check generators Jun 14, 2022

oli-obk force-pushed the tait_ice_async branch from 85bc912 to b394703 Compare June 22, 2022 10:34

oli-obk removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 22, 2022

oli-obk commented Jun 22, 2022

View reviewed changes

compiler/rustc_trait_selection/src/traits/wf.rs Show resolved Hide resolved

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

rust-highfive assigned jackh726 and unassigned cjgillot Jun 26, 2022

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

jackh726 reviewed Jul 10, 2022

View reviewed changes

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 force-pushed the tait_ice_async branch from b394703 to aab1483 Compare July 11, 2022 10:54

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

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

Check generators for well formedness

f403260

oli-obk force-pushed the tait_ice_async branch from aab1483 to f403260 Compare July 18, 2022 17:26

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

This was referenced Jul 19, 2022

Rollup of 8 pull requests #99450

Closed

Rollup of 8 pull requests #99451

Merged

bors merged commit 881e1c1 into rust-lang:master Jul 19, 2022

rustbot added this to the 1.64.0 milestone Jul 19, 2022

aliemjay mentioned this pull request Aug 10, 2022

closures/async blocks capture all lifetimes in scope #100372

Open

oli-obk deleted the tait_ice_async branch November 9, 2022 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wf-check generators #97183

wf-check generators #97183

oli-obk commented May 19, 2022 •

edited

Loading

This comment has been minimized.

cjgillot commented May 19, 2022

oli-obk commented May 19, 2022

cjgillot commented May 22, 2022

cjgillot commented May 22, 2022

oli-obk commented Jun 2, 2022

bors commented Jun 8, 2022

oli-obk commented Jun 14, 2022

bors commented Jun 20, 2022

oli-obk commented Jun 22, 2022

cjgillot commented Jun 26, 2022

jackh726 commented Jun 26, 2022

oli-obk commented Jun 27, 2022

bors commented Jun 27, 2022

bors commented Jun 27, 2022

oli-obk commented Jun 27, 2022

craterbot commented Jun 27, 2022

craterbot commented Jun 29, 2022

oli-obk commented Jun 29, 2022

jackh726 left a comment

jackh726 Jul 10, 2022

oli-obk Jul 11, 2022

jackh726 Jul 18, 2022

jackh726 Jul 10, 2022

oli-obk Jul 12, 2022

oli-obk commented Jul 11, 2022

oli-obk commented Jul 14, 2022

bors commented Jul 18, 2022

jackh726 commented Jul 18, 2022

oli-obk commented Jul 18, 2022

bors commented Jul 18, 2022

wf-check generators #97183

wf-check generators #97183

Conversation

oli-obk commented May 19, 2022 • edited Loading

This comment has been minimized.

cjgillot commented May 19, 2022

oli-obk commented May 19, 2022

cjgillot commented May 22, 2022

cjgillot commented May 22, 2022

oli-obk commented Jun 2, 2022

bors commented Jun 8, 2022

oli-obk commented Jun 14, 2022

bors commented Jun 20, 2022

oli-obk commented Jun 22, 2022

cjgillot commented Jun 26, 2022

jackh726 commented Jun 26, 2022

oli-obk commented Jun 27, 2022

bors commented Jun 27, 2022

bors commented Jun 27, 2022

oli-obk commented Jun 27, 2022

craterbot commented Jun 27, 2022

craterbot commented Jun 29, 2022

oli-obk commented Jun 29, 2022

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Jul 10, 2022

Choose a reason for hiding this comment

oli-obk Jul 11, 2022

Choose a reason for hiding this comment

jackh726 Jul 18, 2022

Choose a reason for hiding this comment

jackh726 Jul 10, 2022

Choose a reason for hiding this comment

oli-obk Jul 12, 2022

Choose a reason for hiding this comment

oli-obk commented Jul 11, 2022

oli-obk commented Jul 14, 2022

bors commented Jul 18, 2022

jackh726 commented Jul 18, 2022

oli-obk commented Jul 18, 2022

bors commented Jul 18, 2022

oli-obk commented May 19, 2022 •

edited

Loading