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

Propagate container across object cast #22012

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 6, 2015

Given <expr> as Box<Trait>, infer that Box<_> is expected type for <expr>.

This is useful for addressing fallout from newly proposed box protocol; see #22006 for examples of such fallout, much of which will be unnecessary with this fix.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 6, 2015

cc @nikomatsakis

impl Foo for u32 { fn foo(&self) -> u32 { *self } }

trait Boxed { fn make() -> Self; }
impl Boxed for Box<u32> { fn make() -> Self { Box::new(7) } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a second impl that returns something which is not Box<_>? (same for Refed below)
I recall being bit before by having just one impl and getting it selected even without enough type hints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. (Its interesting, @alexcrichton brought up the same issue on the RFC itself. I wonder if this is an artifact of the old trait matching code, or the new, or both...)

@eddyb
Copy link
Member

eddyb commented Feb 9, 2015

r=me with the test additions.

@pnkfelix pnkfelix force-pushed the propagate-container-across-object-cast branch from ca73b6f to ee87aa1 Compare February 9, 2015 18:56
(with multiple impls to further exercise correct trait-matching.)
@pnkfelix pnkfelix force-pushed the propagate-container-across-object-cast branch from ee87aa1 to a1b3189 Compare February 9, 2015 18:59
@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 9, 2015

@bors: r=eddyb a1b3189 rollup

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 10, 2015
…-object-cast, r=eddyb

Given `<expr> as Box<Trait>`, infer that `Box<_>` is expected type for `<expr>`.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 10, 2015
…object-cast

Given `<expr> as Box<Trait>`, infer that `Box<_>` is expected type for `<expr>`.

This is useful for addressing fallout from newly proposed box protocol; see rust-lang#22006 for examples of such fallout, much of which will be unnecessary with this fix.
@bors bors merged commit a1b3189 into rust-lang:master Feb 10, 2015
pnkfelix added a commit to pnkfelix/rust that referenced this pull request Feb 11, 2015
…using `fresh_ty`

(hat tip to nikomatsakis who was the one who pointed out this
simplification to the logic.)
bors added a commit that referenced this pull request Feb 12, 2015
PR #22012 followup: clean up vtable::check_object_cast by reusing `fresh_ty`

(hat tip to nikomatsakis, who was the one who pointed out this simplification to the logic.)
@nrc
Copy link
Member

nrc commented Mar 22, 2015

Are we going to miss this? I have a branch which removes this facility. The root problem is that we do object conversions differently depending on if we are using coercion or casting. My branch removes the cast code path completely so x as Box<Foo> is essentially compiled as (x: Box<Foo>) as Box<Foo>. The solution is to re-implement this patch for coercions, but I'm not sure if that is possible because coercions are a little less 'directed' by the types involved.

@nrc
Copy link
Member

nrc commented Mar 22, 2015

ping @pnkfelix about that last comment

@pnkfelix
Copy link
Member Author

@nrc when you say "so x as Box is essentially compiled as (x: Box<Foo>) as Box<Foo>", you mean that is what you do on your branch?

If so, then I think that should be fine; the important thing was getting the type-hint that x has some Box<_> type, at least for my purposes.

If you look at appendix b from the most recent box/placement RFC, it shows the particular place where I needed this PR, namely the bit that says:

#[cfg(coerce_works3)] // (This one assumes PR 22012 has landed)
pub fn coerce<'a, F>(f: F) -> BoxFn<'a> where F: Fn(), F: 'a { box_!( f ) as BoxFn }

so you should be able to take that code, put it into a file, and try your branch on it, to make sure you don't regress the behavior here. (I guess I should have maybe put in some tests to ensure this did not get regressed.)

In other words, make sure that this playpen does not stop working, and I'll be happy.

@pnkfelix
Copy link
Member Author

@nrc and I discussed further; I was over-optimistic when I said:

If so, then I think that should be fine

because what I would have really liked would be to compile x as Box<Foo> into (x: Box<_>) as Box<Foo> (and not (x: Box<Foo> as Box<Foo>))

but in any case, after further discussion I decided that its not the end of the world if @nrc makes the change he desires here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants