facebookincubator / cinder

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

A test in `test_static` should catch `AssertionError` rather than `TypeError` #65

Closed LuKuangChen closed 2 years ago

LuKuangChen commented 2 years ago

2022-01-07

foo("a") raises an AssertionError, but the test expects a TypeError.

def test_assert_narrowing_optimized(self):
    # We ensure that the code without the assert would work in the runtime.
    codestr = """
    def foo(x: int | str) -> object:
        assert isinstance(x, int)
        return x
    """

    with self.in_module(codestr, optimize=1) as mod:
        foo = mod.foo
        self.assertEqual(foo(1), 1)
        with self.assertRaises(TypeError):
            foo("a")

There is a test that looks like a corrected version of this one.

def test_assert_narrowing_debug(self):
    codestr = """
    def foo(x: int | str) -> int:
        assert isinstance(x, int)
        return x + 1
    """
    with self.in_module(codestr) as mod:
        foo = mod.foo
        self.assertEqual(foo(1), 2)
        with self.assertRaises(AssertionError):
            foo("a")
carljm commented 2 years ago

Assertions in Python are stripped by the compiler if the optimization level is 1+. If we refine a type in our static compiler based on an isinstance assert, we transform that assert into a CAST at runtime. If the type is wrong, a CAST raises TypeError. So these two tests are both passing, and correct. With optimization level 1, the assertion is stripped but replaced by a CAST and so TypeError is raised. With optimization level 0, AssertionError is raised.

shriram commented 2 years ago

I'm curious, shouldn't the test then be predicated by the optimization level? You're saying there are really two different tests here, one for level 0 and one for level 1+.

carljm commented 2 years ago

The one test passes optimize=1 to in_module which compiles the code str with opt level 1; the other compiles with the default opt level 0.

Are you seeing the _optimized one fail when you run it? It passes here and in GitHub CI.

bennn commented 2 years ago

Ah, that explains it. We'd been running the codestr by themselves and hadn't noticed optimize=1 was setting up a context.