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

Refactor away the TypeckTables::cast_kinds field #52482

Closed
oli-obk opened this issue Jul 18, 2018 · 15 comments · Fixed by #59050
Closed

Refactor away the TypeckTables::cast_kinds field #52482

oli-obk opened this issue Jul 18, 2018 · 15 comments · Fixed by #59050
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2018

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:

  1. HIR -> MIR conversion
    if let Some(&TyCastKind::CoercionCast) = cx.tables()
  2. rvalue promotion
    match v.tables.cast_kinds().get(from.hir_id) {

The 1. use can be fixed by just checking whether the type of source is the same type as expr_ty. This means all foo as Bar casts get ignored if foo is already of type Bar. 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 (Use CastTy to figure out the details):

Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) => {
let operand_ty = operand.ty(self.mir, self.tcx);
let cast_in = CastTy::from_ty(operand_ty).expect("bad input type for cast");
let cast_out = CastTy::from_ty(cast_ty).expect("bad output type for cast");
match (cast_in, cast_out) {
(CastTy::Ptr(_), CastTy::Int(_)) |
(CastTy::FnPtr, CastTy::Int(_)) => {

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.

@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jul 18, 2018
@eddyb
Copy link
Member

eddyb commented Jul 18, 2018

cc @nikomatsakis @arielb1

@cjkenn
Copy link
Contributor

cjkenn commented Jul 19, 2018

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 -_-)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 19, 2018

@cjkenn awesome! Don't hesitate to ask here or on irc/discord if you have any questions

@arielb1
Copy link
Contributor

arielb1 commented Jul 20, 2018

@oli-obk

The reason I wanted to detect CoercionCast in typeck was because the types on both sides of the cast could be subtypes but not equal (e.g. converting a for<'a> dyn Trait<'a> to a Trait<'s>).

However, I think we can do a subtype check in hair these days, so this is a non-problem.

@cjkenn
Copy link
Contributor

cjkenn commented Jul 20, 2018

@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

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2018

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?

@eddyb
Copy link
Member

eddyb commented Jul 20, 2018

@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 source expression?

@matthew-russo
Copy link
Contributor

@cjkenn @oli-obk Is this still being actively worked on?

@arielb1
Copy link
Contributor

arielb1 commented Aug 11, 2018

@eddyb

@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 source expression?

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.

@cjkenn
Copy link
Contributor

cjkenn commented Aug 12, 2018

@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.

@ZerothLaw
Copy link

Is this work still needed? Will it conflict with Chalkification?

@saleemjaffer
Copy link
Contributor

@oli-obk I would like to take a shot at this :) I am a newbie to rust.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2019

sure! The instructions in the main issue should still apply, even if the exact locations of the code may have changed

@saleemjaffer
Copy link
Contributor

@oli-obk
For 1. I do something like this but keep gettting a thread 'rustc' panicked at 'bad input type for cast', src/libcore/option.rs:1038:5d. I am stuck at this. Any help?

        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() }
            } 

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 23, 2019

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.

saleemjaffer pushed a commit to saleemjaffer/rust that referenced this issue Feb 23, 2019
saleemjaffer pushed a commit to saleemjaffer/rust that referenced this issue Mar 5, 2019
saleemjaffer pushed a commit to saleemjaffer/rust that referenced this issue Mar 5, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 9, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 9, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 9, 2019
bors added a commit that referenced this issue Mar 9, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
7 participants