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

Implement deref coercions. #21351

Merged
merged 4 commits into from
Jan 30, 2015
Merged

Implement deref coercions. #21351

merged 4 commits into from
Jan 30, 2015

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jan 18, 2015

Coercions will now attempt to autoderef as needed before reborrowing.
This includes overloaded Deref, e.g. &Rc<T> coerces to &T, and
DerefMut, e.g. &mut Vec<T> coerces to &mut [T] (in addition to &[T]).
Closes #21432.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@jdm
Copy link
Contributor

jdm commented Jan 18, 2015

💃

@pnkfelix
Copy link
Member

@nikomatsakis I'm not sure I'm the best person to review this. Maybe @nick29581 ?

@nrc nrc assigned nrc and unassigned pnkfelix Jan 19, 2015
@nrc
Copy link
Member

nrc commented Jan 19, 2015

Do you know what change made all the coercion cruft in commit three obsolete? Do we have tests to ensure nothing has regressed here?

@nrc
Copy link
Member

nrc commented Jan 19, 2015

LGTM. I'd like to see a few more tests - in particular some more compile fail tests to check we don't allow anything we shouldn't. The rpass tests look OK, I think we should probably have some more to check more extreme edge cases - coercions equivalent to &**********foo, that kind of thing.

And of course we need to wait for the RFC to land

@eddyb
Copy link
Member Author

eddyb commented Jan 19, 2015

The removed trait object coercions were dead code, the cases are handled by coerce_unsized.
As for the other simplifications, they're also DST fallout that never got properly unified with the common paths.

@aturon
Copy link
Member

aturon commented Jan 20, 2015

FYI: The RFC has been merged.


use std::rc::Rc;

// Examples from the "deref coercions" RFC, at rust-lang/rfcs#241.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see some negative tests for borrow checker failures.

For example, passing the same &mut Box<T> value for two distinct &mut T arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, passing to one &T and one &mut T argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

And one like this:

fn copy<T>(x: &mut T) -> &mut T { x }

fn foo<T>(x: &mut Box<T>) {
    let y = copy(x);
    let z = copy(x); // ERROR (I hope)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

And one like this:

fn copy<T>(x: &T) -> &T { x }

fn foo(x: &mut Box<int>) {
    let y = copy(x); // OK
    let z = copy(x); // OK
    **x += 1; // ERROR, borrowed
}

@nikomatsakis
Copy link
Contributor

@bors r+ 46696c84fd2d2459186252657bcf1e76428c5043

@nikomatsakis
Copy link
Contributor

The coercion logic around unsafe pointers in particular looks strange to me, but that seems to be a pre-existing problem. e.g. this line here https://github.com/eddyb/rust/blob/67718004499d1e446eef4def35aeafc8bf4c0858/src/librustc_typeck/check/coercion.rs#L235 is strange, since it seems to only permit coercions from &T to *Unsized and not *T to *Unsized? There is also a question of transitivity. (cc @nick29581, this relates to our discussion from the other day)

@bors
Copy link
Contributor

bors commented Jan 29, 2015

⌛ Testing commit 46696c8 with merge 40c00b4...

@bors
Copy link
Contributor

bors commented Jan 29, 2015

💔 Test failed - auto-mac-32-opt

@eddyb
Copy link
Member Author

eddyb commented Jan 29, 2015

@bors retry q_q

@bors
Copy link
Contributor

bors commented Jan 29, 2015

☔ Merge conflict

@eddyb
Copy link
Member Author

eddyb commented Jan 29, 2015

@bors r=nikomatsakis ae076e1

@bors
Copy link
Contributor

bors commented Jan 30, 2015

⌛ Testing commit ae076e1 with merge 398ac8f...

@bors
Copy link
Contributor

bors commented Jan 30, 2015

💔 Test failed - auto-win-32-nopt-t

@eddyb
Copy link
Member Author

eddyb commented Jan 30, 2015

@bors retry

@bors
Copy link
Contributor

bors commented Jan 30, 2015

⌛ Testing commit ae076e1 with merge e0f5980...

bors added a commit that referenced this pull request Jan 30, 2015
Coercions will now attempt to autoderef as needed before reborrowing.
This includes overloaded `Deref`, e.g. `&Rc<T>` coerces to `&T`, and
`DerefMut`, e.g. `&mut Vec<T>` coerces to `&mut [T]` (in addition to `&[T]`).
Closes #21432.
@bors bors merged commit ae076e1 into rust-lang:master Jan 30, 2015
@eddyb eddyb deleted the x-coerce--a-new-hope branch January 30, 2015 11:16
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.

Tracking issue for deref coercions RFC
8 participants