Turn `trans_fulfill_obligation` into a query #44967

wesleywiser · Oct 2, 2017

Part of #44891

rust-highfive · Oct 2, 2017

r? @nikomatsakis

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

wesleywiser · Oct 2, 2017

wesleywiser reviewed Oct 2, 2017

View changes

wesleywiser · Oct 2, 2017

src/librustc/traits/trans/mod.rs

+/// Assumes that this is run after the entire crate has been successfully type-checked.
+pub fn trans_fulfill_obligation<'a, 'tcx>(ty: TyCtxt<'a, 'tcx, 'tcx>,
+                                          (def_id, param_env, trait_ref):
+                                          (DefId, ty::ParamEnv<'tcx>, ty::PolyTraitRef<'tcx>))


I changed this to take a DefId instead of a Span because it looked like all of the other queries used DefId and not Spans. However, there were two places where DUMMY_SP was passed in. I changed those to use DefIds that seemed appropriate but I'm not sure. I'll indicate those below.

If that wasn't the right thing to do, I can revert it.

Hm, @nikomatsakis would know more about how the span here is actually used.

wesleywiser · Oct 2, 2017

src/librustc_trans/monomorphize.rs

@@ -113,7 +111,7 @@ fn resolve_associated_item<'a, 'tcx>(

    let trait_ref = ty::TraitRef::from_method(tcx, trait_id, rcvr_substs);
    let vtbl = tcx.trans_fulfill_obligation(
-        DUMMY_SP, ty::ParamEnv::empty(traits::Reveal::All), ty::Binder(trait_ref));
+        (trait_id, ty::ParamEnv::empty(traits::Reveal::All), ty::Binder(trait_ref)));


This was the first place DUMMY_SP was passed in.

wesleywiser · Oct 2, 2017

