Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#4897: allow raise E where E: Type[Exn] #4918

Merged
merged 4 commits into from May 15, 2018

Conversation

bwo
Copy link
Contributor

@bwo bwo commented Apr 16, 2018

That is, where E is annotated as a Type, rather than being inferred as a
FunctionLike which is a type object.

This also allows raise E where E is Type[Any], which seems right but
I was not certain about.

Fixes #4897

that is, where E is annotated as a Type, rather than being inferred as a
FunctionLike which is a type object.

This also allows `raise E` where E is Type[Any], which *seems* right but
I was not certain about.
[case testRaiseExceptionType]
import typing
x = None # type: typing.Type[BaseException]
raise x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a negative test (e.g., x: Type[int]; raise x still errors)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@JelleZijlstra
Copy link
Member

I think allowing Type[Any] is right: Any means any type, so it may be a valid exception type.

@Michael0x2a
Copy link
Collaborator

I think your code would probably be cleaner and start type-checking if you first checked if it's an instance of TypeType and extracted the inner type before doing the if isinstance(typ, FunctionLike) and typ.is_type_obj() check. Currently, you do it the other way around.

Basically, extract the inner type first if needed, then do the same checks whether the type was extracted or not.

@bwo
Copy link
Contributor Author

bwo commented Apr 17, 2018

done

@bwo
Copy link
Contributor Author

bwo commented Apr 21, 2018

ping?

@@ -2357,6 +2357,10 @@ def visit_raise_stmt(self, s: RaiseStmt) -> None:
def type_check_raise(self, e: Expression, s: RaiseStmt,
optional: bool = False) -> None:
typ = self.expr_checker.accept(e)
if isinstance(typ, TypeType):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't (for consistency) the same case be added to check_except_handler_test (with test cases)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you mean that this should work:

[case testExceptWithTypeType]
import typing
E = BaseException  # type: typing.Type[BaseException]

try:
    pass
except E:
    pass
[builtins fixtures/exception.pyi]

it already does, though I can certainly add that test.

@msullivan
Copy link
Collaborator

@bwo, do you have a chance to look at Ivan's feedback?

@bwo
Copy link
Contributor Author

bwo commented May 15, 2018

@msullivan finally got back to this. Thanks for the reminder.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, looks good to me now!

@ilevkivskyi ilevkivskyi merged commit 9844936 into python:master May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants