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

Allow @final on TypedDict #13557

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Conversation

godlygeek
Copy link
Contributor

Allow a TypedDict to be decorated with @final. Like a regular class, mypy will emit an error if a final TypedDict is subclassed.

Relates-to: #7981

Description

Allow @final to be applied to a TypedDict, and have mypy emit an error if class is derived from a final TypedDict. This goes some way towards closing #7981 and closing a feature gap with pyright, though not the whole way, as #7981 also asks for additional type narrowing for a final TypedDict.

Test Plan

testCannotUseFinalDecoratorWithTypedDict was removed, and testCannotSubclassFinalTypedDict added.

@github-actions

This comment has been minimized.

@@ -1469,8 +1469,8 @@ def analyze_typeddict_classdef(self, defn: ClassDef) -> bool:
for decorator in defn.decorators:
decorator.accept(self)
if isinstance(decorator, RefExpr):
if decorator.fullname in FINAL_DECORATOR_NAMES:
self.fail("@final cannot be used with TypedDict", decorator)
if decorator.fullname in FINAL_DECORATOR_NAMES and info is not None:
Copy link
Member

Choose a reason for hiding this comment

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

What if the info is None? Does that happen if there's a circular reference? It would be good to get a test covering that code path.

Copy link
Member

Choose a reason for hiding this comment

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

It could be FakeInfo for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the info is None? Does that happen if there's a circular reference?

When there's a forward declaration in the TypedDict body, analyze_typeddict_classdef gets called multiple times. info is initially None in that case, but eventually it's a TypeInfo.

It would be good to get a test covering that code path.

Done.

It could be FakeInfo for some reason.

When would that happen?

@JelleZijlstra
Copy link
Member

Also the CI failures look like there are some duplicate error messages.

Allow a `TypedDict` to be decorated with `@final`. Like a regular class,
mypy will emit an error if a final `TypedDict` is subclassed.

Relates-to: python#7981
@godlygeek
Copy link
Contributor Author

Also the CI failures look like there are some duplicate error messages.

That's what I get for only running the TypedDict tests. Should be fixed - we need to fail in analyze_typeddict_classdef only if the class with a @final base actually is a TypedDict, and otherwise leave the failing to visit_class_def.

@github-actions

This comment has been minimized.

@godlygeek
Copy link
Contributor Author

godlygeek commented Aug 30, 2022

Huh. This seems to be causing a mypyc segfault, judging by Exit status: -11? 👀

The mypyc tests (pytest -q mypyc) all pass for me locally... I'm not sure how to debug that...

@JelleZijlstra
Copy link
Member

The segfaults may be unrelated, we're seeing them on a bunch of PRs. Not sure what's causing them :(

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Although it's a little surprising to me that we don't have the correct MRO (in which case we could just fallback to the existing final logic in checker.py). Not familiar enough with mypy's TypedDict impl to know if that's intentional.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

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.

4 participants