fix Go-to-def fails on unreachable branches #1254

pyrefly/lib/binding/bindings.rs

@@ -14,6 +14,7 @@ use std::sync::Arc;
 use dupe::Dupe;
 use pyrefly_python::ast::Ast;
 use pyrefly_python::module_name::ModuleName;
+use pyrefly_python::module_path::ModulePathDetails;
 use pyrefly_python::nesting_context::NestingContext;
 use pyrefly_python::short_identifier::ShortIdentifier;
 use pyrefly_python::sys_info::SysInfo;
@@ -198,6 +199,7 @@ pub struct BindingsBuilder<'a> {
     pub has_docstring: bool,
     pub scopes: Scopes,
     table: BindingTable,
+    error_suppression_depth: usize,
     pub untyped_def_behavior: UntypedDefBehavior,
     unused_parameters: Vec<UnusedParameter>,
     unused_imports: Vec<UnusedImport>,
@@ -405,6 +407,7 @@ impl Bindings {
             has_docstring: Ast::has_docstring(&x),
             scopes: Scopes::module(x.range, enable_trace),
             table: Default::default(),
+            error_suppression_depth: 0,
             untyped_def_behavior,
             unused_parameters: Vec::new(),
             unused_imports: Vec::new(),
@@ -748,6 +751,7 @@ impl<'a> BindingsBuilder<'a> {
     }
 
     pub fn init_static_scope(&mut self, x: &[Stmt], top_level: bool) {
+        let include_unreachable_defs = self.should_bind_unreachable_branches();
         self.scopes.init_current_static(
             x,
             &self.module_info,
@@ -760,6 +764,7 @@ impl<'a> BindingsBuilder<'a> {
                     .0
                     .insert(KeyAnnotation::Annotation(x))
             },
+            include_unreachable_defs,
         );
     }
 
@@ -769,6 +774,29 @@ impl<'a> BindingsBuilder<'a> {
         }
     }
 
