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 hidden types at definition site #114933

Closed
wants to merge 1 commit into from

Conversation

aliemjay
Copy link
Member

@aliemjay aliemjay commented Aug 17, 2023

I believe this is the only way to fix both linked issues.
Fixes #114728
Fixes #114572

Ideally we would do WF-check after each subtyping relation, but that's cumbersome and most likely a perf hit. On the other extreme, we can do the WF-check in InferCtxt::register_hidden_type but that would break the tests in tests/ui/type-alias-impl-trait/wf-nested.rs.

This PR, as a middle ground, takes an advantage of the fact that the trait solver have limited occasions where subtyping occurs:

  • PredicateKind::{Subtype,Coerce}: irrelevant in MIR typeck.
  • PredicateKind::AliasRelate: fixed in this PR.
  • higher-ranked equality, because it requires mutual subtyping A == B => A <: B, B<: A: we can ignore them. Why?

Given all of this we can rely on the fact that the trait solver will never infer an ill-formed type if the goal input types are well-formed.

To make sure that the trait solver input types are well-formed when invoked from MIR typeck:

  • manually check every prove_predicate call in MIR typeck.
  • wf-check the input types before registering AliasRelate obligations. Done in register_type_relate_obligation

r? @compiler-errors @lcnr
cc @oli-obk

@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 labels Aug 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2023

Some changes occurred to the core trait solver

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

@aliemjay
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 17, 2023

⌛ Trying commit 83930b0 with merge b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603...

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=aliemjay
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_607ed7a3-b068-476e-8ddd-1a2ab2a9483e
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://github.com/gitapi/graphql
GITHUB_HEAD_REF=opaque-wf-check
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_607ed7a3-b068-476e-8ddd-1a2ab2a9483e
GITHUB_REF=refs/pull/114933/merge
GITHUB_REF_NAME=114933/merge
GITHUB_REF_PROTECTED=false
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=88d523480097732deca19b48135a8f60c9ef8769
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_607ed7a3-b068-476e-8ddd-1a2ab2a9483e
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_607ed7a3-b068-476e-8ddd-1a2ab2a9483e
GITHUB_TRIGGERING_ACTOR=aliemjay
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/114933/merge
GITHUB_WORKFLOW_SHA=88d523480097732deca19b48135a8f60c9ef8769
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_18_X64=/opt/hostedtoolcache/go/1.18.10/x64
---
failures:

---- [ui] tests/ui/traits/new-solver/auto-with-drop_tracking_mir.rs#pass stdout ----
normalized stderr:
error[E0271]: type mismatch resolving `[async fn body@$DIR/auto-with-drop_tracking_mir.rs:12:16: 21:2] <: impl Future<Output = ()>`
   |
LL |   async fn foo() {
   |  ________________^
LL | |
LL | |
LL | |     #[cfg(pass)]
LL | |     let x = &();
LL | |     drop(x);
LL | | }
   | |_^ types differ


error[E0271]: type mismatch resolving `[async fn body@$DIR/auto-with-drop_tracking_mir.rs:23:16: 23:18] <: impl Future<Output = ()>`
   |
LL | async fn bar() {}
   |                ^^ types differ


error: aborting due to 2 previous errors

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



The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/traits/new-solver/auto-with-drop_tracking_mir.pass/auto-with-drop_tracking_mir.pass.stderr
To only update this specific test, also pass `--test-args traits/new-solver/auto-with-drop_tracking_mir.rs`


error in revision `pass`: 1 errors occurred comparing output.
status: exit status: 1
command: RUSTC_ICE="0" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/traits/new-solver/auto-with-drop_tracking_mir.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--cfg" "pass" "--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/traits/new-solver/auto-with-drop_tracking_mir.pass" "-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/traits/new-solver/auto-with-drop_tracking_mir.pass/auxiliary" "-Ztrait-solver=next" "-Zdrop-tracking-mir" "--edition=2021"
--- stderr -------------------------------
--- stderr -------------------------------
error[E0271]: type mismatch resolving `[async fn body@/checkout/tests/ui/traits/new-solver/auto-with-drop_tracking_mir.rs:12:16: 21:2] <: impl Future<Output = ()>`
   |
LL |   async fn foo() {
   |  ________________^
   |  ________________^
LL | | //[pass]~^ ERROR type mismatch
LL | |     #[cfg(pass)]
LL | |     let x = &();
LL | |     drop(x);
LL | | }
   | |_^ types differ


error[E0271]: type mismatch resolving `[async fn body@/checkout/tests/ui/traits/new-solver/auto-with-drop_tracking_mir.rs:23:16: 23:18] <: impl Future<Output = ()>`
   |
LL | async fn bar() {}
   |                ^^ types differ

@lcnr lcnr self-assigned this Aug 17, 2023
@bors
Copy link
Contributor

bors commented Aug 17, 2023

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

@compiler-errors
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-114933 created and queued.
🤖 Automatically detected try build b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2023
@lcnr lcnr added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Aug 18, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114933 is now running

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

@aliemjay
Copy link
Member Author

@craterbot
Copy link
Collaborator

👌 Experiment pr-114933-1 created and queued.
🤖 Automatically detected try build b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603
🔍 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-114933 is completed!
📊 27 regressed and 1 fixed (349709 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 Aug 20, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114933-1 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114933-1 is completed!
📊 6 regressed and 0 fixed (27 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

@aliemjay
Copy link
Member Author

aliemjay commented Aug 23, 2023

@rust-timer build b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 23, 2023
@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.0% [0.9%, 1.1%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.3% [4.1%, 4.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 632.38s -> 635.628s (0.51%)
Artifact size: 347.07 MiB -> 347.09 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 23, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Aug 28, 2023

this regresses

All of them have updates in the last month and at least the first two seem easily fixable.

@compiler-errors compiler-errors added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Aug 30, 2023
@compiler-errors compiler-errors removed their assignment Sep 2, 2023
@wesleywiser wesleywiser added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 7, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Sep 7, 2023

This PR fixes a soundness hole, where we were failing to check that hidden types are actually well formed wrt to lifetimes. From a not-well formed hidden type you can easily satisfy trait bounds that make no sense and thus allow treating any lifetime as another lifetime that lives longer.

Considering that the implementation does nothing but add well-formedness predicates, which should always be sound and at worst a performance issue, I do not believe there is any possible risk with this PR, beyond causing breakage for benign code that just forgot some bounds somewhere. Crater showed 3 such cases, which we should all fix before landing this.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Sep 7, 2023

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

Concerns:

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 7, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Sep 7, 2023

@rfcbot concern we should open PRs against the crates that will break

@jackh726
Copy link
Member

jackh726 commented Sep 7, 2023

Can we get specific minimal examples of the crates that are breaking? Are they potential soundness issues?

@lcnr
Copy link
Contributor

lcnr commented Sep 7, 2023

@rfcbot concern why prefer this over #115008

We currently have 3 different PRs up to fix this bug, this one, #115008 and #114740, I consider the approach of this PR to be fairly fragile and would prefer something closer to #115008.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 18, 2023

Closing in favor of #115008, will explain the differences and reasons on that PR when starting the FCP

@oli-obk oli-obk closed this Sep 18, 2023
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 18, 2023
@workingjubilee workingjubilee added the F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPIT hidden types can be ill-formed Incorrect lifetime bound check in async + impl_trait_in_assoc_type