From 216bef226e41d36a9b82f14579143c2e6ca538f7 Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Wed, 20 Mar 2024 11:42:43 +0100
Subject: [PATCH] extend doc comment for reachability set computation

---
 compiler/rustc_passes/src/reachable.rs | 40 +++++++++++++++++---------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/compiler/rustc_passes/src/reachable.rs b/compiler/rustc_passes/src/reachable.rs
index b7135de08ba84..6aeed6c5997bf 100644
--- a/compiler/rustc_passes/src/reachable.rs
+++ b/compiler/rustc_passes/src/reachable.rs
@@ -1,9 +1,15 @@
-// Finds items that are externally reachable, to determine which items
-// need to have their metadata (and possibly their AST) serialized.
-// All items that can be referred to through an exported name are
-// reachable, and when a reachable thing is inline or generic, it
-// makes all other generics or inline functions that it references
-// reachable as well.
+//! Finds local items that are externally reachable, to determine which items
+//! need to have their metadata (and possibly their AST) serialized.
+//!
+//! This set is *not* transitively closed, i.e., in general the set only contains definitions that
+//! can be reached *directly* via an exported name, not private functions that can only be reached
+//! transitively.
+//!
+//! However, there's a catch: if an item is generic or cross-crate inlinable, then it will have its
+//! code generated by some downstream crate. Now if that item calls private monomorphic
+//! non-cross-crate-inlinable items, then those can be reached by the code generated by the
+//! downstream create! Therefore, when a reachable thing is cross-crate inlinable or generic, it
+//! makes all other functions that it references reachable as well.
 
 use hir::def_id::LocalDefIdSet;
 use rustc_data_structures::stack::ensure_sufficient_stack;
@@ -56,10 +62,15 @@ impl<'tcx> Visitor<'tcx> for ReachableContext<'tcx> {
             hir::ExprKind::Path(ref qpath) => {
                 Some(self.typeck_results().qpath_res(qpath, expr.hir_id))
             }
-            hir::ExprKind::MethodCall(..) => self
-                .typeck_results()
-                .type_dependent_def(expr.hir_id)
-                .map(|(kind, def_id)| Res::Def(kind, def_id)),
+            hir::ExprKind::MethodCall(..) => {
+                // If this is a method call on a generic type, we might not be able to find the
+                // callee. That's why `reachable_set` also adds all potential callees for such
+                // calls, i.e. all trait impl items, to the reachable set. So here we only worry
+                // about the calls we can identify.
+                self.typeck_results()
+                    .type_dependent_def(expr.hir_id)
+                    .map(|(kind, def_id)| Res::Def(kind, def_id))
+            }
             hir::ExprKind::Closure(&hir::Closure { def_id, .. }) => {
                 self.reachable_symbols.insert(def_id);
                 None
@@ -394,6 +405,7 @@ fn has_custom_linkage(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
         || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED_LINKER)
 }
 
+/// See module-level doc comment above.
 fn reachable_set(tcx: TyCtxt<'_>, (): ()) -> LocalDefIdSet {
     let effective_visibilities = &tcx.effective_visibilities(());
 
@@ -427,10 +439,10 @@ fn reachable_set(tcx: TyCtxt<'_>, (): ()) -> LocalDefIdSet {
         }
     }
     {
-        // Some methods from non-exported (completely private) trait impls still have to be
-        // reachable if they are called from inlinable code. Generally, it's not known until
-        // monomorphization if a specific trait impl item can be reachable or not. So, we
-        // conservatively mark all of them as reachable.
+        // As explained above, we have to mark all functions called from reachable
+        // `item_might_be_inlined` items as reachable. The issue is, when those functions are
+        // generic and call a trait method, we have no idea where that call goes! So, we
+        // conservatively mark all trait impl items as reachable.
         // FIXME: One possible strategy for pruning the reachable set is to avoid marking impl
         // items of non-exported traits (or maybe all local traits?) unless their respective
         // trait items are used from inlinable code through method call syntax or UFCS, or their