Move monomorphize::resolve() to librustc #44896

qmx · 2017-09-28T03:28:45Z

this moves monomorphize::resolve(..) to librustc, and re-enables inlining for some trait methods, fixing #44389

@nikomatsakis I've kept the calls to the new ty::Instance::resolve(....) always .unwrap()-ing for the moment, how/do you want to add more debugging info via .unwrap_or() or something like this?

we still have some related resolve_* functions on monomorphize, but I wasn't sure moving them was into the scope for this PR too.

@eddyb mind to take a look too?

rust-highfive · 2017-09-28T03:28:56Z

r? @nikomatsakis

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

eddyb · 2017-09-28T10:47:45Z

src/librustc/ty/instance.rs

+        result
+    }
+
+


Two unnecessary empty lines.

eddyb · 2017-09-28T10:48:30Z

src/librustc_trans/callee.rs

+    get_fn(ccx, ty::Instance::resolve(ccx.tcx(),
+    ty::ParamEnv::empty(traits::Reveal::All),
+    def_id,
+    substs).unwrap())


The indentation here is off - it should be at least once to the right for the 3 above lines.

eddyb · 2017-09-28T10:48:52Z

src/librustc_trans/mir/block.rs

+                        (Some(ty::Instance::resolve(bcx.ccx.tcx(),
+                        ty::ParamEnv::empty(traits::Reveal::All),
+                        def_id,
+                        substs).unwrap()),


Same indentation problem here.

eddyb · 2017-09-28T10:48:58Z

src/librustc_trans/mir/constant.rs

+        let instance = ty::Instance::resolve(ccx.tcx(),
+        ty::ParamEnv::empty(traits::Reveal::All),
+        def_id,
+        substs).unwrap();


nikomatsakis

OK I left a few thoughts. Most some pre-existing nits that aren't your fault, but which I would like to see fixed. The other question in my mind is whether to make this a query. We probably need some more guidelines on that point. =)

nikomatsakis · 2017-09-28T13:17:33Z

src/librustc_trans/callee.rs

@@ -179,5 +180,8 @@ pub fn resolve_and_get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
                                    substs: &'tcx Substs<'tcx>)
                                    -> ValueRef
 {
-    get_fn(ccx, monomorphize::resolve(ccx.tcx(), def_id, substs))
+    get_fn(ccx, ty::Instance::resolve(ccx.tcx(),


Nit: I try not to be a stickler on formatting (rustfmt can't come soon enough), but I find this particular line rather hard to parse. It looks to my eye like the lines below are additional arguments to get_fn. I'd prefer to see ty::Instance::resolve on a line of its own, and its arguments indented relative to it.

For example, you might do something like:

get_fn( ccx, ty::Instance::resolve( ccx.tcx(), ty::ParamEnv::empty(traits::Reveal::All), def_id, substs, ).unwrap(), )

(I believe this is roughly what rustfmt would do here.)

nikomatsakis · 2017-09-28T14:09:42Z

src/librustc/ty/instance.rs

+
+    /// The point where linking happens. Resolve a (def_id, substs)
+    /// pair to an instance.
+    pub fn resolve(tcx: TyCtxt<'a, 'tcx, 'tcx>,


I wonder if this should be a query. Seems like a handy thing to cache.

If we did make it a query, I'd prefer to see the provider moved into traits/instance or something.

nikomatsakis · 2017-09-28T14:21:24Z

src/librustc/ty/instance.rs

@@ -111,4 +115,192 @@ impl<'a, 'b, 'tcx> Instance<'tcx> {
    pub fn def_id(&self) -> DefId {
        self.def.def_id()
    }
+
+    /// The point where linking happens. Resolve a (def_id, substs)
+    /// pair to an instance.


I think this comment could say a lot more. Example:

// Resolve a (def_id, substs) pair to an (optional) instance -- most commonly, // this is used to find the precise code that will run for a trait method invocation, // if known. // // Returns `None` if we cannot resolve `Instance` to a specific instance. // For example, in a context like this, // // ``` // fn foo<T: Debug>(t: T) { ... } // ``` // // trying to resolve `Debug::fmt` applied to `T` will yield `None`, because we do not // know what code ought to run. (Note that this setting is also affected by the // `RevealMode` in the parameter environment.) // // Presuming that coherence and type-check have succeeded, if this method is invoked // in a monomorphic context (i.e., like during trans), then it is guaranteed to return // `Some`.

nikomatsakis · 2017-09-28T14:23:42Z

src/librustc/ty/instance.rs

+                substs: rcvr_substs
+            })
+        }
+        _ => {


This is pre-existing, but I would rather see this match be made exhaustive. My usual rule of thumb is this:

If a new variant is added to traits::Vtable, how likely is that this code will have to be changed?

If the answer is anything other than "rather unlikely", the match should be exhaustive. Here I would say the answer is "very likely".

To make it exhaustive, I would probably remove the if clauses from the various arms, and move them into the body. e.g., if Some(trait_id) == clone_trait { ... } else { None }.

nikomatsakis · 2017-09-28T14:24:36Z

src/librustc/ty/instance.rs

+                // These are both the same at trans time.
+                Ok(true)
+            }
+        _ => Err(()),


Also pre-existing, but I would make this exhaustive by adding variants like:

(ty::ClosureKind::FnMut, _) | (ty::ClosureKind::FnOnce, _) => Err(())

this fixes rust-lang#44389

arielb1 · 2017-09-30T09:38:03Z

src/librustc_trans/callee.rs

+            ccx.tcx(),
+            ty::ParamEnv::empty(traits::Reveal::All),
+            def_id,
+            substs,)


nit: extra comma

arielb1 · 2017-09-30T09:39:27Z

r+ with stray comma fixed

qmx · 2017-09-30T15:09:58Z

@arielb1 there you go - I could swear I've fixed before :/

arielb1 · 2017-10-01T13:02:02Z

src/librustc_trans/callee.rs

@@ -186,7 +186,7 @@ pub fn resolve_and_get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
            ccx.tcx(),
            ty::ParamEnv::empty(traits::Reveal::All),
            def_id,
-            substs,)
-        .unwrap()
+            substs,


still a stray comma,

arielb1 · 2017-10-01T15:33:46Z

@bors r+

bors · 2017-10-01T15:33:47Z

📌 Commit 11e141e has been approved by arielb1

bors · 2017-10-01T16:11:02Z

⌛ Testing commit 11e141e with merge 8e8b47f5c004d8f30dc4bb65cc60206ec2a81b88...

bors · 2017-10-01T17:32:31Z

💔 Test failed - status-appveyor

aidanhs · 2017-10-02T11:17:50Z

@bors retry

#43402

bors · 2017-10-02T20:17:43Z

⌛ Testing commit 11e141e with merge fa1d352c7c162da84114bbd4ab58edff3368ceff...

bors · 2017-10-02T21:16:12Z

💔 Test failed - status-travis

sfackler · 2017-10-02T22:02:54Z

@bors retry

bors · 2017-10-03T01:05:27Z

⌛ Testing commit 11e141e with merge 6c39ff409c79d5d4acc5025988a45d10359189e6...

bors · 2017-10-03T02:26:22Z

💔 Test failed - status-appveyor

qmx · 2017-10-03T03:01:10Z

Apparently, this is failing on a completely unrelated thing - test suite runs cleanly locally :(

arielb1 · 2017-10-03T09:35:41Z

Spurious failure in issue-26092 #43402
@bors retry

bors · 2017-10-03T12:18:19Z

⌛ Testing commit 11e141e with merge 8891044...

@nikomatsakis

Move monomorphize::resolve() to librustc this moves `monomorphize::resolve(..)` to librustc, and re-enables inlining for some trait methods, fixing #44389 @nikomatsakis I've kept the calls to the new `ty::Instance::resolve(....)` always `.unwrap()`-ing for the moment, how/do you want to add more debugging info via `.unwrap_or()` or something like this? we still have some related `resolve_*` functions on monomorphize, but I wasn't sure moving them was into the scope for this PR too. @eddyb mind to take a look too?

bors · 2017-10-03T14:44:04Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 8891044 to master...

rust-highfive assigned nikomatsakis Sep 28, 2017

eddyb reviewed Sep 28, 2017

View reviewed changes

src/librustc/ty/instance.rs Outdated

result

}

Copy link

Member

eddyb Sep 28, 2017

Choose a reason for hiding this comment

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

Two unnecessary empty lines.

eddyb reviewed Sep 28, 2017

View reviewed changes

qmx force-pushed the move-resolve-to-librustc branch from 9789b66 to 9e24115 Compare September 28, 2017 12:40

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2017

nikomatsakis requested changes Sep 28, 2017

View reviewed changes

qmx added 10 commits September 29, 2017 22:34

copy resolve() over to librustc

baa0105

re-enable mir inlining across trait methods

3bd09de

this fixes rust-lang#44389

always use resolve

5eb302f

make it not to be a method

24fc50f

take ParamEnv into account when resolving

8a5800e

stop using monomorphize::resolve()

081b29c

style fixes as requested by @eddyb

2922066

fix formatting

08e1f0b

improve documentation for resolve()

59e778e

make match exaustive

a29c770

qmx force-pushed the move-resolve-to-librustc branch from 6028d9c to a29c770 Compare September 30, 2017 02:35

arielb1 reviewed Sep 30, 2017

View reviewed changes

src/librustc_trans/callee.rs Outdated

ccx.tcx(),

ty::ParamEnv::empty(traits::Reveal::All),

def_id,

substs,)