+    pub fn with_error_suppression<R>(
+        &mut self,
+        f: impl FnOnce(&mut BindingsBuilder<'a>) -> R,
+    ) -> R {
+        self.error_suppression_depth += 1;
+        let result = f(self);
+        self.error_suppression_depth -= 1;
+        result
+    }
+
+    #[inline]
+    fn errors_suppressed(&self) -> bool {
+        self.error_suppression_depth > 0
+    }
+
+    pub(crate) fn should_bind_unreachable_branches(&self) -> bool {
+        matches!(
+            self.module_info.path().details(),
+            ModulePathDetails::FileSystem(_) | ModulePathDetails::Memory(_)
+        ) && self.module_info.name() != ModuleName::builtins()
+            && self.module_info.name() != ModuleName::extra_builtins()
+    }
+
     fn inject_globals(&mut self) {
         for global in ImplicitGlobal::implicit_globals(self.has_docstring) {
             let key = Key::ImplicitGlobal(global.name().clone());
@@ -891,10 +919,16 @@ impl<'a> BindingsBuilder<'a> {
     }
 
     pub fn error(&self, range: TextRange, info: ErrorInfo, msg: String) {
+        if self.errors_suppressed() {
+            return;
+        }
         self.errors.add(range, info, vec1![msg]);
     }
 
     pub fn error_multiline(&self, range: TextRange, info: ErrorInfo, msg: Vec1<String>) {
+        if self.errors_suppressed() {
+            return;
+        }
         self.errors.add(range, info, msg);
     }
 
@@ -1078,19 +1112,38 @@ impl<'a> BindingsBuilder<'a> {
         idx: Idx<Key>,
         style: FlowStyle,
     ) -> Option<Idx<KeyAnnotation>> {
-        let name = Hashed::new(name);
-        let write_info = self
-            .scopes
-            .define_in_current_flow(name, idx, style)
-            .unwrap_or_else(|| {
-                panic!(
-                    "Name `{name}` not found in static scope of module `{}`.",
-                    self.module_info.name(),
-                )
-            });
+        let mut hashed_name = Hashed::new(name);
+        let allow_unreachable_defs =
+            self.errors_suppressed() && self.should_bind_unreachable_branches();
+        let mut write_info = self.scopes.define_in_current_flow(
+            hashed_name,
+            idx,
+            style.clone(),
+            allow_unreachable_defs,
+        );
+        if write_info.is_none() && allow_unreachable_defs {
+            let key_range = self.table.types.0.idx_to_key(idx).range();
+            self.scopes.add_synthetic_definition(name, key_range);
+            hashed_name = Hashed::new(name);
+            write_info = self.scopes.define_in_current_flow(
+                hashed_name,
+                idx,
+                style.clone(),
+                allow_unreachable_defs,
+            );
+        }
+        let write_info = write_info.unwrap_or_else(|| {
+            panic!(
+                "Name `{name}` not found in static scope of module `{}`.",
+                self.module_info.name(),
+            )
+        });
+        if !write_info.reachability.is_reachable() {
+            debug_assert!(allow_unreachable_defs);
+        }
         if let Some(range) = write_info.anywhere_range {
             self.table
-                .record_bind_in_anywhere(name.into_key().clone(), range, idx);
+                .record_bind_in_anywhere(hashed_name.into_key().clone(), range, idx);
         }
         write_info.annotation
     }

pyrefly/lib/binding/scope.rs

@@ -69,6 +69,7 @@ use crate::export::definitions::Definition;
 use crate::export::definitions::DefinitionStyle;
 use crate::export::definitions::Definitions;
 use crate::export::definitions::MutableCaptureKind;
+use crate::export::definitions::Reachability;
 use crate::export::exports::ExportLocation;
 use crate::export::exports::LookupExport;
 use crate::export::special::SpecialExport;
@@ -115,6 +116,8 @@ pub struct NameWriteInfo {
     /// If this name only has one assignment, we will skip the `Anywhere` as
     /// an optimization, and this field will be `None`.
     pub anywhere_range: Option<TextRange>,
+    /// Whether the definition is reachable for the current sys_info configuration.
+    pub reachability: Reachability,
 }
 
 #[derive(Clone, Debug)]
@@ -172,6 +175,7 @@ struct Static(SmallMap<Name, StaticInfo>);
 struct StaticInfo {
     range: TextRange,
     style: StaticStyle,
+    reachability: Reachability,
 }
 
 #[derive(Clone, Debug)]
@@ -249,7 +253,7 @@ impl StaticStyle {
 
     fn of_definition(
         name: Hashed<&Name>,
-        definition: Definition,
+        definition: &Definition,
         scopes: Option<&Scopes>,
         get_annotation_idx: &mut impl FnMut(ShortIdentifier) -> Idx<KeyAnnotation>,
     ) -> Self {
@@ -315,15 +319,26 @@ impl StaticInfo {
             } else {
                 None
             },
+            reachability: self.reachability,
         }
     }
 }
 
 impl Static {
-    fn upsert(&mut self, name: Hashed<Name>, range: TextRange, style: StaticStyle) {
+    fn upsert(
+        &mut self,
+        name: Hashed<Name>,
+        range: TextRange,
+        style: StaticStyle,
+        reachability: Reachability,
+    ) {
         match self.0.entry_hashed(name) {
             Entry::Vacant(e) => {
-                e.insert(StaticInfo { range, style });
+                e.insert(StaticInfo {
+                    range,
+                    style,
+                    reachability,
+                });
             }
             Entry::Occupied(mut e) => {
                 let found = e.get_mut();
@@ -360,6 +375,7 @@ impl Static {
                         }
                     }
                 }
+                found.reachability = found.reachability.combine(reachability);
             }
         }
     }
@@ -373,12 +389,14 @@ impl Static {
         sys_info: &SysInfo,
         get_annotation_idx: &mut impl FnMut(ShortIdentifier) -> Idx<KeyAnnotation>,
         scopes: Option<&Scopes>,
+        include_unreachable_defs: bool,
     ) {
         let mut d = Definitions::new(
             x,
             module_info.name(),
             module_info.path().is_init(),
             sys_info,
+            include_unreachable_defs,
         );
         if top_level {
             if module_info.name() != ModuleName::builtins() {
@@ -404,13 +422,18 @@ impl Static {
             // same name in this scope.
             let range = definition.range;
             let style =
-                StaticStyle::of_definition(name.as_ref(), definition, scopes, get_annotation_idx);
-            self.upsert(name, range, style);
+                StaticStyle::of_definition(name.as_ref(), &definition, scopes, get_annotation_idx);
+            self.upsert(name, range, style, definition.reachability);
         }
         for (range, wildcard) in wildcards {
             for name in wildcard.iter_hashed() {
                 // TODO: semantics of import * and global var with same name
-                self.upsert(name.cloned(), range, StaticStyle::MergeableImport)
+                self.upsert(
+                    name.cloned(),
+                    range,
+                    StaticStyle::MergeableImport,
+                    Reachability::Reachable,
+                )
             }
         }
     }
@@ -421,6 +444,7 @@ impl Static {
                 Hashed::new(name.id.clone()),
                 name.range,
                 StaticStyle::SingleDef(None),
+                Reachability::Reachable,
             )
         };
         Ast::expr_lvalue(x, &mut add);
@@ -1263,6 +1287,7 @@ impl Scopes {
         lookup: &dyn LookupExport,
         sys_info: &SysInfo,
         get_annotation_idx: &mut impl FnMut(ShortIdentifier) -> Idx<KeyAnnotation>,
+        include_unreachable_defs: bool,
     ) {
         let mut initialize = |scope: &mut Scope, myself: Option<&Self>| {
             scope.stat.stmts(
@@ -1273,6 +1298,7 @@ impl Scopes {
                 sys_info,
                 get_annotation_idx,
                 myself,
+                include_unreachable_defs,
             );
             // Presize the flow, as its likely to need as much space as static
             scope.flow.info.reserve(scope.stat.0.capacity());
@@ -1566,6 +1592,7 @@ impl Scopes {
         name: Hashed<&Name>,
         idx: Idx<Key>,
         style: FlowStyle,
+        allow_unreachable: bool,
     ) -> Option<NameWriteInfo> {
         let in_loop = self.loop_depth() != 0;
         match self.current_mut().flow.info.entry_hashed(name.cloned()) {
@@ -1577,6 +1604,9 @@ impl Scopes {
             }
         }
         let static_info = self.current().stat.0.get_hashed(name)?;
+        if !allow_unreachable && !static_info.reachability.is_reachable() {
+            return None;
+        }
         Some(static_info.as_name_write_info())
     }
 
@@ -1708,6 +1738,7 @@ impl Scopes {
             Hashed::new(name.id.clone()),
             name.range,
             StaticStyle::SingleDef(ann),
+            Reachability::Reachable,
         )
     }
 
@@ -1805,6 +1836,7 @@ impl Scopes {
             Hashed::new(name.id.clone()),
             name.range,
             StaticStyle::PossibleLegacyTParam,
+            Reachability::Reachable,
         )
     }
 
@@ -1821,6 +1853,21 @@ impl Scopes {
         self.current_mut().stat.expr_lvalue(x);
     }
 
+    /// Synthesize a static definition entry for `name` in the current scope if it
+    /// is missing. Used when we analyze unreachable code for IDE metadata.
+    pub fn add_synthetic_definition(&mut self, name: &Name, range: TextRange) {
+        let hashed_ref = Hashed::new(name);
+        if self.current().stat.0.get_hashed(hashed_ref).is_some() {
+            return;
+        }
+        self.current_mut().stat.upsert(
+            Hashed::new(name.clone()),
+            range,
+            StaticStyle::SingleDef(None),
+            Reachability::Reachable,
+        );
+    }
+
     /// Add a loop exit point to the current innermost loop with the current flow.
     ///
     /// Return a bool indicating whether we were in a loop (if we weren't, we do nothing).
@@ -2310,6 +2357,9 @@ impl ScopeTrace {
                     {
                         definition = Some(key);
                     }
+                    Key::Anywhere(_, range) if range.contains_inclusive(position) => {
+                        definition = Some(key);
+                    }
                     _ => {}
                 }
             });

pyrefly/lib/binding/stmt.rs

@@ -717,14 +717,22 @@ impl<'a> BindingsBuilder<'a> {
                         Some(x) => self.sys_info.evaluate_bool(x),
                     };
                     self.ensure_expr_opt(test.as_mut(), &mut Usage::Narrowing(None));
-                    let new_narrow_ops = if this_branch_chosen == Some(false) {
-                        // Skip the body in this case - it typically means a check (e.g. a sys version,
-                        // platform, or TYPE_CHECKING check) where the body is not statically analyzable.
+                    let new_narrow_ops = NarrowOps::from_expr(self, test.as_ref());
+                    if this_branch_chosen == Some(false) {
+                        // Skip contributing to flow merges, but still bind names so IDE features work.
+                        if self.should_bind_unreachable_branches() {
+                            self.bind_narrow_ops(
+                                &new_narrow_ops,
+                                NarrowUseLocation::Span(range),
+                                &Usage::Narrowing(None),
+                            );
+                            self.with_error_suppression(|builder| {
+                                builder.stmts(body, parent);
+                            });
+                        }
                         self.abandon_branch();
                         continue;
-                    } else {
-                        NarrowOps::from_expr(self, test.as_ref())
-                    };
+                    }
                     if let Some(test_expr) = test {
                         // Typecheck the test condition during solving.
                         self.insert_binding(