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

WIP: eliminate DefineOpaqueTypes by using Yes across the compiler #127034

Closed
wants to merge 6 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 27, 2024

Opening this PR because

  1. I'm stuck
  2. 48cb487 is quite a major change (but unblocks the follow-up commits, which otherwise set the hidden type of the opaque type as anything from the param env that has matching bounds, so a T: Trait will match an -> impl Trait whenver select_where_possible tries to solve some available bounds. This all happens before actually matching the opaque type to the returned value, at which point everything would work correctly, but obviously have issues with ordering). The next solver does not have this issue because it just converts opaque types to inference vars. Tho how it handles the send bounds in that case I do not know, will investigate.

cc @lcnr @compiler-errors I'll open individual issues for things so we can keep separate things separate instead of talking about everything in this one big PR that I'll move out of WIP status when I can add a commit to that removes DefineOpaqueTypes.

Related:

@rustbot

This comment was marked as resolved.

@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. labels Jun 27, 2024
.eq(DefineOpaqueTypes::No, placeholder_trait_predicate, candidate)
.eq(DefineOpaqueTypes::Yes, placeholder_trait_predicate, candidate)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's discuss these in #127035

Comment on lines -2710 to +2678
.eq(DefineOpaqueTypes::No, predicate.trait_ref, trait_ref)
.eq(DefineOpaqueTypes::Yes, predicate.trait_ref, trait_ref)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in selection (behind a probe) and confirmation. Without the previous commit skipping assemble_candidates_from_caller_bounds for opaque types in their defining scope, this will cause functions like

fn filter_fold<T, Acc>(
mut predicate: impl FnMut(&T) -> bool,
mut fold: impl FnMut(Acc, T) -> Acc,
) -> impl FnMut(Acc, T) -> Acc {
move |acc, item| if predicate(&item) { fold(acc, item) } else { acc }
}

to immediately match the opaque type against the APIT with the same bounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've looked into it a bit. Do you know of any fundamental issues with moving the old solver to a similar normalization scheme for opaques as the new solver uses? Or is that just a bad version of lazy norm?

Comment on lines 17 to 22
pub fn recursive_fn<E>() -> impl Parser<E> {
//[current]~^ ERROR: cycle
move || recursive_fn().parse()
//[next]~^ ERROR: type annotations needed
//~^ ERROR: type annotations needed
//[current]~| ERROR: no method named `parse`
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we need to fix this issue for the new solver and the current one before we can land this commit

@bors
Copy link
Contributor

bors commented Jul 8, 2024

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

@oli-obk oli-obk force-pushed the define_opaque_types16 branch from 2b1f439 to 513b139 Compare July 25, 2024 10:57
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:3aacb9c90579defe09351ac5e8ee504359f8054da6326ff19038f1b7c90e3cb2aafe33685c6d9b76ee8d2ccbd187ca80c46ab5380485abdd8c0ce7d69cd8d8fd:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
---- [ui] tests/ui/auto-traits/opaque_type_candidate_selection.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/auto-traits/opaque_type_candidate_selection.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/auto-traits/opaque_type_candidate_selection" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/auto-traits/opaque_type_candidate_selection/auxiliary"
--- stderr -------------------------------
error[E0284]: type annotations needed
##[error]  --> /checkout/tests/ui/auto-traits/opaque_type_candidate_selection.rs:10:20
   |
   |
LL |     pub fn cast<T>(x: Container<Alias<T>, T>) -> Container<T, T> {
   |                    ^ cannot infer type
   |
   = note: cannot satisfy `<Alias<T> as Trait<T>>::Assoc == _`
note: required because it appears within the type `Container<Alias<T>, T>`
   |
   |
LL | struct Container<T: Trait<U>, U> {
   = help: unsized fn params are gated as an unstable feature
help: function arguments must have a statically known size, borrowed types always have a known size
   |
   |
LL |     pub fn cast<T>(x: &Container<Alias<T>, T>) -> Container<T, T> {

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0284`.
For more information about this error, try `rustc --explain E0284`.
------------------------------------------


---- [ui] tests/ui/coherence/occurs-check/opaques.rs#old stdout ----

error in revision `old`: test compilation failed although it shouldn't!
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/coherence/occurs-check/opaques.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--cfg" "old" "--check-cfg" "cfg(FALSE,old,next)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/coherence/occurs-check/opaques.old" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/coherence/occurs-check/opaques.old/auxiliary"
--- stderr -------------------------------
error[E0284]: type annotations needed
##[error]  --> /checkout/tests/ui/coherence/occurs-check/opaques.rs:13:20
   |
   |
LL |     pub fn cast<T>(x: Container<Alias<T>, T>) -> Container<T, T> {
   |                    ^ cannot infer type
   |
   = note: cannot satisfy `<Alias<T> as Trait<T>>::Assoc == _`
note: required because it appears within the type `Container<Alias<T>, T>`
   |
   |
LL | struct Container<T: Trait<U>, U> {
   = help: unsized fn params are gated as an unstable feature
help: function arguments must have a statically known size, borrowed types always have a known size
   |
   |
LL |     pub fn cast<T>(x: &Container<Alias<T>, T>) -> Container<T, T> {

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0284`.
---

16 LL + use Parser;
17    |
18 
- error[E0391]: cycle detected when computing type of opaque `recursive_fn::{opaque#0}`
-    |
-    |
- LL | pub fn recursive_fn<E>() -> impl Parser<E> {
-    |
-    |
- note: ...which requires type-checking `recursive_fn`...
-    |
-    |
- LL |     move || recursive_fn().parse()
-    |             ^^^^^^^^^^^^^^
-    = note: ...which requires evaluating trait selection obligation `recursive_fn::{opaque#0}: core::marker::Unpin`...
-    = note: ...which again requires computing type of opaque `recursive_fn::{opaque#0}`, completing the cycle
- note: cycle used when computing type of `recursive_fn::{opaque#0}`
-    |
-    |
- LL | pub fn recursive_fn<E>() -> impl Parser<E> {
-    |                             ^^^^^^^^^^^^^^
-    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information
38 
- error: aborting due to 3 previous errors
- 
- Some errors have detailed explanations: E0282, E0391, E0599.
---
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/recursive-bound-eval.current/recursive-bound-eval.current.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args impl-trait/recursive-bound-eval.rs`

error in revision `current`: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/impl-trait/recursive-bound-eval.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--cfg" "current" "--check-cfg" "cfg(FALSE,next,current)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/recursive-bound-eval.current" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/recursive-bound-eval.current/auxiliary"
--- stderr -------------------------------
error[E0282]: type annotations needed
##[error]  --> /checkout/tests/ui/impl-trait/recursive-bound-eval.rs:19:28
   |
   |
LL |     move || recursive_fn().parse()

error[E0599]: no method named `parse` found for opaque type `impl Parser<_>` in the current scope
##[error]  --> /checkout/tests/ui/impl-trait/recursive-bound-eval.rs:19:28
   |
   |
LL |     move || recursive_fn().parse()
   |                            ^^^^^ method not found in `impl Parser<_>`
   = help: items from traits can only be used if the trait is implemented and in scope
help: trait `Parser` which provides `parse` is implemented but not in scope; perhaps you want to import it
   |
LL + use Parser;

@bors
Copy link
Contributor

bors commented Jul 29, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants