rustc_target: TyAndLayout::field should never error.
#88337
Conversation
|
Some changes occured to rustc_codegen_cranelift cc @bjorn3 Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
|
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @nagisa |
eddyb
force-pushed
the
field-failure-is-not-an-option
branch
from
b6d4f6c to
cfabbc6
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit cfabbc654c3b77e927f3eadaebd95497e433c066 with merge 04674fabacf07ea0451047603a002a2d7a389cf2... |
|
☀️ Try build successful - checks-actions |
|
Queued 04674fabacf07ea0451047603a002a2d7a389cf2 with parent dfd6306, future comparison URL. |
|
Finished benchmarking try commit (04674fabacf07ea0451047603a002a2d7a389cf2): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
A 1.25% regression for coercions-debug incr-patched: add static arr item, but otherwise all changes are easily within noise. |
That regression is pretty surprising in many ways. It doesn't match any patterns in other benchmarks and the query view shows things like |
There was a problem hiding this comment.
Hm, would there be value in making sure the naming between this and the ty_and_layout_FOR_variant is consistent?
There was a problem hiding this comment.
The prefix ty_and_layout_ is consistent between the two, IIRC. If you mean TyAndLayout::for_variant should be renamed, I guess as_variant or variant_downcast might be okay, but I don't have a strong preference.
There was a problem hiding this comment.
Yeah I mean ty_and_layout_for_variant+ty_and_layout_for_field or just ty_and_layout_variant+ty_and_layout_field.
|
r=me up to you to decide if you want to do anything about the inline comments. |
eddyb
force-pushed
the
field-failure-is-not-an-option
branch
from
cfabbc6 to
78778fc
Compare
|
@bors r=nagisa I resolved the first comment, but my reasoning for not changing the |
|
📌 Commit 78778fc has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@9556d7a. Direct link to PR: <rust-lang/rust#88337> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
Provide `layout_of` automatically (given tcx + param_env + error handling). After rust-lang#88337, there's no longer any uses of `LayoutOf` within `rustc_target` itself, so I realized I could move the trait to `rustc_middle::ty::layout` and redesign it a bit. This is similar to rust-lang#88338 (and supersedes it), but at no ergonomic loss, since there's no funky `C: LayoutOf<Ty = Ty>` -> `Ty: TyAbiInterface<C>` generic `impl` chain, and each `LayoutOf` still corresponds to one `impl` (of `LayoutOfHelpers`) for the specific context. After this PR, this is what's needed to get `trait LayoutOf` (with the `layout_of` method) implemented on some context type: * `TyCtxt`, via `HasTyCtxt` * `ParamEnv`, via `HasParamEnv` * a way to transform `LayoutError`s into the desired error type * an error type of `!` can be paired with having `cx.layout_of(...)` return `TyAndLayout` *without* `Result<...>` around it, such as used by codegen * this is done through a new `LayoutOfHelpers` trait (and so is specifying the type of `cx.layout_of(...)`) When going through this path (and not bypassing it with a manual `impl` of `LayoutOf`), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform. (**EDIT**: just noticed that because of the supertrait relationship, you cannot actually implement `LayoutOf` yourself, the blanket `impl` fully covers all possible context types that could ever implement it) Part of the motivation for this shape of API is that I've been working on querifying `FnAbi::of_*`, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the `FnAbi` methods in `rustc_target`, since they're much more tied to `rustc` concepts. r? `@nagisa` cc `@oli-obk` `@bjorn3`
Provide `layout_of` automatically (given tcx + param_env + error handling). After rust-lang#88337, there's no longer any uses of `LayoutOf` within `rustc_target` itself, so I realized I could move the trait to `rustc_middle::ty::layout` and redesign it a bit. This is similar to rust-lang#88338 (and supersedes it), but at no ergonomic loss, since there's no funky `C: LayoutOf<Ty = Ty>` -> `Ty: TyAbiInterface<C>` generic `impl` chain, and each `LayoutOf` still corresponds to one `impl` (of `LayoutOfHelpers`) for the specific context. After this PR, this is what's needed to get `trait LayoutOf` (with the `layout_of` method) implemented on some context type: * `TyCtxt`, via `HasTyCtxt` * `ParamEnv`, via `HasParamEnv` * a way to transform `LayoutError`s into the desired error type * an error type of `!` can be paired with having `cx.layout_of(...)` return `TyAndLayout` *without* `Result<...>` around it, such as used by codegen * this is done through a new `LayoutOfHelpers` trait (and so is specifying the type of `cx.layout_of(...)`) When going through this path (and not bypassing it with a manual `impl` of `LayoutOf`), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform. (**EDIT**: just noticed that because of the supertrait relationship, you cannot actually implement `LayoutOf` yourself, the blanket `impl` fully covers all possible context types that could ever implement it) Part of the motivation for this shape of API is that I've been working on querifying `FnAbi::of_*`, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the `FnAbi` methods in `rustc_target`, since they're much more tied to `rustc` concepts. r? `@nagisa` cc `@oli-obk` `@bjorn3`
Provide `layout_of` automatically (given tcx + param_env + error handling). After rust-lang#88337, there's no longer any uses of `LayoutOf` within `rustc_target` itself, so I realized I could move the trait to `rustc_middle::ty::layout` and redesign it a bit. This is similar to rust-lang#88338 (and supersedes it), but at no ergonomic loss, since there's no funky `C: LayoutOf<Ty = Ty>` -> `Ty: TyAbiInterface<C>` generic `impl` chain, and each `LayoutOf` still corresponds to one `impl` (of `LayoutOfHelpers`) for the specific context. After this PR, this is what's needed to get `trait LayoutOf` (with the `layout_of` method) implemented on some context type: * `TyCtxt`, via `HasTyCtxt` * `ParamEnv`, via `HasParamEnv` * a way to transform `LayoutError`s into the desired error type * an error type of `!` can be paired with having `cx.layout_of(...)` return `TyAndLayout` *without* `Result<...>` around it, such as used by codegen * this is done through a new `LayoutOfHelpers` trait (and so is specifying the type of `cx.layout_of(...)`) When going through this path (and not bypassing it with a manual `impl` of `LayoutOf`), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform. (**EDIT**: just noticed that because of the supertrait relationship, you cannot actually implement `LayoutOf` yourself, the blanket `impl` fully covers all possible context types that could ever implement it) Part of the motivation for this shape of API is that I've been working on querifying `FnAbi::of_*`, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the `FnAbi` methods in `rustc_target`, since they're much more tied to `rustc` concepts. r? `@nagisa` cc `@oli-obk` `@bjorn3`
8 participants
This refactor (making
TyAndLayout::fieldreturnTyAndLayoutwithout anyResultaround it) is based on a simple observation, regardingTyAndLayout::field:If
cx.layout_of(ty)succeeds (for somecxandty), then.field(cx, i)on the resultingTyAndLayoutshould always succeed in computingcx.layout_of(field_ty)(wherefield_tyis the type of theith field ofty).The reason for this is that no matter which field is chosen,
cx.layout_of(field_ty)will have already been computed, as part of computingcx.layout_of(ty), as we cannot determine the layout of any type without considering the layouts of all of its fields.And so it should be fine to turn any errors into ICEs, since they likely indicate a
cxmismatch, or some other edge case that is due to a compiler bug (as opposed to ever being an user-facing error).Each commit should probably be reviewed separately, though note that there's some
whereclauses (inrustc_target::abi::call::*) that change in most commits.cc @nagisa @oli-obk