Downgrade ProjectionTy's TraitRef to its substs #42388

tbg · 2017-06-02T19:58:51Z

Addresses the second part of #42171 by removing the TraitRef from
ProjectionTy, and directly storing its Substs.

rust-highfive · 2017-06-02T19:58:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

tbg · 2017-06-02T19:59:27Z

r? @nikomatsakis

eddyb · 2017-06-02T20:11:31Z

src/librustc/ty/mod.rs

ty::tls::with is not for regular consumption, only for pretty-printing.

Gone now thanks to your other comment.

eddyb · 2017-06-02T20:12:34Z

src/librustc/ty/structural_impls.rs

Same here, use folder.tcx().

eddyb · 2017-06-02T20:12:41Z

src/librustc/ty/structural_impls.rs

Same here, use visitor.tcx().

No such thing, unfortunately.

Oh, right, visitor. You'll have to update all the implementers that overload visit_trait_ref to handle TyProjection themselves. At this rate I doubt visit_trait_ref is useful anymore since it shouldn't appear in a Ty. What you have to do here fwiw is to visit only substitutions.

Yes, I think we should just replace this code with:

let ty::ProjectionTy { ref substs, item_def_id: _ } = *self; substs.visit_with(visitor)

and be done with it.

eddyb · 2017-06-02T20:12:58Z

src/librustc/util/ppaux.rs

This one is okay, I suppose.

Although I don't think I agree with the indentation, the closure arguments should be on the previous line.

I'm happy to change the indentation to what you prefer. In the meantime, moved |tcx| up where it belongs.

Like I said, I think this can use parametrized.

eddyb · 2017-06-02T20:13:38Z

src/librustc_typeck/constrained_type_params.rs

Pass tcx in from the caller.

You still have two more cases here.

eddyb · 2017-06-02T20:14:27Z

src/librustc/traits/trans/mod.rs

prof.item_def_id might be good enough? cc @nikomatsakis @michaelwoerister

eddyb · 2017-06-02T20:14:52Z

src/librustc/ty/instance.rs

See #42388 (comment)

eddyb · 2017-06-02T20:15:33Z

src/librustc/ty/mod.rs

Can't implement ToPolyTraitRef - nor does that make sense IMO.

What do you mean/what would you like me to do?

Remove the ToPolyTraitRef impl and see what breaks.

Still applicable.

No worries, I track all comments that I haven't answered in the affirmative.

Removing the impl breaks, among other places, process_predicate:

ty::Predicate::Projection(ref data) => { let project_obligation = obligation.with(data.clone()); match project::poly_project_and_unify_type(selcx, &project_obligation) { Ok(None) => { pending_obligation.stalled_on = trait_ref_type_vars(selcx, data.to_poly_trait_ref()); Ok(None) } Ok(v) => Ok(v), Err(e) => Err(CodeProjectionError(e)) } }

Yeah, that could do the work itself. Not sure if data.trait_ref(tcx).to_poly_trait_ref() works though (it might assert).

Okay, going through .trait_ref wouldn't work, TraitRef::to_poly_trait_ref will assert, you need to move this to an inherent method that takes TyCtxt.

I agree with @eddyb that it is what dubious for this type to implement to_poly_trait_ref. An inherent function seems better. Basically is converting a predicate like T: Iterator<Item=Foo> into the underlying T: Iterator trait-reference -- that's not wrong, but it feels like it merits an explicit "cast". This was (however) equally true before, I guess -- but still better to fix.

Ok, I'm looking at this now.

Done. Moved the method off the trait, supplied the tcx in the ~4 call sites.

eddyb · 2017-06-02T20:18:33Z

src/librustc/ty/mod.rs

Replace trait_inputs with data.substs.types().

Done (assume you mean changing to data.0.projection_ty.substs.types().chain(Some(data.0.ty)).collect()

bors · 2017-06-02T22:22:47Z

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

eddyb · 2017-06-03T13:10:40Z

src/librustc/infer/region_inference/mod.rs

mk_projection should not do the lookup itself.

I assume you are suggesting that mk_projection should change its signature, and in particular likely take item_def_id directly? Before I go wild on this, what would the final signature be? There would be some fallout, some call sites have exactly trait_ref and item_name available.

DefId and Substs. No callsites should really need to search for the associated item if they don't already, e.g. anything to do with lang item traits with an associated type should just get the associated type ignoring its name.

Ok. Did the easy part of this - some call sites remain. I'll take another look at those. For example, what am I doing below? Refactor ast_path_to_mono_trait_ref?

let trait_ref = self.ast_path_to_mono_trait_ref(span, trait_def_id, self_ty, trait_segment); debug!("qpath_to_ty: trait_ref={:?}", trait_ref); self.normalize_ty(span, tcx.mk_deprecated_projection(trait_ref, item_segment.name))

The question is where trait_def_id comes from there - there should be an associated type DefId somewhere, and you can combine that with trait_ref.substs.

eddyb · 2017-06-03T13:12:10Z

src/librustc/traits/project.rs

Indeed this is projection_ty.item_def_id. In general avoid using the new trait_ref method where possible.

eddyb · 2017-06-03T13:13:09Z

src/librustc/traits/project.rs

Same here, looks like an earlier PR didn't clean all of these up. Calls of item_name should also be rare.

eddyb · 2017-06-03T13:15:27Z

src/librustc/traits/project.rs

Don't need to call trait_ref to get something from the substs. Maybe add self_ty to ProjectionTy.

eddyb · 2017-06-03T13:17:12Z

src/librustc/ty/relate.rs

Relate substitutions directly. Also the name check above is silly now, should check DefId.

eddyb · 2017-06-03T13:18:18Z

src/librustc/ty/structural_impls.rs

This should lift substitutions instead. from_ref_and_name should likely not exist.

Done. (from_ref_and_name still exists, but I'll keep knocking 'em down and when you're satisfied look at the whole diff again to see what else can go).

eddyb · 2017-06-03T13:19:07Z

src/librustc/ty/structural_impls.rs

Can folding a trait ref be overloaded? i.e. is there any fold_trait_ref? If not you can just fold substitutions.

There's this:

fn fold_trait_ref(&mut self, p: TraitRef) -> TraitRef { noop_fold_trait_ref(p, self) }

Is it overloaded by anything?

Uhm I think I removed on master recently. So no, it wasn't needed, you can just fold substs.

fold_trait_ref should be gone now. Oh wait. What you found is an AST folder facepalms.
Apologies, you could've done this before the rebase too. Just fold the substs.

eddyb · 2017-06-03T13:20:01Z

src/librustc/ty/sty.rs

This indentation is off to the right.

Done. WDYT about the message?

Irrelevant and there is a method to get the DefId anyway, that'd make this cleaner.

Done, here and in the other place.

eddyb · 2017-06-03T13:20:46Z

src/librustc/ty/walk.rs

This is fine.

eddyb · 2017-06-03T13:22:07Z

src/librustc_privacy/lib.rs

This can just call proj.substs.visit_with.

eddyb · 2017-06-03T13:22:50Z

src/librustc_save_analysis/lib.rs

Another unnecessary search for a DefId that is already known.

eddyb · 2017-06-03T13:23:34Z

src/librustc_typeck/astconv.rs

This reminds me that ExistentialProjection also needs to be updated.

tbg · 2017-06-03T13:24:21Z

Thanks @eddyb! Addressed parts of your comments, will come back to the rest later. Avoiding the rebase intentionally for now until the dust has settled.

eddyb · 2017-06-03T13:24:33Z

src/librustc_typeck/astconv.rs

This can probably just work on .item_def_id.

bors · 2017-07-06T10:22:36Z

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

tbg · 2017-07-06T19:02:27Z

ping @nikomatsakis - rebased & green.

nikomatsakis · 2017-07-07T17:00:10Z

@bors r+

bors · 2017-07-07T17:00:12Z

📌 Commit 0d436ed has been approved by nikomatsakis

bors · 2017-07-07T18:23:33Z

⌛ Testing commit 0d436edf030c4afea671069456e1f0084040d1dd with merge 63ff04c3ec23fe9760cc9fe876a3c5d7b989f0d4...

bors · 2017-07-07T18:42:11Z

💔 Test failed - status-travis

tbg · 2017-07-08T00:47:40Z

@eddyb if you have the bandwidth - a PR before me (#42125) in the merge queue used visit_trait_ref, so I had to do another round of refactors. Tests pass now, but I think a pair of eyes is advised (new commits only).

nikomatsakis · 2017-07-10T17:49:15Z

The new commits look good to me. r=me, did you want to fixup first though? (your comment titles suggest you did :)

tbg · 2017-07-10T18:17:14Z

Yep, done. Ready to hit the queue.

nikomatsakis · 2017-07-10T18:18:54Z

@bors r+

bors · 2017-07-10T18:18:55Z

📌 Commit 0b321bf has been approved by nikomatsakis

bors · 2017-07-10T21:05:17Z

⌛ Testing commit 0b321bfe9b47fb4114968497ed3a4cb50b017b70 with merge 5a89479419a0b2823a93041206ed0dad2dc16feb...

bors · 2017-07-10T21:18:08Z

💔 Test failed - status-travis

bors · 2017-07-11T00:04:18Z

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

Addresses the second part of rust-lang#42171 by removing the `TraitRef` from `ProjectionTy`, and directly storing its `Substs`. Closes rust-lang#42171.

nikomatsakis · 2017-07-11T14:58:37Z

@bors r+

bors · 2017-07-11T14:58:38Z

📌 Commit 687ee7f has been approved by nikomatsakis

bors · 2017-07-11T15:14:39Z

⌛ Testing commit 687ee7f with merge 1475e2c...

Downgrade ProjectionTy's TraitRef to its substs Addresses the second part of #42171 by removing the `TraitRef` from `ProjectionTy`, and directly storing its `Substs`. Closes #42171.

bors · 2017-07-11T17:52:38Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 1475e2c to master...

rust-highfive assigned eddyb Jun 2, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Jun 2, 2017

eddyb reviewed Jun 2, 2017

View reviewed changes

src/librustc/traits/trans/mod.rs Outdated

Copy link

Member

eddyb Jun 2, 2017

Choose a reason for hiding this comment

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

prof.item_def_id might be good enough? cc @nikomatsakis @michaelwoerister

eddyb reviewed Jun 2, 2017

View reviewed changes

src/librustc/ty/instance.rs Outdated

Copy link

Member

eddyb Jun 2, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

See #42388 (comment)

eddyb reviewed Jun 2, 2017

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 2, 2017

eddyb reviewed Jun 3, 2017

View reviewed changes

tbg force-pushed the projty-substs branch from e684309 to 0d436ed Compare July 6, 2017 17:18

nikomatsakis approved these changes Jul 7, 2017

View reviewed changes

tbg force-pushed the projty-substs branch 2 times, most recently from b2cd0bf to 3166a02 Compare July 7, 2017 21:11

tbg force-pushed the projty-substs branch from a97bcc5 to 0b321bf Compare July 10, 2017 18:16

Downgrade ProjectionTy's TraitRef to its substs

687ee7f

Addresses the second part of rust-lang#42171 by removing the `TraitRef` from `ProjectionTy`, and directly storing its `Substs`. Closes rust-lang#42171.

tbg force-pushed the projty-substs branch from 0b321bf to 687ee7f Compare July 11, 2017 14:33

bors merged commit 687ee7f into rust-lang:master Jul 11, 2017

tbg deleted the projty-substs branch July 11, 2017 18:02

Downgrade ProjectionTy's TraitRef to its substs #42388

Downgrade ProjectionTy's TraitRef to its substs #42388

Uh oh!

Conversation

tbg commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jun 2, 2017

Uh oh!

tbg commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tbg commented Jun 2, 2017 •

edited

Loading

eddyb Jun 2, 2017 •

edited

Loading

eddyb Jun 10, 2017 •

edited

Loading