check_boolean_op: clean up semanal/type-based reachability

[ikonst]

ExpressionChecker.check_boolean_op supports two potentially conflicting ways of determining whether an expression is always true/false: name-based checks from semantic analysis (e.g. name in options.always_true) and type narrowing in TypeChecker.find_isinstance_check (*). The two ways can be set to conflict, e.g.

# flags: --always-true COMPILE_TIME_TRUE
COMPILE_TIME_TRUE: None

In the above case, COMPILE_TIME_TRUE would be always true per semanal and always false per find_isinstance_check, which results in an assertion failure here:

mypy/mypy/checkexpr.py

Line 2828 in 5eb4de4

assert left_map is not None # find_isinstance_check guarantees this

and here:

mypy/mypy/checkexpr.py

Line 2832 in 5eb4de4

assert right_map is not None # find_isinstance_check guarantees this

This change reconciles the two, by prioritizing the semanal flags (e.right_always, e.right_unreachable) over type-narrowing (if_map, else_map). Then we explicitly avoid flagging reachability errors when the semanal flags are set, since they're obviously used intentionally.

(*) which seems to do slightly more than isinstance checks

[ikonst]

For always-true/false cases, find_isinstance_check returns a {}, None or a None, {} pair. We do the same here, but without even calling find_isinstance_check.

[ikonst]

Can safely move it down here since nothing uses restricted_left_type and result_is_left until this point.

[ikonst]

@hauntsaninja @JukkaL want to take a look?

[ikonst]

@hauntsaninja @JukkaL gentle ping

[hauntsaninja]

Sorry for the delay and thanks for this well explained PR that fixes a crash!
However, the reveal types in your test cases aren't quite what I'd expect. E.g., in your first test case I'd expect reveal_type(b) to be bool (Literal[True] and None shouldn't really be appearing anywhere, since mypy should always think the COMPILE_TIME_* are True or False).
I think we should maybe set left_type and right_type if e.right_always or e.right_unreachable? Also happy to merge as is!

[ikonst]

However, the reveal types in your test cases aren't quite what I'd expect. E.g., in your first test case I'd expect reveal_type(b) to be bool (Literal[True] and None shouldn't really be appearing anywhere, since mypy should always think the COMPILE_TIME_* are True or False).

You're saying you'd expect:

 indeterminate: bool
 COMPILE_TIME_FALSE: Literal[True]
 b = indeterminate or COMPILE_TIME_FALSE
-reveal_type(b)  # N: Revealed type is "Union[builtins.bool, Literal[True]]"
+reveal_type(b)  # N: Revealed type is "builtins.bool"

but we don't know if indeterminate is truthy; if it is, the expression's type is LHS type, else, it's the RHS type (never mind if RHS is truthy or not).

Or in this case you'd expect:

 indeterminate: bool
 COMPILE_TIME_TRUE: None
 b = indeterminate or COMPILE_TIME_TRUE
-reveal_type(b)  # N: Revealed type is "Union[builtins.bool, None]"
+reveal_type(b)  # N: Revealed type is "builtins.bool"

Same logic. COMPILE_TIME_TRUE is typed None, never mind if it's truthy or not. The expression should have either LHS or RHS type -- since LHS is indeterminate, a union of both.

[hauntsaninja]

mypy --help says:

  --always-true NAME        Additional variable to be considered True (may be repeated)
  --always-false NAME       Additional variable to be considered False (may be repeated)

which I interpreted as the variable should be interpreted as that literal boolean value, not just something that's truthy or falsey (TYPE_CHECKING and MYPY are often used to make bold lies to the type checker, so just wiping out the type seemed like a valid interpretation).
But https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-always-true reads much more ambiguously, it seems you didn't read it that way, and changing it would require more work for something that's kind of niche. So like I said, happy to merge, and thank you again!

[ikonst]

the variable should be interpreted as that literal boolean value, not just something that's truthy or falsey

That's what I'd assume too, and maybe that'd be a better way to implement it. As for now, those options manifest in the OpExpr. right_always and OpExpr.right_unreachable (a semanal artifact) which are booleans used purely for this feature. I've added comments to document that:

-    # Is the right side going to be evaluated every time?
+    # Per static analysis only: Is the right side going to be evaluated every time?
     right_always = False
-    # Is the right side unreachable?
+    # Per static analysis only: Is the right side unreachable?
     right_unreachable = False