Remove DefiningAnchor from InferCtxt #108389

oli-obk · 2023-02-23T10:03:09Z

This PR removes the InferCtxt::defining_use_anchor field and instead bubbles it through the appropriate types and functions that already need to know about their body owner (e.g. hir typeck or mir borrowck/typeck).

follow-up to #108311

r? @lcnr

rustbot · 2023-02-23T10:03:18Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information.

rustbot · 2023-02-23T10:03:22Z

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

Some changes occurred to the core trait solver

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

oli-obk · 2023-02-23T10:03:22Z

@bors try @rust-timer queue

need to crater this, too

bors · 2023-02-23T10:03:31Z

⌛ Trying commit c6f51440db29e048884073c7767841e88a8c9275 with merge f8af1895d636b112468fb6304c316e2cce25a8d6...

bors · 2023-02-23T10:16:59Z

💔 Test failed - checks-actions

lcnr

I really like this change.

I personally think it would be good to stop using DefiningAnchor::Error as the default and always require the user to explicitly provide the anchor.

E.g. by changing infcx.at(param_env, cause).define_opaque_types(anchor) to infcx.at(param_env, cause, anchor).

Same for ObligationCtxt and probably a few other places.

having looked through all changes yet, but I am really happy about what I've seen so far and the general direction of this PR.

compiler/rustc_hir_typeck/src/fn_ctxt/mod.rs

compiler/rustc_infer/src/infer/canonical/query_response.rs

oli-obk · 2023-02-23T11:11:10Z

I personally would really like to stop using DefiningAnchor::Error as the default and always require the user to explicitly provide the anchor.

yea, but doing that at the same time as this PR would've been increadibly hard to review properly

oli-obk · 2023-02-23T12:08:40Z

@bors try @rust-timer queue

bors · 2023-02-23T12:08:50Z

⌛ Trying commit 557cf7bf1aea5ccdaa663a66cc254fe6d5c5d405 with merge 9edc8987ad42f7a9d21d57465015000ae3578c17...

bors · 2023-02-23T14:58:12Z

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

rustbot · 2023-02-23T15:19:10Z

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2023-02-23T15:19:37Z

@craterbot check

craterbot · 2023-02-23T15:19:44Z

👌 Experiment pr-108389 created and queued.
🤖 Automatically detected try build 9edc8987ad42f7a9d21d57465015000ae3578c17
⚠️ Try build based on commit 557cf7bf1aea5ccdaa663a66cc254fe6d5c5d405, but latest commit is 4d36170c6d09c01e3a14d1ab2509e41966c68039. Did you forget to make a new try build?
🔍 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

…s `Error`)

It was only used in error paths or places already using `DefiningAchor::Error` already

They were already `Error` at all use sites

…assing it in every time

… method calls

rust-log-analyzer · 2023-03-02T04:59:17Z

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

