-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[ty] Improve @override, @final and Liskov checks in cases where there are multiple reachable definitions
#21767
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
Changes from all commits
597c778
d2e8da5
53598ac
c483914
fa361c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -488,11 +488,110 @@ class C(A): | |
| pass | ||
|
|
||
| if coinflip(): | ||
| def method2(self) -> None: ... # TODO: should emit [override-of-final-method] | ||
| def method2(self) -> None: ... # error: [override-of-final-method] | ||
| else: | ||
| def method2(self) -> None: ... # TODO: should emit [override-of-final-method] | ||
| def method2(self) -> None: ... | ||
|
|
||
| if coinflip(): | ||
| def method3(self) -> None: ... # error: [override-of-final-method] | ||
| def method4(self) -> None: ... # error: [override-of-final-method] | ||
|
|
||
| # TODO: we should emit Liskov violations here too: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like we do? At least at the first definition...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, we emit a diagnostic stating that an |
||
| if coinflip(): | ||
| method4 = 42 # error: [override-of-final-method] | ||
| else: | ||
| method4 = 56 | ||
| ``` | ||
|
|
||
| ## Definitions in statically known branches | ||
|
|
||
| ```toml | ||
| [environment] | ||
| python-version = "3.10" | ||
| ``` | ||
|
|
||
| ```py | ||
| import sys | ||
| from typing_extensions import final | ||
|
|
||
| class Parent: | ||
| if sys.version_info >= (3, 10): | ||
| @final | ||
| def foo(self) -> None: ... | ||
| @final | ||
| def foooo(self) -> None: ... | ||
| @final | ||
| def baaaaar(self) -> None: ... | ||
| else: | ||
| @final | ||
| def bar(self) -> None: ... | ||
| @final | ||
| def baz(self) -> None: ... | ||
| @final | ||
| def spam(self) -> None: ... | ||
|
|
||
| class Child(Parent): | ||
| def foo(self) -> None: ... # error: [override-of-final-method] | ||
|
|
||
| # The declaration on `Parent` is not reachable, | ||
| # so this is fine | ||
| def bar(self) -> None: ... | ||
|
|
||
| if sys.version_info >= (3, 10): | ||
| def foooo(self) -> None: ... # error: [override-of-final-method] | ||
| def baz(self) -> None: ... | ||
| else: | ||
| # Fine because this doesn't override any reachable definitions | ||
| def foooo(self) -> None: ... | ||
| # There are `@final` definitions being overridden here, | ||
| # but the definitions that override them are unreachable | ||
| def spam(self) -> None: ... | ||
| def baaaaar(self) -> None: ... | ||
| ``` | ||
|
|
||
| ## Overloads in statically-known branches in stub files | ||
|
|
||
| <!-- snapshot-diagnostics --> | ||
|
|
||
| ```toml | ||
| [environment] | ||
| python-version = "3.10" | ||
| ``` | ||
|
|
||
| ```pyi | ||
| import sys | ||
| from typing_extensions import overload, final | ||
|
|
||
| class Foo: | ||
| if sys.version_info >= (3, 10): | ||
| @overload | ||
| @final | ||
| def method(self, x: int) -> int: ... | ||
| else: | ||
| @overload | ||
| def method(self, x: int) -> int: ... | ||
| @overload | ||
| def method(self, x: str) -> str: ... | ||
|
|
||
| if sys.version_info >= (3, 10): | ||
| @overload | ||
| def method2(self, x: int) -> int: ... | ||
| else: | ||
| @overload | ||
| @final | ||
| def method2(self, x: int) -> int: ... | ||
| @overload | ||
| def method2(self, x: str) -> str: ... | ||
|
|
||
| class Bar(Foo): | ||
| @overload | ||
| def method(self, x: int) -> int: ... | ||
| @overload | ||
| def method(self, x: str) -> str: ... # error: [override-of-final-method] | ||
|
|
||
| # This is fine: the only overload that is marked `@final` | ||
| # is in a statically-unreachable branch | ||
| @overload | ||
| def method2(self, x: int) -> int: ... | ||
| @overload | ||
| def method2(self, x: str) -> str: ... | ||
| ``` | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BurntSushi -- both of the tests being changed in this file have FIXME comments above them. But I'm not sure if the changes this PR makes to the tests fixes the problems, or makes the problems worse 😆 I'm not totally sure I understand what these tests are meant to be asserting
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LMK if you consider this a regression and I can fix it in a followup. It shouldn't be too hard to track whether a member has multiple definitions and propagate that information upwards, if that's what the autocomplete machinery wants to know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea here was just trying to test what happens when we try to insert an import for a symbol that is already imported, but whose imports are conditional. I think in this case we don't want to add any new imports. But we weren't doing that before either. So I think this is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thank you!