Bug Fix 708 - Union Iterability Checker

[Tejas-Ketkar]

This PR tries to solve bug #708. The problem with the current implementation was that given a union type, solve.rs would iterate over each of the two types without considering whether both types were iterable. The fix tries to check if each option in the union is iterable, checking for explicit __iter__ or seeing if it behaves like a tuple or is a tuple. The original bug was added as a test to pattern_match.rs.

[migeed-z]

can we add is_iterable be added to types.rs and invoked from there instead?

[Tejas-Ketkar]

Moving the function out is a good idea, but I think placing the function types.rs won't work since pyrefly_types is in a different crate. From the way it's defined right now we'd end up with circular dependencies?

[stroxler]

I think that's correct, it needs access to self.

Let's just pull it into a helper method?

We probably should make an AnswersSolver module for "type utilities" that require access to a solver and therefore cannot be in types.rs at some point.

[stroxler]

This looks great, thanks for the contribution!

I think it would be nice to extract the helper method, after that I could work on getting it merged.

[Tejas-Ketkar]

Done! Let me know if there's any other changes needed.

[facebook-github-bot]

@stroxler has imported this pull request. If you are a Meta employee, you can view this in D78620634.

[stroxler]

LGTM, I'll aim to get it merged soon

[ndmitchell]

I think this is wrong, as given the example:

from typing import reveal_type


def f(x: int | tuple[int, int]):
    for y in x:
        reveal_type(y)

Before this diff we get an error Type int is not iterable, and after we succeed. My best guess is that for pattern matching, where we are asking "does this type match", we should be making a suitable check. But for other uses of iterate, we should require it to match. But I don't know enough about how pattern matching works to know what to do there.

[stroxler]

Oh good point - we probably need something like this logic, but only used in match.

The idea is that if we see a tuple form in a match, we should take all the iterable cases of a union as potential hits (so if we have x: float | tuple[int, int] | list[bytes] then matching x against a, b should give us tuple[int, int] and list[bytes] as the potential union cases that match, but the float is eliminated)

It might actually need to be done as a narrowing operation

cc @yangdanny97 who has the most context on matching

[yangdanny97]

@stroxler We do have narrowing that checks sequence length, but it doesn't exclude types that can't be sequences. I don't think we can modify it directly because it's used for stuff like if len(x) == 2, so maybe we could introduce a new NarrowOp and generate a compound narrow op for match statements.

[yangdanny97]

@Tejas-Ketkar there was a bit of back and forth discussion on this PR earlier, but it sounds like the next step is to update the PR it s.t. the checker only works when pattern matching.

[Tejas-Ketkar]

Thanks, I'll look into making the fix.