Click to see the possible cause of the failure (guessed by this bot)

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:ac593985615ec2ede58e132d2e21d2b1cbd6127c)
Download action repository 'rust-lang/simpleinfra@master' (SHA:0fdabd83e1d3faaa8e9cfd7c00031e3a92997344)
Complete job name: PR (x86_64-gnu-tools, false, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: x86_64-gnu-tools
---
   Compiling rustc_data_structures v0.0.0 (/checkout/compiler/rustc_data_structures)
error[E0284]: type annotations needed
   --> compiler/rustc_data_structures/src/obligation_forest/mod.rs:352:63
    |
352 |                     None => self.obligation_tree_id_generator.next().unwrap(),
    |
    |
    = note: cannot satisfy `<_ as Iterator>::Item == ObligationTreeId`
    |
    |
352 |                     None => <Self as Iterator>::next(self.obligation_tree_id_generator).unwrap(),

For more information about this error, try `rustc --explain E0284`.
error: could not compile `rustc_data_structures` due to previous error
Build completed unsuccessfully in 0:05:06

bors · 2023-03-03T23:34:23Z

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

oli-obk · 2023-03-15T11:43:34Z

@lcnr is working on doing something similar to this PR in smaller steps.

… r=oli-obk make `define_opaque_types` fully explicit based on the idea of rust-lang#108389. Moved `define_opaque_types` into the actual operations, e.g. `eq`, instead of `infcx.at` because normalization doesn't use `define_opaque_types` and even creates it's own `At` with a different `define_opaque_types` internally. Somewhat surprisingly, coherence actually relies on `DefineOpaqueTypes::Yes` for soundness which was revealed because I've incorrectly used `DefineOpaqueTypes::No` in `equate_impl_headers`. It feels concerning that even though this is the case, we still sometimes use `DefineOpaqueTypes::No` in coherence. I did not look into this as part of this PR as it is purely changing the structure of the code without changing behavior in any way. r? `@oli-obk`

… r=oli-obk make `define_opaque_types` fully explicit based on the idea of rust-lang#108389. Moved `define_opaque_types` into the actual operations, e.g. `eq`, instead of `infcx.at` because normalization doesn't use `define_opaque_types` and even creates it's own `At` with a different `define_opaque_types` internally. Somewhat surprisingly, coherence actually relies on `DefineOpaqueTypes::Yes` for soundness which was revealed because I've incorrectly used `DefineOpaqueTypes::No` in `equate_impl_headers`. It feels concerning that even though this is the case, we still sometimes use `DefineOpaqueTypes::No` in coherence. I did not look into this as part of this PR as it is purely changing the structure of the code without changing behavior in any way. r? ``@oli-obk``

… r=oli-obk make `define_opaque_types` fully explicit based on the idea of rust-lang#108389. Moved `define_opaque_types` into the actual operations, e.g. `eq`, instead of `infcx.at` because normalization doesn't use `define_opaque_types` and even creates it's own `At` with a different `define_opaque_types` internally. Somewhat surprisingly, coherence actually relies on `DefineOpaqueTypes::Yes` for soundness which was revealed because I've incorrectly used `DefineOpaqueTypes::No` in `equate_impl_headers`. It feels concerning that even though this is the case, we still sometimes use `DefineOpaqueTypes::No` in coherence. I did not look into this as part of this PR as it is purely changing the structure of the code without changing behavior in any way. r? ```@oli-obk```

rustbot assigned lcnr Feb 23, 2023

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 Feb 23, 2023

This comment has been minimized.

Sign in to view

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

bors 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 Feb 23, 2023

This comment has been minimized.

Sign in to view

lcnr reviewed Feb 23, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_infer/src/infer/canonical/query_response.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the invert_defines branch from c6f5144 to 557cf7b Compare February 23, 2023 12:06

This comment has been minimized.

Sign in to view

craterbot removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 23, 2023

oli-obk added 21 commits March 1, 2023 11:24

Prefer a local DefiningAnchor over the InferCtxt one

379c5ac

Avoid using the old anchor in new_in_snapshot

d0f6215

Actually set the defining use anchor on ObligationCtxt

f928076

Avoid giving an inferctxt a defining use anchor

b0639e9

Stop using old_defining_use_anchor except in select_where_possible

581c1e0

Bubble old_defining_use_anchor upwards from select_all_or_error

60ac06e

Bubble defining use anchor out of fully_solve_obligations

ace9146

Bubble anchor out of fully_solve_obligation

9543b0e

Replace a use of old_defining_use_anchor with Error (it was alway…

26f46e8

…s `Error`)

Replace another old_defining_use_anchor with error

c7c4f24

It was only used in error paths or places already using `DefiningAchor::Error` already

Replace two uses of old_defining_use_anchor with error

2a39fb2

They were already `Error` at all use sites

Bubble up the defining_use_anchor from fully_perform

6ff727b

Remove another use of old_defining_use_anchor

5fcffb6

Remove the final use of old_defining_use_anchor

209103f

Remove DefiningAnchor from InferCtxt entirely

429e107

Require all ObligationCtxt to have a defining anchor at creation time

ad0bb9c

Give all trait engines direct access to DefiningAnchor instead of p…

f26c91b

…assing it in every time

Remove the now-redundant field from Inherited

a5f9af2

Require SelectionContext to have a DefiningAnchor from the start

01aacd4

Add at methods to FnCtxt and Coerce to simplify the "inherited"…

e920c5d

… method calls

Require all .at calls to provide a defining use anchor

17f29f4

oli-obk force-pushed the invert_defines branch from 4d36170 to 17f29f4 Compare March 1, 2023 17:13

oli-obk closed this Mar 15, 2023

lcnr mentioned this pull request Mar 15, 2023

make define_opaque_types fully explicit #109166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DefiningAnchor from InferCtxt #108389

Remove DefiningAnchor from InferCtxt #108389

oli-obk commented Feb 23, 2023 •

edited

Loading

rustbot commented Feb 23, 2023

rustbot commented Feb 23, 2023

oli-obk commented Feb 23, 2023

This comment has been minimized.

bors commented Feb 23, 2023

bors commented Feb 23, 2023

This comment has been minimized.

This comment has been minimized.

lcnr left a comment •

edited

Loading

oli-obk commented Feb 23, 2023

oli-obk commented Feb 23, 2023

This comment has been minimized.

bors commented Feb 23, 2023

bors commented Feb 23, 2023

This comment has been minimized.

rustbot commented Feb 23, 2023

oli-obk commented Feb 23, 2023

craterbot commented Feb 23, 2023

rust-log-analyzer commented Mar 2, 2023

bors commented Mar 3, 2023

oli-obk commented Mar 15, 2023

Remove DefiningAnchor from InferCtxt #108389

Remove DefiningAnchor from InferCtxt #108389

Conversation

oli-obk commented Feb 23, 2023 • edited Loading

rustbot commented Feb 23, 2023

rustbot commented Feb 23, 2023

oli-obk commented Feb 23, 2023

This comment has been minimized.

bors commented Feb 23, 2023

bors commented Feb 23, 2023

This comment has been minimized.

This comment has been minimized.

lcnr left a comment • edited Loading

Choose a reason for hiding this comment

oli-obk commented Feb 23, 2023

oli-obk commented Feb 23, 2023

This comment has been minimized.

bors commented Feb 23, 2023

bors commented Feb 23, 2023

This comment has been minimized.

rustbot commented Feb 23, 2023

oli-obk commented Feb 23, 2023

craterbot commented Feb 23, 2023

rust-log-analyzer commented Mar 2, 2023

bors commented Mar 3, 2023

oli-obk commented Mar 15, 2023

oli-obk commented Feb 23, 2023 •

edited

Loading

lcnr left a comment •

edited

Loading