Copy link

Contributor

arielb1 Sep 30, 2017

Choose a reason for hiding this comment

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

nit: extra comma

weird formatting

b24a672

arielb1 reviewed Oct 1, 2017

View reviewed changes

stray comma

11e141e

nikomatsakis approved these changes Oct 3, 2017

View reviewed changes

bors merged commit 11e141e into rust-lang:master Oct 3, 2017

bors mentioned this pull request Oct 3, 2017

Turn trans_fulfill_obligation into a query #44967

Merged

qmx deleted the move-resolve-to-librustc branch October 3, 2017 15:01

Move monomorphize::resolve() to librustc #44896

Move monomorphize::resolve() to librustc #44896

Uh oh!

Conversation

qmx commented Sep 28, 2017

Uh oh!

rust-highfive commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Sep 28, 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!

nikomatsakis left a comment

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!

arielb1 commented Sep 30, 2017

Uh oh!

qmx commented Sep 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Oct 1, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

aidanhs commented Oct 2, 2017

Uh oh!

bors commented Oct 2, 2017

Uh oh!

bors commented Oct 2, 2017

Uh oh!

sfackler commented Oct 2, 2017

Uh oh!

bors commented Oct 3, 2017

Uh oh!

bors commented Oct 3, 2017

Uh oh!

qmx commented Oct 3, 2017

Uh oh!

arielb1 commented Oct 3, 2017

Uh oh!

bors commented Oct 3, 2017

Uh oh!

bors commented Oct 3, 2017

Uh oh!

Uh oh!

eddyb Sep 28, 2017 •

edited

Loading