facebookincubator / cinder

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

type aliases no longer work? #116

Open bennn opened 1 year ago

bennn commented 1 year ago

On cinder/3.10, fannkuch_static_lib.py and nqueens_static_lib.py give errors like this:

  File "fannkuch_static_lib.py", line 29
    count[r - 1] = r
compiler.errors.TypedSyntaxError: type mismatch: int64 cannot be assigned to dynamic

The array count is defined using a type alias:

ArrayI64 = Array[int64]
....
count: ArrayI64 = ArrayI64(range(1, nb + 1))

Changing the definition to use the type directly removes the error:

count: Array[int64] = ArrayI64(range(1, nb + 1))

That's surprising! Did something break with type aliases?

cc @vivaan2006 who helped discover this

DinoV commented 1 year ago

Thanks for the report... It looks like this was caused by 6ce212a0464e747f1301c3101fba43ef5f3abadc. That prevents code like:

import __static__

class C:
    def __init__(self):
        self.x = 42

def f():
    global a
    a = 42
a = C()
f()

print(a.x)

From crashing, but we'll need to come up with a better solution here and retain the implicit type information while disallowing the conflicting assignment. We do report the invalid assignment if def f comes after the assignment to a so it's probably just adjusting how our passes run or adding a new pass.

DinoV commented 1 year ago

We've come up with some other interesting examples:

class A: pass
class B(A): pass

def f():
    global x
    x = A()  # this assignment should be ok

def g(arg: B): pass

x: A = B()

f()

g(x)  # should be a TypedSyntaxError, with or without `f()` (or even the definition of `f`)

And we've looked at what Pyre does here as well. Both Pyre and static Python have a notion of inferred type as well as the declared type. In some annotated cases like "x = 42" it seems that there ends up being an inferred type of Literal[42] and a declared type of int.

Currently we only end up with an inferred type, and then we're turning that into dynamic. We probably need to more closely mimic what Pyre is doing for getting a declared type in these situations, and then drop the inferred type. And we need to understand more when we should treat the inferred type as declared.

We also may be able to do this in limited scenarios, e.g. where there are no references to global in the module, where we know the value won't change (because the module is known to be immutable).

And we may need to add an extra pass in to accomodate this as well.

Unfortunately this will take a little while to get to!