[PEP 695] Fix multiple nested classes don't work#17820
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR! The logic looks mostly right, but I don't think it works for packages.
mypy/semanal.py
Outdated
| return None | ||
| # Check if there's nested module A.B.C | ||
| splitted = fullname.rsplit(".") | ||
| module, names = splitted[0], splitted[1:] |
There was a problem hiding this comment.
I think this logic is not quite right for packages. We should find the longest prefix of fullname that is a module (it could be e.g. a.b for a.b.C.D), and then look up through TypeInfos based on that.
Also add a test case for a case like a.b.C.D where a.b is a package.
There was a problem hiding this comment.
Sorry about that! I'll take a look as soon as possible :)
This comment has been minimized.
This comment has been minimized.
|
Hi @JukkaL, I have fixed this. Now, the logic identifies the longest prefix of full name that represents a module. I have also added a test case to cover scenarios with nested packages. Let me know if you want me to adjust the implementation further. Thanks! |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates! I have few comments, mostly stylistic.
mypy/semanal.py
Outdated
| If the module is not found, pop the last element of the splitted list and append it to the names list. | ||
|
|
||
| This is to find the longest prefix of the module name that is in the modules dictionary. | ||
| """ |
There was a problem hiding this comment.
Style nit: We only use docstrings at beginning of a function/module/class, not inside function bodies.
| # we might keep the module level only lookup for thing like 'builtins.int'). | ||
| assert "." in fullname | ||
| module, name = fullname.rsplit(".", maxsplit=1) | ||
| if module not in self.modules: |
There was a problem hiding this comment.
Please keep the original "fast path" which we'll be hitting most of the time. This way your new logic, which is a bit more expensive, it not slowing he typical code path. This may be performance critical. In more concrete terms, add your new logic under if module not in self.modules:, and keep the other code as is in the function.
mypy/semanal.py
Outdated
| module, name = fullname.rsplit(".", maxsplit=1) | ||
| if module not in self.modules: | ||
|
|
||
| splitted_modules = fullname.rsplit(".") |
There was a problem hiding this comment.
Minor: Here you can use just fullname.split("."), since it won't make a difference in semantics and split is the more usual variant.
This comment has been minimized.
This comment has been minimized.
|
@JukkaL Thanks for the comments. I have made some changes to them. Let me know if you have any extra comments! Since this change will allow support for nested classes, should I remove the below TODO? |
Yes, please! |
|
@JukkaL It's done! Is there anything else I need to add? |
This comment has been minimized.
This comment has been minimized.
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates! Looks good now.
|
@JukkaL No worries! |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
2 participants
This PR modifies the
lookup_fully_qualified_or_nonemethod to support multiple nested classes.Fixes #17780