rust icon indicating copy to clipboard operation
rust copied to clipboard

Do not try to reveal hidden types when trying to prove Freeze in the defining scope

Open oli-obk opened this issue 1 year ago • 5 comments

based on https://github.com/rust-lang/rust/pull/122077 (only the last two commits are new)

fixes #99793

this avoids the cycle error by just causing a selection error, which is not fatal

oli-obk avatar Mar 08 '24 12:03 oli-obk

r? @estebank

rustbot has assigned @estebank. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot avatar Mar 08 '24 12:03 rustbot

Some changes occurred to the core trait solver

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

rustbot avatar Mar 08 '24 12:03 rustbot

Reassigning reviewer because it allows more code on stable and needs trait system reviews

oli-obk avatar Mar 08 '24 19:03 oli-obk

happy with this change in general, unless this blocks the stabilization of ATPIT I would wait until https://github.com/rust-lang/rust/pull/122077 is merged for an in-depth review

lcnr avatar Mar 11 '24 14:03 lcnr

This PR is low priority and deserves some mir_const_qualifs cleanups and reduplications before landing anyway. The other two PRs this one is based on are needed for ATPIT

oli-obk avatar Mar 11 '24 14:03 oli-obk

I would like to make more code compile by avoiding unnecessary (and false) query cycles in Freeze bound checking during const checks. Specifically, the following code snippet should compile (it doesn't before this PR):

const fn f() -> impl Eq {
    g()
}
const fn g() {}

The reason is that when checking whether the return type implements Freeze we end up trying to reveal the opaque type. Since we're also defining the opaque type, this causes a cycle. What this PR instead does (which is what the new solver already does), is to never reveal opaque types for auto traits if we're in the defining scope. Instead, we just bail out with a non-fatal selection error. This means we do not know the answer, but if the code asking cares only about knowing for sure a trait is implemented, but is fine pessimistically assuming it is not, then we can just do that. This means that even if the hidden type is Freeze, we assume it is not, which is always a safe decision, even if it means some code won't compile even though it could.

@rfcbot merge

oli-obk avatar Apr 25 '24 14:04 oli-obk

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

  • [x] @BoxyUwU
  • [ ] @aliemjay
  • [x] @compiler-errors
  • [x] @jackh726
  • [x] @lcnr
  • [ ] @nikomatsakis
  • [x] @oli-obk
  • [x] @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot avatar Apr 25 '24 14:04 rfcbot

As a quick note: this differs from the new solver in that the new solver instead normalizes the opaque type by looking at the opaque type storage, so the new solver may instead return ambiguity or success instead of NoSolution. The important part is that the new solver new gets query cycles here, so moving to the new solver will not cause any breakage.

Theoretically the incompleteness of the old solver may guide type inference, but as there are no trait implementations with Freeze bounds this should not happen. Even if it could, it would still be merely a theoretical concern.

@rfcbot reviewed

lcnr avatar Apr 25 '24 14:04 lcnr

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot avatar May 24 '24 13:05 rfcbot

:umbrella: The latest upstream changes (presumably #125521) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar May 24 '24 23:05 bors

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

rfcbot avatar Jun 03 '24 13:06 rfcbot

@rustbot ready

oli-obk avatar Jun 03 '24 15:06 oli-obk

@bors r=lcnr

oli-obk avatar Jun 04 '24 16:06 oli-obk

:pushpin: Commit ad6e85be316b72e85ed46f21e3a32908fb2cbc1b has been approved by lcnr

It is now in the queue for this repository.

bors avatar Jun 04 '24 16:06 bors

@bors r-

lcnr avatar Jun 05 '24 08:06 lcnr

because candidate key caching is unsound? :3

that's an issue in general :o right now we're allowed to move stuff to the global cache even if it relies on the hidden type of opaques or on whether the opaque can be defined.

lcnr avatar Jun 05 '24 08:06 lcnr

Ah cool, so now that I have a reliable way to trigger that, lemme try to avoid caching

oli-obk avatar Jun 05 '24 09:06 oli-obk

please try to fix caching in a separate PR, this is very subtle '^^

or hm :shrug: this PR gives an easy way to actually test the changes, so whatever you prefer :sweat_smile:

lcnr avatar Jun 05 '24 09:06 lcnr

or hm 🤷 this PR gives an easy way to actually test the changes, so whatever you prefer 😅

I can do the fix in a separate PR, but it means we won't have a test

I'll create the separate PR based on this one for testing and review purposes. Maybe we just land this PR after the beta cutoff and then do the other PR on its own?

oli-obk avatar Jun 05 '24 09:06 oli-obk

:umbrella: The latest upstream changes (presumably #126838) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jun 22 '24 21:06 bors

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)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[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
---

- error: cannot check whether the hidden type of opaque type satisfies auto traits
-   --> $DIR/unsized_coercion3.rs:15:32
-    |
- LL |         let y: Box<dyn Send> = x;
-    |
-    |
-    = note: fetching the hidden types of an opaque inside of the defining scope is not supported. You can try moving the opaque type and the item that actually registers a hidden type into a new submodule
- note: opaque type is declared here
-    |
-    |
- LL | fn hello() -> Box<impl Trait + ?Sized> {
-    |                   ^^^^^^^^^^^^^^^^^^^
-    = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Send>`
15 error[E0277]: the size for values of type `impl Trait + ?Sized` cannot be known at compilation time
16   --> $DIR/unsized_coercion3.rs:15:32
17    |


21    = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
22    = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Send>`
- error: aborting due to 2 previous errors
+ error: aborting due to 1 previous error
25 
26 For more information about this error, try `rustc --explain E0277`.
26 For more information about this error, try `rustc --explain E0277`.
27 


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion3.old/unsized_coercion3.old.stderr
To only update this specific test, also pass `--test-args impl-trait/unsized_coercion3.rs`


error in revision `old`: 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/unsized_coercion3.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,next,old)" "--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/unsized_coercion3.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/impl-trait/unsized_coercion3.old/auxiliary"
--- stderr -------------------------------
error[E0277]: the size for values of type `impl Trait + ?Sized` cannot be known at compilation time
##[error]  --> /checkout/tests/ui/impl-trait/unsized_coercion3.rs:15:32
   |
   |
LL |         let y: Box<dyn Send> = x;
   |
   = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
   = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
   = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Send>`
error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.
------------------------------------------
---
11 
- error: cannot check whether the hidden type of opaque type satisfies auto traits
-   --> $DIR/unsized_coercion5.rs:16:32
-    |
- LL |         let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
-    |
-    |
-    = note: fetching the hidden types of an opaque inside of the defining scope is not supported. You can try moving the opaque type and the item that actually registers a hidden type into a new submodule
- note: opaque type is declared here
-    |
-    |
- LL | fn hello() -> Box<impl Trait + ?Sized> {
-    |                   ^^^^^^^^^^^^^^^^^^^
-    = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`
26 error[E0277]: the size for values of type `impl Trait + ?Sized` cannot be known at compilation time
27   --> $DIR/unsized_coercion5.rs:16:32
28    |


32    = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
33    = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`
- error: aborting due to 3 previous errors
+ error: aborting due to 2 previous errors
36 
37 Some errors have detailed explanations: E0277, E0308.
37 Some errors have detailed explanations: E0277, E0308.
38 For more information about an error, try `rustc --explain E0277`.


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion5.old/unsized_coercion5.old.stderr
To only update this specific test, also pass `--test-args impl-trait/unsized_coercion5.rs`


error in revision `old`: 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/unsized_coercion5.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,next,old)" "--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/unsized_coercion5.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/impl-trait/unsized_coercion5.old/auxiliary"
--- stderr -------------------------------
error[E0308]: mismatched types
##[error]  --> /checkout/tests/ui/impl-trait/unsized_coercion5.rs:16:32
   |
   |
LL |         let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
   |                -------------   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected trait `Send`, found trait `Trait + Send`
   |                expected due to this
   |
   = note: expected struct `Box<dyn Send>`
              found struct `Box<dyn Trait + Send>`
              found struct `Box<dyn Trait + Send>`

error[E0277]: the size for values of type `impl Trait + ?Sized` cannot be known at compilation time
##[error]  --> /checkout/tests/ui/impl-trait/unsized_coercion5.rs:16:32
   |
LL |         let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
   |
   = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
   = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
   = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
For more information about an error, try `rustc --explain E0277`.
------------------------------------------


---- [ui] tests/ui/type-alias-impl-trait/in-where-clause.rs stdout ----
diff of stderr:

26    = 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
28 error[E0283]: type annotations needed: cannot satisfy `Bar: Send`
-   --> $DIR/in-where-clause.rs:10:10
+   --> $DIR/in-where-clause.rs:13:9
30    |
30    |
- LL |     Bar: Send,
-    |          ^^^^
+ LL |     [0; 1 + 2]
33    |
34    = note: cannot satisfy `Bar: Send`
+ note: required by a bound in `foo`
+   --> $DIR/in-where-clause.rs:10:10
---
To only update this specific test, also pass `--test-args type-alias-impl-trait/in-where-clause.rs`

error: 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/type-alias-impl-trait/in-where-clause.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/type-alias-impl-trait/in-where-clause" "-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/type-alias-impl-trait/in-where-clause/auxiliary"
--- stderr -------------------------------
--- stderr -------------------------------
error[E0391]: cycle detected when computing type of `Bar::{opaque#0}`
   |
LL | type Bar = impl Sized;
   |            ^^^^^^^^^^
   |
   |
note: ...which requires computing type of opaque `Bar::{opaque#0}`...
   |
LL | type Bar = impl Sized;
   |            ^^^^^^^^^^
   |            ^^^^^^^^^^
note: ...which requires type-checking `foo`...
   |
LL | / fn foo() -> Bar
LL | | where
LL | |     Bar: Send,
LL | |     Bar: Send,
   | |______________^
   = note: ...which requires revealing opaque types in `[Binder { value: TraitPredicate(<Bar as core::marker::Send>, polarity:Positive), bound_vars: [] }]`...
   = note: ...which again requires computing type of `Bar::{opaque#0}`, completing the cycle
note: cycle used when checking that `Bar::{opaque#0}` is well-formed
   |
LL | type Bar = impl Sized;
   |            ^^^^^^^^^^
   |            ^^^^^^^^^^
   = 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
error[E0283]: type annotations needed: cannot satisfy `Bar: Send`
##[error]  --> /checkout/tests/ui/type-alias-impl-trait/in-where-clause.rs:13:9
   |
   |
LL |     [0; 1 + 2]
   |
   = note: cannot satisfy `Bar: Send`
note: required by a bound in `foo`
  --> /checkout/tests/ui/type-alias-impl-trait/in-where-clause.rs:10:10
---

---- [ui] tests/ui/type-alias-impl-trait/reveal_local.rs stdout ----
diff of stderr:

16 LL | fn is_send<T: Send>() {}
18 
18 
- error: cannot check whether the hidden type of `reveal_local[9507]::Foo::{opaque#0}` satisfies auto traits
+ error[E0283]: type annotations needed: cannot satisfy `Foo: Send`
20   --> $DIR/reveal_local.rs:22:15
22 LL |     is_send::<Foo>();

23    |               ^^^
24    |
24    |
-    = note: fetching the hidden types of an opaque inside of the defining scope is not supported. You can try moving the opaque type and the item that actually registers a hidden type into a new submodule
- note: opaque type is declared here
-   --> $DIR/reveal_local.rs:5:12
- LL | type Foo = impl Debug;
-    |            ^^^^^^^^^^
+    = note: cannot satisfy `Foo: Send`
31 note: required by a bound in `is_send`
---
To only update this specific test, also pass `--test-args type-alias-impl-trait/reveal_local.rs`

error: 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/type-alias-impl-trait/reveal_local.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/type-alias-impl-trait/reveal_local" "-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/type-alias-impl-trait/reveal_local/auxiliary"
--- stderr -------------------------------
--- stderr -------------------------------
error: cannot check whether the hidden type of `reveal_local[9507]::Foo::{opaque#0}` satisfies auto traits
   |
LL |     is_send::<Foo>();
   |               ^^^
   |
   |
   = note: fetching the hidden types of an opaque inside of the defining scope is not supported. You can try moving the opaque type and the item that actually registers a hidden type into a new submodule
note: opaque type is declared here
   |
LL | type Foo = impl Debug;
   |            ^^^^^^^^^^
note: required by a bound in `is_send`
note: required by a bound in `is_send`
  --> /checkout/tests/ui/type-alias-impl-trait/reveal_local.rs:7:15
   |
LL | fn is_send<T: Send>() {}

error[E0283]: type annotations needed: cannot satisfy `Foo: Send`
##[error]  --> /checkout/tests/ui/type-alias-impl-trait/reveal_local.rs:22:15
   |
   |
LL |     is_send::<Foo>();
   |               ^^^
   |
   = note: cannot satisfy `Foo: Send`
note: required by a bound in `is_send`
  --> /checkout/tests/ui/type-alias-impl-trait/reveal_local.rs:7:15
   |
LL | fn is_send<T: Send>() {}

error: aborting due to 2 previous errors

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

rust-log-analyzer avatar Jul 15 '24 10:07 rust-log-analyzer

:umbrella: The latest upstream changes (presumably #126024) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jul 24 '24 15:07 bors

@bors r=lcnr

oli-obk avatar Jul 24 '24 16:07 oli-obk

:pushpin: Commit 8ea461da5595067433803555b3fdb00d75e95b83 has been approved by lcnr

It is now in the queue for this repository.

bors avatar Jul 24 '24 16:07 bors