Added conformance tests for new "Exceptions" chapter. #1724
Conversation
| with NoSuppress1(): | ||
| raise ValueError("This exception is not suppressed") | ||
|
|
||
| return 1 # OK |
There was a problem hiding this comment.
I feel a type checker that recognizes that this code is unreachable could still reasonably show an error here, since the return type is definitely inconsistent with the annotation. Therefore, I'd prefer to use a different mechanism to test the desired semantics here.
This seems to work well:
from typing import Any, Literal, assert_type
class CMBase:
def __enter__(self) -> None:
pass
class Suppress1(CMBase):
def __exit__(self, exc_type, exc_value, traceback) -> bool:
return True
class Suppress2(CMBase):
def __exit__(self, exc_type, exc_value, traceback) -> Literal[True]:
return True
class NoSuppress1(CMBase):
def __exit__(self, exc_type, exc_value, traceback) -> None:
return None
def suppress1(x: int | str) -> None:
if isinstance(x, int):
with Suppress1():
raise ValueError
assert_type(x, int | str)
def nosuppress1(x: int | str) -> None:
if isinstance(x, int):
with NoSuppress1():
raise ValueError
assert_type(x, str)There was a problem hiding this comment.
Ah, nice. Yeah, I was trying to think of other code flow patterns that might work better. I like your suggestion. I'll make that change.
There was a problem hiding this comment.
Hmm... Pyre shouldn't pass this test, because I know for a fact that we get this wrong :)
In Pyre, we incorrectly assume that context managers never swallow exceptions. This stems from a fundamental design flaw, where we construct the control flow graph in a phase before we have any type information, so we can't possibly inspect the return type annotation.
There was a problem hiding this comment.
Pyre passing in the playground seems to be an issue with the playground (I will investigate). When I try with a recent build in my dev environment, I see the expected failures for both suppress1 and suppress2.
Incompatible parameter type [6]: In call `assert_type`, for 1st positional argument, expected `Union[int, str]` but got `str`.
There was a problem hiding this comment.
@samwgoldman, hmm, I'm not sure how to square that with the test results. Pyre is emitting no errors (i.e. no assert_type failures) for this test, so it seems to be doing the right thing — unless there is some other bug that's causing it to not detect an assert_type mismatch. Any insights would be appreciated. If there's a better way to write this test, I'm happy to modify it.
There was a problem hiding this comment.
Ah, interesting. The conformance tests make extensive use of assert_type. If assert_type is not working correctly, that will affect many test results.
There was a problem hiding this comment.
Here's a variation that correctly finds the bug in pyre:
from typing import Any, Literal, assert_type
class CMBase:
def __enter__(self) -> None:
pass
class Suppress1(CMBase):
def __exit__(self, exc_type, exc_value, traceback) -> bool:
return True
class Suppress2(CMBase):
def __exit__(self, exc_type, exc_value, traceback) -> Literal[True]:
return True
class NoSuppress1(CMBase):
def __exit__(self, exc_type, exc_value, traceback) -> None:
return None
def suppress1(x: int | str) -> int | str:
if isinstance(x, int):
with Suppress1():
raise ValueError
return x + "x" # E
def nosuppress1(x: int | str) -> str:
if isinstance(x, int):
with NoSuppress1():
raise ValueError
return x + "x"
It produces no errors, so it fails because it doesn't produce the expected error. Mypy and pyright produce the correct error.
There was a problem hiding this comment.
Indeed, Steven fixed assert_type recently (a few weeks ago) in order to better support the conformance tests. I suspect that is the issue and we need to update the playground and re-record the conformance results.
There was a problem hiding this comment.
Steven fixed assert_type recently
Ah, that's great to hear. If you publish a new version of pyre, the conformance test framework will automatically discover it and use it. Do you have a timeline for when you will publish the next version?
There was a problem hiding this comment.
Yes, that's great! We can use the x + y variant I posted above if necessary, but it would be better if we can use assert_type() here.
No description provided.