src/librustc_trans/monomorphize.rs

        substs: tcx.mk_substs_trait(source_ty, &[target_ty])
    });

    match tcx.trans_fulfill_obligation(
-        DUMMY_SP, ty::ParamEnv::empty(traits::Reveal::All), trait_ref) {
+        (def_id, ty::ParamEnv::empty(traits::Reveal::All), trait_ref)) {


This was the other place.

michaelwoerister · Oct 2, 2017

michaelwoerister reviewed Oct 2, 2017

View changes

michaelwoerister · Oct 2, 2017

src/librustc/ich/impls_ty.rs

+    fn hash_stable<W: StableHasherResult>(&self,
+                                          hcx: &mut StableHashingContext<'gcx>,
+                                          hasher: &mut StableHasher<W>) {
+        use traits::Vtable::*;


The enum discriminant also needs to be hashed:

mem::discriminant(self).hash_stable(hcx, hasher);

michaelwoerister · Oct 2, 2017

michaelwoerister reviewed Oct 2, 2017

View changes

michaelwoerister · Oct 2, 2017

src/librustc/ich/impls_ty.rs

+    fn hash_stable<W: StableHasherResult>(&self,
+                                          hcx: &mut StableHashingContext<'gcx>,
+                                          hasher: &mut StableHasher<W>) {
+        self.impl_def_id.hash_stable(hcx, hasher);


So what we usually do here is use an exhaustive destructuring let statement, so in case a field gets added or removed, we don't miss updating the HashStable implementation:

let traits::VtableImplData { impl_def_id, substs, ref nested, } = *self; impl_def_id.hash_stable(hcx, hasher); substs.hash_stable(hcx, hasher); nested.hash_stable(hcx, hasher);

That means a bit more typing but has proven to be very useful in catching oversights.

michaelwoerister · Oct 2, 2017

Very nice, thank you, @wesleywiser!

Please update the HashStable implementation to use destructuring let statements.

I'll let @nikomatsakis do the rest of the review since trait selection is his field.

arielb1 · Oct 2, 2017

r? @arielb1

@wesleywiser

I think you should just not pass the span/defid. It's just used for error reporting, and passing the span around as the query key will prevent all caching.

nikomatsakis · Oct 3, 2017

I agree with @arielb1 -- the query key should just be ParamEnv and TraitRef, we don't need the def-id, we can instead use a dummy span.

nikomatsakis · Oct 3, 2017

nikomatsakis reviewed Oct 3, 2017

View changes

nikomatsakis · Oct 3, 2017

src/librustc/traits/trans/mod.rs

+
+        let span = ty.def_span(def_id);
+
+        let obligation_cause = ObligationCause::misc(span,


here you can use ObligationCause::dummy() instead

nikomatsakis · Oct 3, 2017

nikomatsakis reviewed Oct 3, 2017

View changes

nikomatsakis · Oct 3, 2017

src/librustc/traits/trans/mod.rs

+                debug!("Encountered ambiguity selecting `{:?}` during trans, \
+                        presuming due to overflow",
+                        trait_ref);
+                ty.sess.span_fatal(span,


I actually think this case .. hmm .. maybe cannot occur? That is, @arielb1, would that code you added that causes compilation to abort when types get too large perhaps kick in first?

bors · Oct 3, 2017

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

wesleywiser · Oct 4, 2017

wesleywiser reviewed Oct 4, 2017

View changes

wesleywiser · Oct 4, 2017

src/librustc/traits/trans/mod.rs

+                                        (selection ambiguity)");
+            }
+            Err(e) => {
+                span_bug!(span, "Encountered error `{:?}` selecting `{:?}` during trans",


What should I do about this span here? Pass DUMMY_SP?

Just do a bug! I think. I think for better error reporting we should just dump the query stack during each ICE.

wesleywiser · Oct 6, 2017

I believe I've resolved all of the review feedback.

theotherjimmy · Oct 8, 2017

theotherjimmy reviewed Oct 8, 2017

View changes

theotherjimmy · Oct 8, 2017

src/librustc/ty/maps/plumbing.rs

@@ -765,6 +765,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
        DepKind::SpecializationGraph => { force!(specialization_graph_of, def_id!()); }
        DepKind::ObjectSafety => { force!(is_object_safe, def_id!()); }
        DepKind::TraitImpls => { force!(trait_impls_of, def_id!()); }
+        DepKind::FulfillObligation => { force!(item_body_nested_bodies, def_id!()); }


Should this be forcing fulfull_obligation instead?

Fixed by making it a bug. I don't know how we'd recreate the query key so that seems appropriate to me, but I'm not 100% sure.

nikomatsakis · Oct 9, 2017

nikomatsakis approved these changes Oct 9, 2017

View changes

So this looks right to me. The only question in my mind is if the one case -- which used to be a span_fatal and is now a bug! is truly a bug. If not, I suspect that we should convert the query to yield a Result or Option or something, and let the caller report the problem.

nikomatsakis · Oct 9, 2017

@bors r+

bors · Oct 9, 2017

📌 Commit 31f4b57 has been approved by nikomatsakis

bors · Oct 10, 2017

⌛️ Testing commit 31f4b57 with merge bf27afe...

bors · Oct 10, 2017

💔 Test failed - status-travis

wesleywiser · Oct 11, 2017

Android builder failed. Seems related to some kind of network issue?

[00:03:56] + /android/sdk/tools/android update sdk -a --no-ui --filter platform-tools,android-18,sys-img-armeabi-v7a-android-18
[00:03:57] Refresh Sources:
[00:03:57] Fetching https://dl.google.com/android/repository/addons_list-2.xml
[00:04:02] Failed to fetch URL https://dl.google.com/android/repository/addons_list-2.xml, reason: peer not authenticated
[00:04:02] Fetched Add-ons List successfully
[00:04:02] Refresh Sources
[00:04:02] Fetching URL: https://dl.google.com/android/repository/repository-11.xml
[00:04:02] Failed to fetch URL https://dl.google.com/android/repository/repository-11.xml, reason: SSLPeerUnverified peer not authenticated
[00:04:02] Refresh Sources:
[00:04:02] Fetching URL: https://dl.google.com/android/repository/repository-11.xml
[00:04:02] Failed to fetch URL https://dl.google.com/android/repository/repository-11.xml, reason: SSLPeerUnverified peer not authenticated
[00:04:02] There is nothing to install or update.
[00:04:06] + true
[00:04:06] + echo yes
[00:04:06] + create_avd armeabi-v7a 18
[00:04:06] + abi=armeabi-v7a
[00:04:06] + api=18
[00:04:06] + echo no
[00:04:06] + /android/sdk/tools/android create avd --name armeabi-v7a-18 --target android-18 --abi armeabi-v7a
[00:04:06] Error: Target id is not valid. Use 'android list targets' to get the target ids.
[00:04:07] The command '/bin/sh -c . /scripts/android-sdk.sh && download_and_create_avd tools_r25.2.5-linux.zip armeabi-v7a 18' returned a non-zero code: 1
[00:04:07] The command has failed after 5 attempts.
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.

kennytm · Oct 11, 2017

@bors retry

Android SDK HTTPS issue (fixed in #45193)

bors · Oct 11, 2017

⌛️ Testing commit 31f4b57 with merge 58bb629...

bors · Oct 12, 2017

💔 Test failed - status-travis

wesleywiser · Oct 12, 2017

Not sure what this means:

[01:40:10] ------------------------------------------
[01:40:10] stderr:
[01:40:10] ------------------------------------------
[01:40:10]
[01:40:10] ------------------------------------------
[01:40:10]
[01:40:10] thread '[debuginfo-lldb] debuginfo/lexical-scopes-in-block-expression.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2425:8
[01:40:10] note: Run with RUST_BACKTRACE=1 for a backtrace.
[01:40:10]
[01:40:10]
[01:40:10] failures:
[01:40:10] [debuginfo-lldb] debuginfo/lexical-scopes-in-block-expression.rs
[01:40:10]
[01:40:10] test result: �[31mFAILED�(B�[m. 97 passed; 1 failed; 10 ignored; 0 measured; 0 filtered out
[01:40:10]
[01:40:10] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:323:21
[01:40:10]
[01:40:10]
[01:40:10] command did not execute successfully: "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/stage0-tools/x86_64-apple-darwin/release/compiletest" "--compile-lib-path" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/stage2/lib" "--run-lib-path" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/stage2/lib/rustlib/x86_64-apple-darwin/lib" "--rustc-path" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/stage2/bin/rustc" "--src-base" "/Users/travis/build/rust-lang/rust/src/test/debuginfo" "--build-base" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/test/debuginfo" "--stage-id" "stage2-x86_64-apple-darwin" "--mode" "debuginfo-lldb" "--target" "x86_64-apple-darwin" "--host" "x86_64-apple-darwin" "--llvm-filecheck" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/llvm/build/bin/FileCheck" "--nodejs" "/Users/travis/.nvm/versions/node/v6.9.1/bin/node" "--host-rustcflags" "-Crpath -O" "--target-rustcflags" "-Crpath -O -Lnative=/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/native/rust-test-helpers" "--docck-python" "/usr/local/opt/python/bin/python2.7" "--lldb-python" "/usr/bin/python" "--lldb-version" "lldb-360.1.70" "--lldb-python-dir" "/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Resources/Python" "--llvm-version" "4.0.1\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:40:10] expected success, got: exit code: 101
[01:40:10]
[01:40:10]
[01:40:10] failed to run: /Users/travis/build/rust-lang/rust/build/bootstrap/debug/bootstrap test
[01:40:10] Build completed unsuccessfully in 0:28:09
[01:40:10] make: *** [check] Error 1
[01:40:10] make: INTERNAL: Exiting with 1 jobserver tokens available; should be 2!

kennytm · Oct 12, 2017

@bors retry LLDB segfault.

bors · Oct 12, 2017

⌛️ Testing commit 31f4b57 with merge 39fd958...

bors · Oct 12, 2017

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

rust-highfive assigned nikomatsakis Oct 2, 2017

arielb1 assigned arielb1 and unassigned nikomatsakis Oct 2, 2017

carols10cents added the S-waiting-on-review label Oct 2, 2017

wesleywiser force-pushed the wesleywiser:trans_fulfill_obligation branch 2 times, most recently from dbddd4e to a9862b0 Oct 6, 2017

Turn `trans_fulfill_obligation` into a query
Part of #44891

Loading status checks…

31f4b57

wesleywiser force-pushed the wesleywiser:trans_fulfill_obligation branch from a9862b0 to 31f4b57 Oct 8, 2017

nikomatsakis assigned nikomatsakis and unassigned arielb1 Oct 9, 2017

bors added a commit that referenced this pull request Oct 10, 2017

Auto merge of #44967 - wesleywiser:trans_fulfill_obligation, r=nikoma…
…tsakis Turn `trans_fulfill_obligation` into a query Part of #44891

Loading status checks…

bf27afe

bors added a commit that referenced this pull request Oct 11, 2017

Auto merge of #44967 - wesleywiser:trans_fulfill_obligation, r=nikoma…
…tsakis Turn `trans_fulfill_obligation` into a query Part of #44891

Loading status checks…

58bb629

kennytm referenced this pull request Oct 12, 2017
Open
Spurious LLDB segfault (signal 11) #45230

bors added a commit that referenced this pull request Oct 12, 2017

Auto merge of #44967 - wesleywiser:trans_fulfill_obligation, r=nikoma…
…tsakis Turn `trans_fulfill_obligation` into a query Part of #44891

Loading status checks…

39fd958

bors merged commit 31f4b57 into rust-lang:master Oct 12, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Oct 12, 2017

Merged

Queryify Vtable methods #45137

Merged

Create NormalizeTy query #44984

rust-lang/rust

Join GitHub today

Turn `trans_fulfill_obligation` into a query #44967

Conversation

wesleywiser commented Oct 2, 2017

rust-highfive assigned nikomatsakis Oct 2, 2017

This comment has been minimized.

rust-highfive commented Oct 2, 2017

wesleywiser reviewed Oct 2, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

michaelwoerister reviewed Oct 2, 2017 View changes

This comment has been minimized.

michaelwoerister reviewed Oct 2, 2017 View changes

This comment has been minimized.

michaelwoerister Oct 2, 2017 • edited

This comment has been minimized.

michaelwoerister commented Oct 2, 2017

This comment has been minimized.

arielb1 commented Oct 2, 2017

arielb1 assigned arielb1 and unassigned nikomatsakis Oct 2, 2017

carols10cents added the S-waiting-on-review label Oct 2, 2017

This comment has been minimized.

nikomatsakis commented Oct 3, 2017

nikomatsakis reviewed Oct 3, 2017 View changes

This comment has been minimized.

nikomatsakis reviewed Oct 3, 2017 View changes

This comment has been minimized.

nikomatsakis Oct 3, 2017 • edited

This comment has been minimized.

bors commented Oct 3, 2017

wesleywiser reviewed Oct 4, 2017 View changes

This comment has been minimized.

This comment has been minimized.

arielb1 Oct 4, 2017 • edited

wesleywiser force-pushed the wesleywiser:trans_fulfill_obligation branch 2 times, most recently from dbddd4e to a9862b0 Oct 6, 2017

This comment has been minimized.

wesleywiser commented Oct 6, 2017

theotherjimmy reviewed Oct 8, 2017 View changes

This comment has been minimized.

This comment has been minimized.

wesleywiser force-pushed the wesleywiser:trans_fulfill_obligation branch from a9862b0 to 31f4b57 Oct 8, 2017

nikomatsakis approved these changes Oct 9, 2017 View changes

nikomatsakis left a comment

This comment has been minimized.

nikomatsakis commented Oct 9, 2017

This comment has been minimized.

bors commented Oct 9, 2017

nikomatsakis assigned nikomatsakis and unassigned arielb1 Oct 9, 2017

This comment has been minimized.

bors commented Oct 10, 2017

bors added a commit that referenced this pull request Oct 10, 2017

This comment has been minimized.

bors commented Oct 10, 2017

This comment has been minimized.

wesleywiser commented Oct 11, 2017

This comment has been minimized.

kennytm commented Oct 11, 2017

This comment has been minimized.

bors commented Oct 11, 2017

bors added a commit that referenced this pull request Oct 11, 2017

This comment has been minimized.

bors commented Oct 12, 2017

This comment has been minimized.

wesleywiser commented Oct 12, 2017

This comment has been minimized.

kennytm commented Oct 12, 2017

kennytm referenced this pull request Oct 12, 2017

This comment has been minimized.

bors commented Oct 12, 2017

bors added a commit that referenced this pull request Oct 12, 2017

This comment has been minimized.

bors commented Oct 12, 2017

Hide details View details bors merged commit 31f4b57 into rust-lang:master Oct 12, 2017 2 checks passed

2 checks passed

This was referenced Oct 12, 2017

wesleywiser reviewed Oct 2, 2017

View changes

michaelwoerister reviewed Oct 2, 2017

View changes

michaelwoerister reviewed Oct 2, 2017

View changes

michaelwoerister Oct 2, 2017 •

edited

nikomatsakis reviewed Oct 3, 2017

View changes

nikomatsakis reviewed Oct 3, 2017

View changes

nikomatsakis Oct 3, 2017 •

edited

wesleywiser reviewed Oct 4, 2017

View changes

arielb1 Oct 4, 2017 •

edited

wesleywiser force-pushed the wesleywiser:trans_fulfill_obligation branch 2 times, most recently from `dbddd4e` to `a9862b0` Oct 6, 2017

theotherjimmy reviewed Oct 8, 2017

View changes

wesleywiser force-pushed the wesleywiser:trans_fulfill_obligation branch from `a9862b0` to `31f4b57` Oct 8, 2017

nikomatsakis approved these changes Oct 9, 2017

View changes

bors merged commit `31f4b57` into rust-lang:master Oct 12, 2017
2 checks passed