Go-to-def fails on unreachable branches

[yangdanny97]

Describe the Bug

If we incorrectly infer the type of something, our control flow modeling will incorrectly treat branches as unreachable, and go-to-def does not work inside those branches.

The root cause of the unreachability in the sandbox example is https://github.com/facebook/pyrefly/issues/1159, but I think we would want go-to-def to work even if the branch is not reachable.

Sandbox Link

https://pyrefly.org/sandbox/?project=N4IgZglgNgpgziAXKOBDAdgEwEYHsAeAdAA4CeS4ATrgLYAEALqcROgOZ0Q3G6UN2UYANxiooAfSbEYAHXRyAxlFRw4dACrwGiOXT11MMMHXHjWEBqYAUcGFDABKHen2vOxvLihWADE91ugbb2hKh0ALx0AHK46LIugXR2ts6JbsFgoRF0AIx6AMR0bLgAtAylhsYA7rwA1moAFjCCAYEQxhlZEGrouPwxcalp%2BmGRnWGFxWUVRnRgqNCNzTAgADQgZIJgUKSE5TRQFIUACqRbO3RoWHj4dAqxkGwArpSoDBCxhHKFAMowMHQGgwGMQ4IgAPTgzZGHaEXhscEwdDgzC4BRwcH3dCPF5vD7Iua8OioIQLZTYWB3B4QZ6vd6xOi4Yj09BwL7oMgMBqxEoiShwfHZGQgADMhByACZhXIQABfdaoBTvEQAMWgK0QICuOAIJHIsqAA

(Only applicable for extension issues) IDE Information

No response

[stroxler]

I think we only do this on if/else at the moment but not most other cases (e.g. pattern matching keeps going)

[stroxler]

I can probably work on this in November, it is adjacent to the flow-merging stuff I was recently implementing and also to the flow-merging changes needed for #361

But I likely won't get to it until after planning (so 3 weeks from now minimum)

[kinto0]

i added a test for this here

[stroxler]

Hmm. This is actually pretty tricky to do the way I was intending, which was just to stop skipping unreachable if branches.

It is out of the question to do it that way as stated - we absolutely have to skip analysis of "platform level" unreachable code (anything looking at TYPE_CHECKING, sys.version_info, or sys.platform) because the semantics of python mean we break the universe if we analyze those branches (the standard library, for example, makes no sense).

So then the question is, do we want to do something more complex:

• I know how to implement a solution where "platform bools" are treated specially, but it requires distinguishing between platform bools and other statically-known bools, which makes a mess of the sys_info module. I can do it, but there's a complexity tradeoff
• Or, we can use some other solution, which seems to be what pull #1523 is proposing ... it looks like @grievejia already started looking at that, so I'll leave review to Jia unless requested

The main thing that leaves me uncertain the best path forward is that when this issue was filed, the IDE broke silently but now thanks to @asukaminato0721's work we have greyed-out blocks of unreachable code, and I'm not convinced this is a huge problem anymore.

I'm particularly not convinced that it rises to the level of needing to be under the V1 release, I think the greying-out feature was essentially a partial substitute for implementing this.

Thoughts @asukaminato0721, @yangdanny97?

[yangdanny97]

I know how to implement a solution where "platform bools" are treated specially, but it requires distinguishing between platform bools and other statically-known bools, which makes a mess of the sys_info module. I can do it, but there's a complexity tradeoff

I don't see any issue with treating platform bools specially, but I guess it depends on what "mess of the sys_info module" means.

If we're not falsely marking a lot of stuff as unreachable I think it's probably fine to de-prioritize, but if there are false positives here then I think this is necessary for v1