Suppress second error message with := and [truthy-bool]

[sobolevn]

Closes #15685
CC @ikonst

[ikonst]

Bringing it down to 1 message seems positive. I'm just curious how did we end up thinking a walrus's expression is boolean context in the first place...

[ikonst]

Let me refresh my memory on how this all works.

[ikonst]

I was trying to find a way in which we wouldn't need to add is_walrus_value to find_isinstance_check since it already does a lot of things and it's probably better to localize this concern to where walruses are handled.

What's interesting about the elif isinstance(node, AssignmentExpr) branch is that it gets type-maps for the node.target and the node.value. Both of them have the same type at that point, and while node.value can contain some interesting stuff (e.g. isinstance checks), node.target is bound to be pretty boring. Wouldn't it make sense to descend just down node.value?

cc @hauntsaninja who implemented narrowing for walruses (#8258, #11202)

[ikonst]

Ahh, now I get it, because for target := value we need to narrow both target and value. At the same time, though, target doesn't need "real" narrowing, basically just

if (t := if_map.get(node.value)) is not None:
    if_map[node.target] = t
if (t := else_map.get(node.value)) is not None:
    else_map[node.target] = t

[ikonst]

... except that in

s: str | None

if a := s is not None:
  ...

in the 'if' clause:

• a is narrowed to Literal[True]
• s is narrowed to str
• s is not None is not narrowed to Literal[True] :(

[hauntsaninja]

This feels a little icky to me, don't know that the idiom is common enough for this kind of special casing to be worth it.

It's actually generally true that narrowing target is more useful than narrowing from value, because little point in using a walrus in a condition if you're not going to use the narrowed variable. But both are useful, see the test cases in #11202. The last comment you mention is the same as testWalrusConditionalTypeCheck test case.

[sobolevn]

I've added one more test case where we have a problem in the value part. It seems to work correctly.

[sobolevn]

I was trying to find a way in which we wouldn't need to add is_walrus_value to find_isinstance_check since it already does a lot of things and it's probably better to localize this concern to where walruses are handled.

We can create a global state for the Checker with something like with self.in_walrus_value(stmt): ...

[ikonst]

My hunch is that we're doing two things not ideally. For assignment expr AKA walrus, we descend down both nodes (like in a binary op where they're independent) but really should descend down the value node and use the value's if/else types as the target's. Alas it doesn't work when I try it naively, but it's maybe due to a bug?

Otherwise maybe consider renaming is_walrus_value to is_boolean_context (with inverse semantic) and make it False when descending down node.value since it's effectively the only case when find_isinstance_check is called not in boolean context?

[sobolevn]

For assignment expr AKA walrus, we descend down both nodes (like in a binary op where they're independent) but really should descend down the value node and use the value's if/else types as the target's.

This is clearly out of scope for now. I think that opening a new issue might help :)

Otherwise maybe consider renaming is_walrus_value to is_boolean_context (with inverse semantic) and make it False when descending down node.value since it's effectively the only case when find_isinstance_check is called not in boolean context?

Good point, addressed.

[ikonst]

Can't think of a better way to do it, and this does make things better

[sobolevn]

CC @JukkaL @ilevkivskyi

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