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 automatic overloaded deref. #12610

Merged
merged 7 commits into from
Mar 13, 2014
Merged

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 27, 2014

Enables the dereference overloads introduced by #12491 to be applied wherever automatic dereferences would be used (field accesses, method calls and indexing).

@alexcrichton
Copy link
Member

There's a few travis failures you may be interested in, and I would like to see more description added before merging.

@@ -101,7 +101,7 @@ impl<T: Clone + Float> Cmplx<T> {
/// Convert a polar representation into a complex number.
#[inline]
pub fn from_polar(r: &T, theta: &T) -> Cmplx<T> {
Cmplx::new(r * theta.cos(), r * theta.sin())
Cmplx::new(*r * theta.cos(), *r * theta.sin())
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean the &1 + 2 bug is fixed?

(If so, needstest)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've only changed x OP y with x &T instead of T where T is a generic type param with the operator overload in its trait bounds, and it wasn't to fix that bug, but prevent nasty recursion in autoderef.

I guess I should fiddle with the lookup code some more to deny more forms, as this is likely to also affect autoderef.

@eddyb
Copy link
Member Author

eddyb commented Feb 28, 2014

@brson this means I need some kind of writeup...

We currently do "autoderef" for expressions like x.foo and x.bar(), causing them to be potentially equivalent to (******x).foo and (********x).bar() (depending on the type of x).

This PR enables the deref overloads introduced by #12491 to be applied in those contexts, allowing one to write v.borrow_mut().push(x) instead of v.borrow().borrow_mut().get().push(x) that is done today for a v of type Rc<RefCell<Vec<T>>>.

@nikomatsakis
Copy link
Contributor

cc me, I will review

@nikomatsakis
Copy link
Contributor

@eddyb I tried to open a PR with my nits / refactorings / comments that I wrote as I went through, but I was not able to base it against your branch (github didn't give me that option for some reason). Here is a patch: https://gist.github.com/nikomatsakis/9414596

@nikomatsakis
Copy link
Contributor

Oh, I just thought of a wrinkle while writing out the set of tests. The current field rules autoderef if the outer type does not define the field. But what if the field is private? This seems bad. We have to figure out the right rules for this!

@nikomatsakis
Copy link
Contributor

As I wrote on IRC, we're going to need more tests. At minimum, I would like to see basically the same suite of tests that are present for explicit deref, but with autoderef instead. But autoderef opens up some new fun so it'd also be good to have tests like:

  • Test a field reference that must autoderef to find the field
  • Test indexing that must autoderef to find the array
  • Test the case where the smart pointer defines a public field that is reached first
  • Test the case where the smart pointer defines a private field that is reached first (hmm)
  • Test the case where there are multiple methods to see which gets priority

Also, there is some discussion amongst the group about what the right semantics are in the face of method resolution ambiguity (ie, if both *x and **x define a method), but I think we'll land the patch now with your "take the first thing you find" approach and maybe as part of the method lookup rewrite that I'm working worry about alternative semantics.

@bill-myers
Copy link
Contributor

This is probably already obvious, but if a private field is reached first, and there is a public field later, it would be a bug to return the private field, since that effectively makes the private field name visible in the API.

@nikomatsakis
Copy link
Contributor

Based on discussion with the group, current plan is to ignore private fields that are not visible during autoderef. But if the autoderef never finds a public field, I'd like to report an error to the effect that the field f is private rather than reporting something misleading like "no field found". (I haven't had a chance to read @eddyb's latest commits, this may well be the semantics he implemented.)

bors added a commit that referenced this pull request Mar 13, 2014
Enables the dereference overloads introduced by #12491 to be applied wherever automatic dereferences would be used (field accesses, method calls and indexing).
@bors bors closed this Mar 13, 2014
@bors bors merged commit 01a15d5 into rust-lang:master Mar 13, 2014
@eddyb eddyb deleted the deref-now-auto branch March 13, 2014 16:15
@oblitum
Copy link

oblitum commented Mar 13, 2014

is this matter for another PR or is it not desired?

use std::rc::Rc;

struct Bar { x: int }

fn foo(p: &int) {
    println!("{}", p);
}

fn baz(p: &Bar) {
    println!("{}", p.x);
}

fn main() {
    let pi = box 10;
    let pb = Rc::new(Bar {x: 3});
    foo(pi);
    // this still doesn't auto borrow:
    //baz(pb);
    // although this works:
    println!("{}", pb.x);
}

@eddyb
Copy link
Member Author

eddyb commented Mar 14, 2014

@oblitum That should be discussed. It's part of the larger auto-ref issue (calling a function taking T with &T).
What you see there is hard coded for ~T -> &T/&mut T and &mut T -> &T (AFAIK).

flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 18, 2024
…-12564, r=dswij

[`manual_unwrap_or_default`]: Check for Default trait implementation in initial condition when linting and use `IfLetOrMatch`

Fixes rust-lang#12564

changelog: Fix [`manual_unwrap_or_default`] false positive when initial `match`/`if let` condition doesn't implement `Default` but the return type does.
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.

7 participants