-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Refactor away the TypeckTables::cast_kinds
field
#52482
Comments
I'd like to take a look at this. I should be able to get a pr in a few days (possibly with a few questions -_-) |
@cjkenn awesome! Don't hesitate to ask here or on irc/discord if you have any questions |
The reason I wanted to detect However, I think we can do a subtype check in hair these days, so this is a non-problem. |
@oli-obk For case 1, do we intend to do something like this: let source_ty = cx.tables().expr_ty(source);
if source_ty == expr_ty {
// Convert the lexpr to a vexpr.
ExprKind::Use { source: source.to_ref() }
else {
... I'm unsure if this results in the same behaviour as the current code |
Oh right. So basically Rptr -> Rptr casts, Rptr -> Ptr casts and any cast where the types are equal. I think that are the only coercions? |
@arielb1 However, if it's a coercion cast, doesn't the coercion happen on top of the unadjusted type? I guess I'm not sure where the coercion is exactly... I'm guessing on the |
Yea, but if you have a no-op coercion (i.e. "just a subtyping conversion"), the adjusted type of the source expression can be the same as the type of the source expression, and a supertype of the type of the destination expression. |
@mcr431 I haven't worked on this at all really :( I'm unsure what to do, and if this is something we can/want to do. |
Is this work still needed? Will it conflict with Chalkification? |
@oli-obk I would like to take a shot at this :) I am a newbie to rust. |
sure! The instructions in the main issue should still apply, even if the exact locations of the code may have changed |
@oli-obk hir::ExprKind::Cast(ref source, ref cast_ty) => {
// Check for a user-given type annotation on this `cast`
let user_provided_types = cx.tables.user_provided_types();
let user_ty = user_provided_types.get(cast_ty.hir_id);
debug!(
"cast({:?}) has ty w/ hir_id {:?} and user provided ty {:?}",
expr,
cast_ty.hir_id,
user_ty,
);
let source_ty = cx.tables().expr_ty(source);
let cast = if source_ty == expr_ty {
// Convert the lexpr to a vexpr.
ExprKind::Use { source: source.to_ref() }
} |
Do you have a backtrace of that panic? I think it would be easier to review if you open a PR with your changes and we'll discuss the problems there. Basically just a single PR for the 1. |
…k_kinds, r=oli-obk fixes rust-lang#52482
…k_kinds, r=oli-obk fixes rust-lang#52482
…k_kinds, r=oli-obk fixes rust-lang#52482
Rollup of 13 pull requests Successful merges: - #58518 (Use early unwraps instead of bubbling up errors just to unwrap in the end) - #58626 (rustdoc: add option to calculate "documentation coverage") - #58629 (rust-lldb: fix crash when printing empty string) - #58660 (MaybeUninit: add read_initialized, add examples) - #58670 (fixes #52482) - #58676 (look for python2 symlinks before bootstrap python) - #58679 (Refactor passes and pass execution to be more parallel) - #58750 (Make `Unique::as_ptr`, `NonNull::dangling` and `NonNull::cast` const) - #58762 (Mention `unwind(aborts)` in diagnostics for `#[unwind]`) - #58924 (Add as_slice() to slice::IterMut and vec::Drain) - #58990 (Actually publish miri in the manifest) - #59018 (std: Delete a by-definition spuriously failing test) - #59045 (Expose new_sub_parser_from_file) Failed merges: r? @ghost
Motivation
This field has a lot of code going into creating it, while its use sites barely care at all about all the nice data it contains. They could just as well compute the data they need themselves. Computing the data is cheap, so there's no point in computing this ahead of time.
Instructions
1. Eliminate uses
The field is used in two places:
rust/src/librustc_mir/hair/cx/expr.rs
Line 590 in 50702b2
rust/src/librustc_passes/rvalue_promotion.rs
Line 371 in 50702b2
The 1. use can be fixed by just checking whether the type of
source
is the same type asexpr_ty
. This means allfoo as Bar
casts get ignored iffoo
is already of typeBar
. Which is exactly what that code is trying to do.The 2. use actually wants to know the cast kind, but there's no need to get that from the
cast_kinds
field. Instead one would again obtain the type of the expression being cast and the type being cast to, and then do exactly what MIR based borrowck does when going through the same code (UseCastTy
to figure out the details):rust/src/librustc_mir/transform/qualify_consts.rs
Lines 719 to 725 in 38168a7
2. Eliminate the field and all the code feeding into it
This part is easy now. Remove the field, and the compiler will guide you to all the places it's used. All you have to do is remove code until the compiler is happy.
The text was updated successfully, but these errors were encountered: