Upgrade ProjectionTy's Name to a DefId #42297

tbg · 2017-05-29T15:11:46Z

Part of #42171, in preparation for downgrading the contained TraitRef to
only its substs.

Some inline questions in the diff. Look for FIXME(tschottdorf). These comments
should be addressed before merging.

rust-highfive · 2017-05-29T15:11:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

nikomatsakis

This looks pretty good. I left a few requests for changes. The main question is whether we care about hygiene for these item lookups (as you noted) -- I would guess no, but perhaps @jseyfried or @petrochenkov knows better?

nikomatsakis · 2017-05-30T20:21:13Z

src/librustc/ty/structural_impls.rs

@@ -771,7 +768,8 @@ impl<'tcx> TypeFoldable<'tcx> for ty::ProjectionTy<'tcx> {
    fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, folder: &mut F) -> Self {
        ty::ProjectionTy {
            trait_ref: self.trait_ref.fold_with(folder),
-            item_name: self.item_name,
+            // FIXME(tschottdorf): is this correct, or a case for tls::with?


this is correct as is

nikomatsakis · 2017-05-30T20:21:57Z

src/librustc/ty/sty.rs

@@ -556,9 +556,39 @@ pub struct ProjectionTy<'tcx> {
    /// The trait reference `T as Trait<..>`.
    pub trait_ref: ty::TraitRef<'tcx>,

-    /// The name `N` of the associated type.
-    pub item_name: Name,
+    /// The DefId of the the associated type `N`.


I would say "The DefId of the TraitItem for the associated type N."

PS kudos to you for commenting :)

nikomatsakis · 2017-05-30T20:22:43Z

src/librustc/ty/sty.rs

+    pub fn from_ref_and_name(
+        tcx: TyCtxt,
+        trait_ref: ty::TraitRef<'tcx>,
+        item_name: Name) -> ProjectionTy<'tcx> {


Nit: formatting. I think that in this case, the -> goes on next line, and I personally would put { indented with pub (so that body is clearly distinguished from the signature). But we don't have hard-and-fast rules here. You could also align with the open (.

Done (I think?)

nikomatsakis · 2017-05-30T20:23:25Z

src/librustc/ty/sty.rs

+        //
+        // let ident = self.tcx.adjust(item_name, def_id, self.body_id).0;
+        //
+        // (There is no body_id in scope here). Is the below incorrect?


Yeah, bother, I don't know how hygiene and "associated item lookup" are supposed to interact here. @jseyfried may have thoughts.

Removed as per comment on main thread.

nikomatsakis · 2017-05-30T20:25:10Z

src/librustc/ty/sty.rs

 }
+
+impl<'a, 'tcx> ProjectionTy<'tcx> {
+    pub fn from_ref_and_name(


Maybe add a comment here?

"Construct a ProjectionTy by searching the trait from trait_ref for the associated item named item_name."

nikomatsakis · 2017-05-30T20:26:03Z

src/librustc/ty/util.rs

@@ -667,7 +667,8 @@ impl<'a, 'gcx, 'tcx, W> TypeVisitor<'tcx> for TypeIdHasher<'a, 'gcx, 'tcx, W>
            }
            TyProjection(ref data) => {
                self.def_id(data.trait_ref.def_id);
-                self.hash(data.item_name.as_str());
+                // FIXME(tschottdorf): are there consequences to this change?


we don't actually need the first hash (of data.trait_ref.def_id); the second should suffice, I think.

nikomatsakis · 2017-05-30T20:27:03Z

src/librustc_typeck/check/autoderef.rs

+                                                           ty::ProjectionTy::from_ref_and_name(
+                                                               tcx,
+                                                               trait_ref,
+                                                               Symbol::intern("Target"),


this is fine for now; eventually, though, we could make lang-items for the associated types from "well known" traits like this

Ack, feel free to throw that my way at some point. I'm not sure what it would entail.

nikomatsakis · 2017-05-30T21:11:26Z

@arielb1 writes on IRC that we don't use hygiene for associated item lookup at present (which is also what I thought). This suggests @tschottdorf that the code for ProjectionTy::from_trait_ref_and_name() is fine just as you have it, and you can remove that comment.

Part of rust-lang#42171, in preparation for downgrading the contained `TraitRef` to only its `substs`.

tbg · 2017-05-31T16:35:29Z

Thanks for the review! Comments addressed.

nikomatsakis · 2017-05-31T21:25:35Z

@bors r+

bors · 2017-05-31T21:25:37Z

📌 Commit e5e664f has been approved by nikomatsakis

Upgrade ProjectionTy's Name to a DefId Part of rust-lang#42171, in preparation for downgrading the contained `TraitRef` to only its `substs`. Some inline questions in the diff. Look for `FIXME(tschottdorf)`. These comments should be addressed before merging.

Rollup of 9 pull requests - Successful merges: #42136, #42275, #42286, #42297, #42302, #42306, #42314, #42324, #42347 - Failed merges:

rust-highfive assigned nikomatsakis May 29, 2017

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2017

tbg force-pushed the proj-ty branch from 18b6154 to 7702c4a Compare May 30, 2017 01:07

nikomatsakis requested changes May 30, 2017

View reviewed changes

tbg force-pushed the proj-ty branch from 7702c4a to 2d74471 Compare May 31, 2017 16:32

Upgrade ProjectionTy's Name to a DefId

e5e664f

Part of rust-lang#42171, in preparation for downgrading the contained `TraitRef` to only its `substs`.

tbg force-pushed the proj-ty branch from 2d74471 to e5e664f Compare May 31, 2017 16:35

nikomatsakis approved these changes May 31, 2017

View reviewed changes

nikomatsakis mentioned this pull request May 31, 2017

[RFC] Refactor types to be a (TypeCore, Substs) pair #42340

Open

4 tasks

Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2017

frewsxcv mentioned this pull request Jun 1, 2017

Rollup of 9 pull requests #42345

Closed

frewsxcv mentioned this pull request Jun 1, 2017

Rollup of 8 pull requests #42346

Closed

frewsxcv mentioned this pull request Jun 1, 2017

Rollup of 9 pull requests #42348

Merged

bors added a commit that referenced this pull request Jun 1, 2017

Auto merge of #42348 - frewsxcv:rollup, r=frewsxcv

38efb2e

Rollup of 9 pull requests - Successful merges: #42136, #42275, #42286, #42297, #42302, #42306, #42314, #42324, #42347 - Failed merges:

bors merged commit e5e664f into rust-lang:master Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade ProjectionTy's Name to a DefId #42297

Upgrade ProjectionTy's Name to a DefId #42297

tbg commented May 29, 2017 •

edited

Loading

rust-highfive commented May 29, 2017

nikomatsakis left a comment

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis May 30, 2017

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis May 30, 2017

tbg May 31, 2017

nikomatsakis commented May 30, 2017

tbg commented May 31, 2017

nikomatsakis commented May 31, 2017

bors commented May 31, 2017

Upgrade ProjectionTy's Name to a DefId #42297

Upgrade ProjectionTy's Name to a DefId #42297

Conversation

tbg commented May 29, 2017 • edited Loading

rust-highfive commented May 29, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented May 30, 2017

tbg commented May 31, 2017

nikomatsakis commented May 31, 2017

bors commented May 31, 2017

tbg commented May 29, 2017 •

edited

Loading