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

Implement support for GeneratorWitnessMIR in new solver #109755

Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 30, 2023

r? @cjgillot

I mostly want this to cut down the number of failing UI tests when running the UI test suite with --compare-mode=next-solver, but there doesn't seem like much reason to block implementing this since it adds minimal complexity to the existing structural traits impl in the new solver.

If others are against adding this for some reason, then maybe we should just make GeneratorWitnessMIR return NoSolution for these traits. Anything but an ICE please 😸 🧊

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 30, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 30, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@lcnr
Copy link
Contributor

lcnr commented Mar 30, 2023

think this is fine, still not completely sure about the current approach wrt to GeneratorWitnessMIR but this feels simple enough to not cause any issue.

@cjgillot
Copy link
Contributor

When I implemented it in the current solver, the subtlety was the handling of lifetimes, since generator_hidden_types only has erased lifetimes.
Do those functions need to account for lifetimes somehow, or are erased lifetimes ok?

@compiler-errors
Copy link
Member Author

Oh, I guess we could instantiate fresh placeholder lifetimes for all the types in here when we encounter ReErased...

I didn't check but the solver probably handles erased lifetimes (I think...), but the region constraints returned by the solver are probably not sound or correct -- so if we had some auto trait bound that holds with region constraints, they're probably not enforced.

@lcnr
Copy link
Contributor

lcnr commented Mar 31, 2023

Oh, I guess we could instantiate fresh placeholder lifetimes for all the types in here when we encounter ReErased...

I didn't check but the solver probably handles erased lifetimes (I think...), but the region constraints returned by the solver are probably not sound or correct -- so if we had some auto trait bound that holds with region constraints, they're probably not enforced.

We currently ICE when encountering ReErased in a query response. If these lifetimes are existentials instantiating them with infer vars would work? 🤔

@compiler-errors
Copy link
Member Author

I thought they would be universal vars, since they're acting more like for-all over the generator interior's choice of lifetimes, but idk

@lcnr
Copy link
Contributor

lcnr commented Mar 31, 2023

🤷 universals is also fine using placeholders. But it feels weird to have universal regions be represented using ReErased instead of using a Binder?

@cjgillot
Copy link
Contributor

cjgillot commented Apr 3, 2023

In the current solver, they are modeled as universals. The idea is: if x: MyType<'erased> appears in MIR, 'a could be the lifetime of a local (so arbitrary small) or of a global (arbitrary large). See bind_generator_hidden_types_above.

If we want lifetime knowledge in generators, we'd need to compute those hidden types in borrowck instead of a separate query. I don't have such plans yet.

@compiler-errors compiler-errors force-pushed the new-solver-generator-witness-mir branch from 0b40906 to 4385165 Compare April 5, 2023 02:58
@compiler-errors compiler-errors force-pushed the new-solver-generator-witness-mir branch from 4385165 to 4a4fc3b Compare April 5, 2023 03:05
@cjgillot
Copy link
Contributor

cjgillot commented Apr 5, 2023

Great!
@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2023

📌 Commit 4a4fc3b has been approved by cjgillot

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 Apr 5, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 6, 2023
…or-witness-mir, r=cjgillot

Implement support for `GeneratorWitnessMIR` in new solver

r? `@cjgillot`

I mostly want this to cut down the number of failing UI tests when running the UI test suite with `--compare-mode=next-solver`, but there doesn't seem like much reason to block implementing this since it adds minimal complexity to the existing structural traits impl in the new solver.

If others are against adding this for some reason, then maybe we should just make `GeneratorWitnessMIR` return `NoSolution` for these traits. Anything but an ICE please 😸 🧊
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 6, 2023
…or-witness-mir, r=cjgillot

Implement support for `GeneratorWitnessMIR` in new solver

r? ``@cjgillot``

I mostly want this to cut down the number of failing UI tests when running the UI test suite with `--compare-mode=next-solver`, but there doesn't seem like much reason to block implementing this since it adds minimal complexity to the existing structural traits impl in the new solver.

If others are against adding this for some reason, then maybe we should just make `GeneratorWitnessMIR` return `NoSolution` for these traits. Anything but an ICE please 😸 🧊
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#109395 (Fix issue when there are multiple candidates for edit_distance_with_substrings)
 - rust-lang#109755 (Implement support for `GeneratorWitnessMIR` in new solver)
 - rust-lang#109782 (Don't leave a comma at the start of argument list when removing arguments)
 - rust-lang#109977 (rustdoc: avoid including line numbers in Google SERP snippets)
 - rust-lang#109980 (Derive String's PartialEq implementation)
 - rust-lang#109984 (Remove f32 & f64 from MemDecoder/MemEncoder)
 - rust-lang#110004 (add `dont_check_failure_status` option in the compiler test)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c86c933 into rust-lang:master Apr 6, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 6, 2023
@compiler-errors compiler-errors deleted the new-solver-generator-witness-mir branch August 11, 2023 20:09
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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants