rustc: remove ty::MethodOrigin and replace its uses with equivalent logic. #26694

eddyb · 2015-06-30T23:10:57Z

MethodCallee now has no information about the method, other than its DefId.
The previous bits of information can be recovered as follows:

let method_item = tcx.impl_or_trait_item(callee.def_id);
let container = method_item.container();

The method is inherent if container is a ty::ImplContainer:

the impl the method comes from is container.id()

The method is a trait method if container is a ty::TraitContainer:

the trait the method is part of is container.id()
a ty::TraitRef can be constructed by putting together:
- container.id() as the trait ID
- callee.substs.clone().method_to_trait() as the trait substs (including Self)
the above trait_ref is a valid T: Trait<A, B, C> predicate
selecting trait_ref could result in one of the following:
- traits::VtableImpl(data): static dispatch to data.impl_def_id
- traits::VtableObject(data): dynamic dispatch, with the vtable index:
  traits::get_vtable_index_of_object_method(tcx, data, callee.def_id)
- other variants of traits::Vtable: various other impl sources

rust-highfive · 2015-06-30T23:11:08Z

r? @Aatch

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

eddyb · 2015-07-01T00:55:34Z

Your "lints" are only unconditional_recursion, and you can get the impl relatively easily (by selecting on the trait - you can copy trans::common::fulfill_obligation).

@arielb1 Right, by "lints" I meant librustc_lint.
I didn't want to jump the gun because it wasn't an obvious cost (all changes that I can think of in this PR have a constant amortized cost difference associated with them).
It's probably not as bad as vtable_index, which can be reused for multiple monomorphizations (but trans could still cache vtable layouts in a more general fashion).

arielb1 · 2015-07-01T09:44:19Z

unconditional_recursion is only called once per method call - this is amortized O(1).

arielb1 · 2015-07-01T10:51:03Z

r+ modulo not storing the vtable + passing tests.

bors · 2015-07-02T04:44:47Z

