facebookincubator / cinder

Cinder is Meta's internal performance-oriented production version of CPython.
https://trycinder.com
Other
3.53k stars 121 forks source link

Cannot use field twice after an `is not None` check? #145

Open bennn opened 1 month ago

bennn commented 1 month ago

On Python 3.10.5+cinder, Cinder commit 1aeff3259dc

Example program:

class Node:
    def __init__(self) -> None:
        self.wins: int = 0
        self.losses: int = 0
        self.parent: None | Node = None

    def score1(self) -> int:
        n = 0
        if self.parent is not None:
            n += self.parent.wins
        if self.parent is not None:
            n += self.parent.losses
        return n

    def score2(self) -> int:
        n = 0
        if self.parent is not None:
            n += self.parent.losses
            n += self.parent.wins # type error !?
        return n

Expected no type error.

But got a type error inside score2 accessing self.parent.wins:

  File "foo.py", line 20
    n += self.parent.wins
        ^
cinderx.compiler.errors.TypedSyntaxError: Optional[__main__.Node]: 'NoneType' object has no attribute 'wins'

If you swap that line and the previous one, the error is on self.parent.losses instead!

bennn commented 1 month ago

Also, it's strange that the test must be self.parent is not None. I'd rather write self.parent by itself.

carljm commented 1 month ago

I think this is Static Python reflecting the idea that losses could be a dynamic property which could execute arbitrary code. So once you've accessed it, any guarantees from the narrowing go out the window. That explains the seemingly odd "second access is an error" behavior.

It might be possible for SP to know that this isn't actually the case for an attribute of a given static type, though, since we can see the definition of the type. The trick here is subclassing, though -- it depends what our rules are on subclasses overriding a normal attribute with a dynamic property.

Respecting if x (and not just if x is not None) as eliminating the possibility of None in an optional type should definitely be possible, I think it just hasn't been implemented.

bennn commented 1 month ago

ah, that makes sense, thanks!

bennn commented 4 weeks ago

This seems like a related issue: if self.parent is optional, then I can't call a method on it (after a guard) because SP thinks I'm passing an optional value as the self argument to the method call:

import __static__
from typing import Optional

class Node:
  def __init__(self, val: int):
    self.parent: Optional[Node] = None
    self.val: int = val

  def add(self, pp: Node):
    self.parent = pp

  def getval(self):
    return self.val

  def pval(self):
    if self.parent is not None:
      self.parent.getval() # ERROR
      # it's fine if I define `pp = self.parent` and do the call on `pp`

def main():
  n0: Node = Node(0)
  n1: Node = Node(1)
  n0.add(n1)
  n0.pval()
  return

main()
 ....
    raise exception
  File "test.py", line 18
    self.parent.getval()
cinderx.compiler.errors.TypedSyntaxError: type mismatch: Optional[__main__.Node] received for positional arg 'self', expected __main__.Node