[3/n] rustc: unify and simplify managing associated items. #37402

eddyb · 2016-10-25T15:46:27Z

This is part of a series (prev | next) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. MIR-based early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments.

ImplOrTraitItem/impl_or_trait_item have been renamed to AssociatedItem/associated_item.

The common fields from (what used to be) ty::ImplOrTraitItem's variants have been pulled out, leaving only an AssociatedKind C-like enum to distinguish between methods, constants and types.

The type information has been removed from AssociatedItem, and as such the latter can now be computed on-demand from the local HIR map, i.e. an extern-crate-enabled TraitItem | ImplItem.
It may be moved to HIR in the future, if we intend to start using HIR types cross-crate.

ty::ExplicitSelfCategory has been moved to rustc_typeck and is produced on-demand from the signature of the method, and a method_has_self_argument field on AssociatedItem, which is used to indicate that the first argument is a sugary "method receiver" and as such, method call syntax can be used.

rust-highfive · 2016-10-25T15:46:40Z

r? @nikomatsakis

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

bors · 2016-10-26T14:02:05Z

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

bors · 2016-10-28T01:20:09Z

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

[2/n] rustc_metadata: move is_extern_item to trans. *This is part of a series ([prev](#37400) | [next](#37402)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments.* <hr> Minor cleanup missed by #36551: `is_extern_item` is one of, if not the only `CrateStore` method who takes a `TyCtxt` but doesn't produce something cached in it, and such methods are going away.

bors · 2016-10-30T07:01:54Z

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

nikomatsakis

r=me, just had a minor question. Seems good!

nikomatsakis · 2016-10-31T14:51:52Z

src/librustc/middle/stability.rs

@@ -685,15 +682,8 @@ fn is_internal<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, span: Span) -> bool {
 }

 fn is_staged_api<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, id: DefId) -> bool {
-    match tcx.trait_item_of_item(id) {
-        Some(trait_method_id) if trait_method_id != id => {


why does this case not matter anymore?

Took me a bit to remember, but it never should've really mattered. Regardless of past practices, we now never refer to associated items in impls of traits, only to the definition in the trait, outside of monomorphization.

eddyb · 2016-11-02T01:43:38Z

@bors r=nikomatsakis

bors · 2016-11-02T01:43:39Z

📌 Commit c8578db has been approved by nikomatsakis

bors · 2016-11-03T13:11:58Z

⌛ Testing commit c8578db with merge 84df530...

bors · 2016-11-03T13:15:39Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

alexcrichton · 2016-11-03T15:50:54Z

@bors: retry

[3/n] rustc: unify and simplify managing associated items. _This is part of a series ([prev](rust-lang#37401) | [next](rust-lang#37404)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments._ <hr> `ImplOrTraitItem`/`impl_or_trait_item` have been renamed to `AssociatedItem`/`associated_item`. The common fields from (what used to be) `ty::ImplOrTraitItem`'s variants have been pulled out, leaving only an `AssociatedKind` C-like enum to distinguish between methods, constants and types. The type information has been removed from `AssociatedItem`, and as such the latter can now be computed on-demand from the local HIR map, i.e. an extern-crate-enabled `TraitItem | ImplItem`. It may be moved to HIR in the future, if we intend to start using HIR types cross-crate. `ty::ExplicitSelfCategory` has been moved to `rustc_typeck` and is produced on-demand from the signature of the method, and a `method_has_self_argument` field on `AssociatedItem`, which is used to indicate that the first argument is a sugary "method receiver" and as such, method call syntax can be used.

bors · 2016-11-04T17:40:58Z

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

bors · 2016-11-06T09:52:41Z

🔒 Merge conflict

[4/n] rustc: harden against InferOk having obligations in more cases. _This is part of a series ([prev](#37402) | [next](#37408)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments._ <hr> This adds more asserts that `InferOk` results have no obligations, pending completion of #32730. Each of these could accidentally drop obligations on the floor if they start getting produced by unification, and a future change does just that, in order to produce a "shallow success" (hopefully leading to ambiguities during trait selection), _without_ the possibility of an eventual success - mostly guarded by ICEs for now.

bors · 2016-11-07T14:10:37Z

📌 Commit 3f0f094 has been approved by nikomatsakis

bors · 2016-11-07T16:39:16Z

💔 Test failed - auto-linux-cross-opt

alexcrichton · 2016-11-07T22:50:24Z

@bors: retry

On Mon, Nov 7, 2016 at 8:39 AM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-cross-opt
https://buildbot.rust-lang.org/builders/auto-linux-cross-opt/builds/4129

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#37402 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95KF0KjzbHaiGMRLdnKXw2RX0YA_Vks5q71Q2gaJpZM4KgJvu
.

bors · 2016-11-07T23:14:37Z

⌛ Testing commit 3f0f094 with merge af1240f...

bors · 2016-11-07T23:16:35Z

💔 Test failed - auto-linux-cross-opt

eddyb · 2016-11-08T05:58:14Z

@bors retry

bors · 2016-11-08T16:02:39Z

⌛ Testing commit 3f0f094 with merge 873de1d...

bors · 2016-11-08T17:05:00Z

💔 Test failed - auto-win-msvc-64-opt-rustbuild

eddyb · 2016-11-08T17:07:31Z

How can this fail on buildbot but not on travis? Is it because of something merged in between?

alexcrichton · 2016-11-08T18:03:19Z

@bors: retry

rustbuild: Fix dependencies of check-error-index #37651

bors · 2016-11-09T23:13:46Z

🔒 Merge conflict

bors · 2016-11-09T23:14:40Z

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

eddyb · 2016-11-10T01:45:58Z

@bors r=nikomatsakis

bors · 2016-11-10T01:46:00Z

📌 Commit de0ffad has been approved by nikomatsakis

[3/n] rustc: unify and simplify managing associated items. _This is part of a series ([prev](rust-lang#37401) | [next](rust-lang#37404)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments._ <hr> `ImplOrTraitItem`/`impl_or_trait_item` have been renamed to `AssociatedItem`/`associated_item`. The common fields from (what used to be) `ty::ImplOrTraitItem`'s variants have been pulled out, leaving only an `AssociatedKind` C-like enum to distinguish between methods, constants and types. The type information has been removed from `AssociatedItem`, and as such the latter can now be computed on-demand from the local HIR map, i.e. an extern-crate-enabled `TraitItem | ImplItem`. It may be moved to HIR in the future, if we intend to start using HIR types cross-crate. `ty::ExplicitSelfCategory` has been moved to `rustc_typeck` and is produced on-demand from the signature of the method, and a `method_has_self_argument` field on `AssociatedItem`, which is used to indicate that the first argument is a sugary "method receiver" and as such, method call syntax can be used.

Rollup of 5 pull requests - Successful merges: #37402, #37412, #37661, #37664, #37667 - Failed merges:

@flodiebold

Separate impl items from the parent impl This change separates impl item bodies out of the impl itself. This gives incremental more resolution. In so doing, it refactors how the visitors work, and cleans up a bit of the collect/check logic (mostly by moving things out of collect that didn't really belong there, because they were just checking conditions). However, this is not as effective as I expected, for a kind of frustrating reason. In particular, when invoking `foo.bar()` you still wind up with dependencies on private items. The problem is that the method resolution code scans that list for methods with the name `bar` -- and this winds up touching *all* the methods, even private ones. I can imagine two obvious ways to fix this: - separating fn bodies from fn sigs (#35078, currently being pursued by @flodiebold) - a more aggressive model of incremental that @michaelwoerister has been advocating, in which we hash the intermediate results (e.g., the outputs of collect) so that we can see that the intermediate result hasn't changed, even if a particular impl item has changed. So all in all I'm not quite sure whether to land this or not. =) It still seems like it has to be a win in some cases, but not with the test cases we have just now. I can try to gin up some test cases, but I'm not sure if they will be totally realistic. On the other hand, some of the early refactorings to the visitor trait seem worthwhile to me regardless. cc #36349 -- well, this is basically a fix for that issue, I guess r? @michaelwoerister NB: Based atop of @eddyb's PR #37402; don't land until that lands.

@flodiebold

Separate impl items from the parent impl This change separates impl item bodies out of the impl itself. This gives incremental more resolution. In so doing, it refactors how the visitors work, and cleans up a bit of the collect/check logic (mostly by moving things out of collect that didn't really belong there, because they were just checking conditions). However, this is not as effective as I expected, for a kind of frustrating reason. In particular, when invoking `foo.bar()` you still wind up with dependencies on private items. The problem is that the method resolution code scans that list for methods with the name `bar` -- and this winds up touching *all* the methods, even private ones. I can imagine two obvious ways to fix this: - separating fn bodies from fn sigs (#35078, currently being pursued by @flodiebold) - a more aggressive model of incremental that @michaelwoerister has been advocating, in which we hash the intermediate results (e.g., the outputs of collect) so that we can see that the intermediate result hasn't changed, even if a particular impl item has changed. So all in all I'm not quite sure whether to land this or not. =) It still seems like it has to be a win in some cases, but not with the test cases we have just now. I can try to gin up some test cases, but I'm not sure if they will be totally realistic. On the other hand, some of the early refactorings to the visitor trait seem worthwhile to me regardless. cc #36349 -- well, this is basically a fix for that issue, I guess r? @michaelwoerister NB: Based atop of @eddyb's PR rust-lang/rust#37402; don't land until that lands.

rust-highfive assigned nikomatsakis Oct 25, 2016

eddyb mentioned this pull request Oct 25, 2016

[2/n] rustc_metadata: move is_extern_item to trans. #37401

Merged

eddyb changed the title ~~rustc: unify and simplify managing associated items.~~ [3/n] rustc: unify and simplify managing associated items. Oct 25, 2016

eddyb force-pushed the lazy-3 branch from cc39dea to 9c8ceb6 Compare October 25, 2016 16:03

eddyb mentioned this pull request Oct 25, 2016

[4/n] rustc: harden against InferOk having obligations in more cases. #37404

Merged

eddyb force-pushed the lazy-3 branch from 9c8ceb6 to 6eb233a Compare October 26, 2016 21:23

eddyb force-pushed the lazy-3 branch 3 times, most recently from 724ab66 to a222706 Compare October 28, 2016 22:27

nikomatsakis approved these changes Oct 31, 2016

View reviewed changes

eddyb force-pushed the lazy-3 branch from a222706 to c8578db Compare November 2, 2016 01:43

sophiajt mentioned this pull request Nov 4, 2016

Rollup of 17 pull requests #37581

Closed

eddyb force-pushed the lazy-3 branch 2 times, most recently from cd64ea7 to 3f0f094 Compare November 7, 2016 14:10

nikomatsakis mentioned this pull request Nov 9, 2016

Separate impl items from the parent impl #37660

Merged

eddyb mentioned this pull request Nov 9, 2016

[7/n] rustc: desugar UFCS in HIR and don't use DefMap for associated resolutions. #37676

Merged

eddyb force-pushed the lazy-3 branch from 3f0f094 to 5bd7dd6 Compare November 9, 2016 23:43

rustc: unify and simplify managing associated items.

de0ffad

eddyb force-pushed the lazy-3 branch from 5bd7dd6 to de0ffad Compare November 10, 2016 00:06

eddyb mentioned this pull request Nov 10, 2016

Rollup of 5 pull requests #37678

Merged

bors added a commit that referenced this pull request Nov 10, 2016

Auto merge of #37678 - eddyb:rollup, r=eddyb

b46ce08

Rollup of 5 pull requests - Successful merges: #37402, #37412, #37661, #37664, #37667 - Failed merges:

bors merged commit de0ffad into rust-lang:master Nov 10, 2016

eddyb deleted the lazy-3 branch November 10, 2016 08:54

mcarton mentioned this pull request Nov 16, 2016

Rustup to rustc 1.15.0-nightly (0ed951993 2016-11-14) and bump to 0.0.99 rust-lang/rust-clippy#1344

Merged

eddyb mentioned this pull request Jan 11, 2017

rustc panics on fn that returns associated struct tuple with Self #38919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3/n] rustc: unify and simplify managing associated items. #37402

[3/n] rustc: unify and simplify managing associated items. #37402

eddyb commented Oct 25, 2016 •

edited

Loading

rust-highfive commented Oct 25, 2016

bors commented Oct 26, 2016

bors commented Oct 28, 2016

bors commented Oct 30, 2016

nikomatsakis left a comment

nikomatsakis Oct 31, 2016

eddyb Nov 1, 2016 •

edited

Loading

eddyb commented Nov 2, 2016

bors commented Nov 2, 2016

bors commented Nov 3, 2016

bors commented Nov 3, 2016

alexcrichton commented Nov 3, 2016

bors commented Nov 4, 2016

bors commented Nov 6, 2016

bors commented Nov 7, 2016

bors commented Nov 7, 2016

alexcrichton commented Nov 7, 2016

bors commented Nov 7, 2016

bors commented Nov 7, 2016

eddyb commented Nov 8, 2016

bors commented Nov 8, 2016

bors commented Nov 8, 2016

eddyb commented Nov 8, 2016

alexcrichton commented Nov 8, 2016

bors commented Nov 9, 2016

bors commented Nov 9, 2016

eddyb commented Nov 10, 2016

bors commented Nov 10, 2016

[3/n] rustc: unify and simplify managing associated items. #37402

[3/n] rustc: unify and simplify managing associated items. #37402

Conversation

eddyb commented Oct 25, 2016 • edited Loading

rust-highfive commented Oct 25, 2016

bors commented Oct 26, 2016

bors commented Oct 28, 2016

bors commented Oct 30, 2016

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 31, 2016

Choose a reason for hiding this comment

eddyb Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

eddyb commented Nov 2, 2016

bors commented Nov 2, 2016

bors commented Nov 3, 2016

bors commented Nov 3, 2016

alexcrichton commented Nov 3, 2016

bors commented Nov 4, 2016

bors commented Nov 6, 2016

bors commented Nov 7, 2016

bors commented Nov 7, 2016

alexcrichton commented Nov 7, 2016

bors commented Nov 7, 2016

bors commented Nov 7, 2016

eddyb commented Nov 8, 2016

bors commented Nov 8, 2016

bors commented Nov 8, 2016

eddyb commented Nov 8, 2016

alexcrichton commented Nov 8, 2016

bors commented Nov 9, 2016

bors commented Nov 9, 2016

eddyb commented Nov 10, 2016

bors commented Nov 10, 2016

eddyb commented Oct 25, 2016 •

edited

Loading

eddyb Nov 1, 2016 •

edited

Loading