Improve query cycle errors for parallel queries

src/librustc/ty/context.rs

@@ -1942,8 +1942,12 @@ pub mod tls {
     /// This is a callback from libsyntax as it cannot access the implicit state
     /// in librustc otherwise
     fn span_debug(span: syntax_pos::Span, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        with(|tcx| {
-            write!(f, "{}", tcx.sess.source_map().span_to_string(span))
+        with_opt(|tcx| {
+            if let Some(tcx) = tcx {
+                write!(f, "{}", tcx.sess.source_map().span_to_string(span))
+            } else {
+                syntax_pos::default_span_debug(span, f)
+            }
         })
     }
 

src/librustc/ty/query/job.rs

@@ -103,8 +103,11 @@ impl<'tcx> QueryJob<'tcx> {
                     condvar: Condvar::new(),
                 });
                 self.latch.await(&waiter);
-
-                match Lrc::get_mut(&mut waiter).unwrap().cycle.get_mut().take() {
+                // FIXME: Get rid of this lock. We have ownership of the QueryWaiter
+                // although another thread may still have a Lrc reference so we cannot
+                // use Lrc::get_mut
+                let mut cycle = waiter.cycle.lock();
+                match cycle.take() {
                     None => Ok(()),
                     Some(cycle) => Err(cycle)
                 }
@@ -326,19 +329,17 @@ fn connected_to_root<'tcx>(
     query: Lrc<QueryJob<'tcx>>,
     visited: &mut FxHashSet<*const QueryJob<'tcx>>
 ) -> bool {
-    // This query is connected to the root (it has no query parent), return true
-    if query.parent.is_none() {
-        return true;
-    }
-
     // We already visited this or we're deliberately ignoring it
     if visited.contains(&query.as_ptr()) {
         return false;
     }
 
-    visited.insert(query.as_ptr());
+    // This query is connected to the root (it has no query parent), return true
+    if query.parent.is_none() {
+        return true;
+    }
 
-    let mut connected = false;
+    visited.insert(query.as_ptr());
 
     visit_waiters(query, |_, successor| {
         if connected_to_root(successor, visited) {
@@ -349,6 +350,28 @@ fn connected_to_root<'tcx>(
     }).is_some()
 }
 
+// Deterministically pick an query from a list
+#[cfg(parallel_queries)]
+fn pick_query<'a, 'tcx, T, F: Fn(&T) -> (Span, Lrc<QueryJob<'tcx>>)>(
+    tcx: TyCtxt<'_, 'tcx, '_>,
+    queries: &'a [T],
+    f: F
+) -> &'a T {
+    // Deterministically pick an entry point
+    // FIXME: Sort this instead
+    let mut hcx = tcx.create_stable_hashing_context();
+    queries.iter().min_by_key(|v| {
+        let (span, query) = f(v);
+        let mut stable_hasher = StableHasher::<u64>::new();
+        query.info.query.hash_stable(&mut hcx, &mut stable_hasher);
+        // Prefer entry points which have valid spans for nicer error messages
+        // We add an integer to the tuple ensuring that entry points
+        // with valid spans are picked first
+        let span_cmp = if span == DUMMY_SP { 1 } else { 0 };
+        (span_cmp, stable_hasher.finish())
+    }).unwrap()
+}
+
 /// Looks for query cycles starting from the last query in `jobs`.
 /// If a cycle is found, all queries in the cycle is removed from `jobs` and
 /// the function return true.
@@ -388,41 +411,52 @@ fn remove_cycle<'tcx>(
 
         // Find the queries in the cycle which are
         // connected to queries outside the cycle
-        let entry_points = stack.iter().filter_map(|query| {
-            // Mark all the other queries in the cycle as already visited
-            let mut visited = FxHashSet::from_iter(stack.iter().filter_map(|q| {
-                if q.1.as_ptr() != query.1.as_ptr() {
-                    Some(q.1.as_ptr())
-                } else {
+        let entry_points: Vec<_> = stack.iter().filter_map(|(span, query)| {
+            if query.parent.is_none() {
+                // This query is connected to the root (it has no query parent)
+                Some((*span, query.clone(), None))
+            } else {
+                let mut waiters = Vec::new();
+                // Find all the direct waiters who lead to the root
+                visit_waiters(query.clone(), |span, waiter| {
+                    // Mark all the other queries in the cycle as already visited
+                    let mut visited = FxHashSet::from_iter(stack.iter().map(|q| q.1.as_ptr()));
+
+                    if connected_to_root(waiter.clone(), &mut visited) {
+                        waiters.push((span, waiter));
+                    }
+
+                    None
+                });
+                if waiters.is_empty() {
                     None
+                } else {
+                    // Deterministically pick one of the waiters to show to the user
+                    let waiter = pick_query(tcx, &waiters, |s| s.clone()).clone();
+                    Some((*span, query.clone(), Some(waiter)))
                 }
-            }));
-
-            if connected_to_root(query.1.clone(), &mut visited) {
-                Some(query.1.clone())
-            } else {
-                None
             }
-        });
+        }).collect();
+
+        let entry_points: Vec<(Span, Lrc<QueryJob<'tcx>>, Option<(Span, Lrc<QueryJob<'tcx>>)>)>
+         = entry_points;
 
         // Deterministically pick an entry point
-        // FIXME: Sort this instead
-        let mut hcx = tcx.create_stable_hashing_context();
-        let entry_point = entry_points.min_by_key(|q| {
-            let mut stable_hasher = StableHasher::<u64>::new();
-            q.info.query.hash_stable(&mut hcx, &mut stable_hasher);
-            stable_hasher.finish()
-        }).unwrap().as_ptr();
+        let (_, entry_point, usage) = pick_query(tcx, &entry_points, |e| (e.0, e.1.clone()));
 
         // Shift the stack so that our entry point is first
-        let entry_point_pos = stack.iter().position(|(_, query)| query.as_ptr() == entry_point);
+        let entry_point_pos = stack.iter().position(|(_, query)| {
+            query.as_ptr() == entry_point.as_ptr()
+        });
         if let Some(pos) = entry_point_pos {
-            stack.rotate_right(pos);
+            stack.rotate_left(pos);
         }
 
+        let usage = usage.as_ref().map(|(span, query)| (*span, query.info.query.clone()));
+
         // Create the cycle error
         let mut error = CycleError {
-            usage: None,
+            usage,
             cycle: stack.iter().map(|&(s, ref q)| QueryInfo {
                 span: s,
                 query: q.info.query.clone(),

src/librustc_driver/test.rs

@@ -98,7 +98,7 @@ fn errors(msgs: &[&str]) -> (Box<dyn Emitter + sync::Send>, usize) {
 
 fn test_env<F>(source_string: &str, args: (Box<dyn Emitter + sync::Send>, usize), body: F)
 where
-    F: FnOnce(Env),
+    F: FnOnce(Env) + sync::Send,
 {
     syntax::with_globals(|| {
         let mut options = config::Options::default();

src/libsyntax_pos/lib.rs

@@ -611,7 +611,7 @@ impl serialize::UseSpecializedDecodable for Span {
     }
 }
 
-fn default_span_debug(span: Span, f: &mut fmt::Formatter) -> fmt::Result {
+pub fn default_span_debug(span: Span, f: &mut fmt::Formatter) -> fmt::Result {
     f.debug_struct("Span")
         .field("lo", &span.lo())
         .field("hi", &span.hi())