☔ The latest upstream changes (presumably #26677) made this pull request unmergeable. Please resolve the merge conflicts.

…ation.

eddyb · 2015-07-04T03:28:59Z

I ended up making all the changes anyway. Original PR description was:

MethodOrigin now only holds the optional impl_def_id, for trait methods, and vtable_index, for object methods.

They could be both removed, at the cost of offloading computation to lints and trans, respectively.
In the case of impl_def_id, using traits::select in lints will provide a more accurate result.
As for vtable_index, it's possible we don't want middle or typeck to deal with vtable layout - or at least, it could be stored outside of MethodOrigin.

MethodOrigin::Inherent could be distinguished from the other cases by checking that the container of the method is an impl.
MethodOrigin::Object can be replaced by an optimization for VtableObject in trans::meth - so that object.method() and Trait::method(object) produce the same IR (currently the latter always goes through the <Trait as Trait>::method shim).

I haven't attempted to implement any of those potential refactors as they might not be entirely beneficial, though they should all be possible (with all of them, MethodOrigin would be gone).

eddyb · 2015-07-04T10:43:11Z

cc @huonw The commit named "rustc_lint: use traits::select for methods in unconditional_recursion." produces warnings for overloaded operators (and probably other things I haven't thought of):

struct Foo;
impl std::ops::Deref for Foo {
    type Target = Foo;
    // warning: function cannot return without recurring
    fn deref(&self) -> &Foo { &**self }
}
impl std::ops::Index<usize> for Foo {
    type Output = Foo;
    // warning: function cannot return without recurring
    fn index(&self, x: usize) -> &Foo { &self[x] }
}

We might want some tests for these.

arielb1 · 2015-07-04T13:18:57Z

@bors r+

bors · 2015-07-04T13:18:58Z

📌 Commit d877cfd has been approved by arielb1

bors · 2015-07-04T13:36:48Z

⌛ Testing commit d877cfd with merge 411a5bf...

bors · 2015-07-04T14:44:45Z

💔 Test failed - auto-linux-64-nopt-t

…rust-lang#26339.

…ent methods.

eddyb · 2015-07-04T14:53:06Z

@bors r=arielb1

bors · 2015-07-04T14:53:07Z

📌 Commit d256eb1 has been approved by arielb1

bluss · 2015-07-04T15:11:28Z

Since this fixes a memory safety bug, can we backport it?

eddyb · 2015-07-04T15:16:18Z

@bluss the refactoring parts are pretty huge, though maybe just the vtable upcasting logic could be... I'm not sure.

bors · 2015-07-04T16:17:43Z

⌛ Testing commit d256eb1 with merge a993275...

bors · 2015-07-04T17:16:50Z

💔 Test failed - auto-mac-64-opt

eddyb · 2015-07-04T17:25:59Z

@bors retry

bors · 2015-07-04T18:49:29Z

⌛ Testing commit d256eb1 with merge 42e545f...

`MethodCallee` now has no information about the method, other than its `DefId`. The previous bits of information can be recovered as follows: ```rust let method_item = tcx.impl_or_trait_item(callee.def_id); let container = method_item.container(); ``` The method is inherent if `container` is a `ty::ImplContainer`: * the `impl` the method comes from is `container.id()` The method is a trait method if `container` is a `ty::TraitContainer: * the `trait` the method is part of is `container.id()` * a `ty::TraitRef` can be constructed by putting together: * `container.id()` as the `trait` ID * `callee.substs.clone().method_to_trait()` as the `trait` substs (including `Self`) * the above `trait_ref` is a valid `T: Trait<A, B, C>` predicate * selecting `trait_ref` could result in one of the following: * `traits::VtableImpl(data)`: static dispatch to `data.impl_def_id` * `traits::VtableObject(data)`: dynamic dispatch, with the vtable index: `traits::get_vtable_index_of_object_method(tcx, data, callee.def_id)` * other variants of `traits::Vtable`: various other `impl` sources

bors · 2015-07-04T20:21:38Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

@huonw

After #26694, the overloaded operator and "impl not known at method lookup time" cases started triggering the lint. I've also added checks for overloaded autoderef and method calls via paths (i.e. `T::method()`). All new 8 test cases did not trigger the lint before #26694. r? @huonw

rust-highfive assigned Aatch Jun 30, 2015

eddyb force-pushed the method-nan branch from 6e25077 to 383cc7e Compare July 3, 2015 02:23

eddyb added 6 commits July 4, 2015 06:21

rustc: remove unused MethodStaticClosure variant of MethodOrigin.

fe354e5

rustc: simplify ty::MethodOrigin and avoid trait item indices.

70365ed

rustc_trans: explicit impl population is unnecessary in fulfill_oblig…

a2fe59a

…ation.

rustc_lint: use traits::select for methods in unconditional_recursion.

5620a58

rustc_trans: remove some outdated and unused logic from callee.

a5447e1

rustc: remove MethodOrigin::Object and use traits::VtableObject instead.

96d24a5

eddyb force-pushed the method-nan branch from 383cc7e to 699ada6 Compare July 4, 2015 03:26

eddyb changed the title ~~rustc: do not use indices inside a trait's items to refer to methods.~~ rustc: remove ty::MethodOrigin and replace its uses with equivalent logic. Jul 4, 2015

eddyb force-pushed the method-nan branch from 699ada6 to d877cfd Compare July 4, 2015 04:07

eddyb added 2 commits July 4, 2015 17:51

rustc: compute the vtable base of a supertrait during selection. Fixes …

536e71b

…rust-lang#26339.

rustc: remove MethodOrigin and use the container to distinguish inher…

d256eb1

…ent methods.

eddyb force-pushed the method-nan branch from d877cfd to d256eb1 Compare July 4, 2015 14:52

bors merged commit d256eb1 into rust-lang:master Jul 4, 2015

This was referenced Jul 4, 2015

lower blanket unsafe block to actual cases of unsafe and adjust indents #26757

Merged

Rewrite the improper_ctypes lint. #26583

Merged

eddyb deleted the method-nan branch July 4, 2015 20:34

eddyb mentioned this pull request Jul 5, 2015

rustc_lint: handle more method calls in unconditional_recursion. #26783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: remove ty::MethodOrigin and replace its uses with equivalent logic. #26694

rustc: remove ty::MethodOrigin and replace its uses with equivalent logic. #26694

eddyb commented Jun 30, 2015

rust-highfive commented Jun 30, 2015

eddyb commented Jul 1, 2015

arielb1 commented Jul 1, 2015

arielb1 commented Jul 1, 2015

bors commented Jul 2, 2015

eddyb commented Jul 4, 2015

eddyb commented Jul 4, 2015

arielb1 commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

eddyb commented Jul 4, 2015

bors commented Jul 4, 2015

bluss commented Jul 4, 2015

eddyb commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

eddyb commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

rustc: remove ty::MethodOrigin and replace its uses with equivalent logic. #26694

rustc: remove ty::MethodOrigin and replace its uses with equivalent logic. #26694

Conversation

eddyb commented Jun 30, 2015

rust-highfive commented Jun 30, 2015

eddyb commented Jul 1, 2015

arielb1 commented Jul 1, 2015

arielb1 commented Jul 1, 2015

bors commented Jul 2, 2015

eddyb commented Jul 4, 2015

eddyb commented Jul 4, 2015

arielb1 commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

eddyb commented Jul 4, 2015

bors commented Jul 4, 2015

bluss commented Jul 4, 2015

eddyb commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015

eddyb commented Jul 4, 2015

bors commented Jul 4, 2015

bors commented Jul 4, 2015