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

[asukaminato0721]

fix #1254

Added a suppression stack so binding code can execute unreachable branches without emitting type errors and exposed a helper to gate this behavior to filesystem/memory modules only, skipping builtins/__builtins__.

Updated the if binding logic to analyze statically false branches only when the new gate allows it, then discard their flow so control-flow facts stay unchanged while IDE features (like go-to-def) gain the needed Keys.

[grievejia]

This feels a bit complicated to me. I wonder if a better alternative would be to just keep all defs in the very beginning when they are calculated, recording their unreachable status so downstream type checking logic would avoid emitting type errors for those instead.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity for more than 2 weeks.

If you are still working on this this pull request, please add a comment or push new commits to keep it active. Otherwise, please unassign yourself and allow someone else to take over.

Thank you for your contributions!

[Copilot]

Pull request overview

This PR fixes issue #1254 where go-to-definition failed in unreachable code branches (e.g., if False:). The solution introduces a Reachability tracking system and error suppression mechanism to allow binding analysis of unreachable branches for IDE features while preventing type errors from being emitted.

Key changes:

• Added Reachability enum to track whether definitions are reachable based on static analysis
• Introduced error suppression stack to prevent type errors in unreachable branches
• Modified if-statement binding logic to analyze statically-false branches with error suppression
• Gated unreachable branch analysis to filesystem/memory modules only, excluding builtins

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pyrefly/lib/export/definitions.rs	Added Reachability enum and integrated it into Definition struct; updated if-statement analysis to track reachability
pyrefly/lib/export/exports.rs	Updated Definitions::new call to pass false for include_unreachable parameter
pyrefly/lib/binding/scope.rs	Integrated Reachability into StaticInfo and NameWriteInfo; added add_synthetic_definition method; modified scope initialization to accept include_unreachable_defs parameter
pyrefly/lib/binding/bindings.rs	Added error suppression mechanism via error_suppression_depth; added should_bind_unreachable_branches helper; modified bind_name to handle synthetic definitions for unreachable code
pyrefly/lib/binding/stmt.rs	Updated if-statement handling to bind names in unreachable branches with error suppression while excluding them from flow merges
pyrefly/lib/test/lsp/hover_type.rs	Updated test expectations to show that hover now works in unreachable branches
pyrefly/lib/test/lsp/definition.rs	Updated test expectations to show that go-to-definition now works in unreachable branches

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Synthetic definitions created for unreachable branches should be marked with Reachability::Unreachable instead of Reachability::Reachable. This function is specifically called when analyzing unreachable code for IDE metadata (as documented in the comment above), so these definitions should reflect their unreachable status.

Suggested change

	Reachability::Reachable,
	Reachability::Unreachable,

[Copilot]

This test should verify the reachability of foo is Unreachable to be consistent with the test above (test_unreachable_if_defs_respected), which checks both that the definition exists and validates its reachability status.

[Copilot]

The Reachability enum would benefit from a doc comment explaining what it represents and when code is considered unreachable (e.g., when it's in a statically-false branch like if False:).