borrowck typeck children together with their root #138499
Conversation
rustbot
added
A-tidy
|
@bors try |
borrowck typeck children together with their root This is a theoretical breaking change, see the `non-structural-match-types-cycle-err.rs` test. Opening for a crater run. r? compiler-errors
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
all regressions look either spurious or due to the removal of |
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
f319a98 to
35f93c5
Compare
This comment has been minimized.
This comment has been minimized.
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
35f93c5 to
519ed02
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #138719) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment was marked as outdated.
This comment was marked as outdated.
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
519ed02 to
48a3c5e
Compare
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
borrowck typeck children together with their root This introduces new cycle errors, even with `feature(inline_const_pat)` removed, see the `non-structural-match-types-cycle-err.rs` test. The new cycle error happens as the layout of `async`-blocks relies on their `optimized_mir`. As that now depends on `mir_borrowck` of its typeck parent, computing the layout of an `async`-block during MIR building, e.g. when evaluating a named `const` pattern. I think there's currently no way to have a named const pattern whose type references an async block while being allowed? cc `@oli-obk` `@RalfJung` I cannot think of other cases where we currently rely on the MIR of a typeck children while borrowchecking their parent. The crater run came back without any breakage. My work here will prevent any future features which rely on this as we'll get locked into borrowchecking them together as I continue to work on rust-lang/types-team#129, cc `@rust-lang/types.` r? compiler-errors
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
48a3c5e to
1bcbc5c
Compare
we already collect opaque types from nested items during `mir_borrowck` of the root, checking that they are consistent this way.
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
1bcbc5c to
e60daca
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
|
⌛ Trying commit e60daca with merge 60ec9868717c8bc856a18fb96d637c2f5286a1a6... |
borrowck typeck children together with their root This introduces new cycle errors, even with `feature(inline_const_pat)` removed, see the `non-structural-match-types-cycle-err.rs` test. The new cycle error happens as the layout of `async`-blocks relies on their `optimized_mir`. As that now depends on `mir_borrowck` of its typeck parent, computing the layout of an `async`-block during MIR building, e.g. when evaluating a named `const` pattern. I think there's currently no way to have a named const pattern whose type references an async block while being allowed? cc `@oli-obk` `@RalfJung` I cannot think of other cases where we currently rely on the MIR of a typeck children while borrowchecking their parent. The crater run came back without any breakage. My work here will prevent any future features which rely on this as we'll get locked into borrowchecking them together as I continue to work on rust-lang/types-team#129, cc `@rust-lang/types.` r? compiler-errors
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (60ec986): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary 1.1%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.539s -> 775.513s (-0.26%) |
rustbot
removed
the
S-waiting-on-perf
So is this outdated information now? The state of this PR is quite confusing.
Sorry, I don't understand this question. What do you mean? |
yeah 😅 originally contained part of this PR in #138785 but ended up cleanly(kinda) separating afterwards
The following test went from a "constant of non-structural type" error to a cycle error: struct AnyOption<T>(T);
impl<T> AnyOption<T> {
const NONE: Option<T> = None;
}
fn uwu() {}
fn defines() {
match Some(async {}) {
AnyOption::<_>::NONE => {}
//[nightly]~^ ERROR constant of non-structural type
//[thisPR]~^ ERROR cycle detected
_ => {}
}
}
fn main() {}The change is that computing the layout of async blocks inside of their parent function now cycles. From my knowledge the only case where we actually do it is when evaluating constants in match patterns. As the type of the async block is unnameable, it has to be part of the constants type signature. Does having an async block/generator in your type always result in structural match errors or are there ways to use a constant whose type references an async block without such an error? I think it doesn't matter too much, as I don't expect people to write this code (as can be seem by the clean crater run), but it would be the difference between a purely internal refactoring and a theoretical breaking change. |
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
e60daca to
f94b96a
Compare
I assume async blocks / generators never implement We require the type of the const to implement So, I think the answer to your question is "no". But I am also rather clueless about everything related to async blocks / generators / opaque types so I am not 100% confident about this. In the future we might want to let people implement |
lcnr
force-pushed
the
borrowck-typeck_root
branch
from
f94b96a to
95de83f
Compare
…r-errors,oli-obk add `TypingMode::Borrowck` Shares the first commit with rust-lang#138499, doesn't really matter which PR to land first 😊 😁 Introduces `TypingMode::Borrowck` which unlike `TypingMode::Analysis`, uses the hidden type computed by HIR typeck as the initial value of opaques instead of an unconstrained infer var. This is a part of rust-lang/types-team#129. Using this new `TypingMode` is unfortunately a breaking change for now, see tests/ui/impl-trait/non-defining-uses/as-projection-term.rs. Using an inference variable as the initial value results in non-defining uses in the defining scope. We therefore only enable it if with `-Znext-solver=globally` or `-Ztyping-mode-borrowck` To do that the PR contains the following changes: - `TypeckResults::concrete_opaque_type` are already mapped to the definition of the opaque type - writeback now checks that the non-lifetime parameters of the opaque are universal - for this, `fn check_opaque_type_parameter_valid` is moved from `rustc_borrowck` to `rustc_trait_selection` - we add a new `query type_of_opaque_hir_typeck` which, using the same visitors as MIR typeck, attempts to merge the hidden types from HIR typeck from all defining scopes - done by adding a `DefiningScopeKind` flag to toggle between using borrowck and HIR typeck - the visitors stop checking that the MIR type matches the HIR type. This is trivial as the HIR type are now used as the initial hidden types of the opaque. This check is useful as a safeguard when not using `TypingMode::Borrowck`, but adding it to the new structure is annoying and it's not soundness critical, so I intend to not add it back. - add a `TypingMode::Borrowck` which behaves just like `TypingMode::Analysis` except when normalizing opaque types - it uses `type_of_opaque_hir_typeck(opaque)` as the initial value after replacing its regions with new inference vars - it uses structural lookup in the new solver fixes rust-lang#112201, fixes rust-lang#132335, fixes rust-lang#137751 r? `@compiler-errors` `@oli-obk`
|
☔ The latest upstream changes (presumably #138785) made this pull request unmergeable. Please resolve the merge conflicts. |
This introduces new cycle errors, even with
feature(inline_const_pat)removed, see thenon-structural-match-types-cycle-err.rstest.The new cycle error happens as the layout of
async-blocks relies on theiroptimized_mir. As that now depends onmir_borrowckof its typeck parent, computing the layout of anasync-block during MIR building, e.g. when evaluating a namedconstpattern. I think there's currently no way to have a named const pattern whose type references an async block while being allowed? cc @oli-obk @RalfJungI cannot think of other cases where we currently rely on the MIR of a typeck children while borrowchecking their parent. The crater run came back without any breakage. My work here will prevent any future features which rely on this as we'll get locked into borrowchecking them together as I continue to work on rust-lang/types-team#129, cc @rust-lang/types.
r? compiler-errors