[PEP 695] Fix multiple nested classes don't work by changhoetyng · Pull Request #17820 · python/mypy (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits9 Checks17 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This PR modifies the lookup_fully_qualified_or_none method to support multiple nested classes.

Fixes #17780

This comment has been minimized.

Choose a reason for hiding this comment

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

Thanks for the PR! The logic looks mostly right, but I don't think it works for packages.

return None
# Check if there's nested module A.B.C
splitted = fullname.rsplit(".")
module, names = splitted[0], splitted[1:]

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Sorry about that! I'll take a look as soon as possible :)

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!

Choose a reason for hiding this comment

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

Thanks for the updates! I have few comments, mostly stylistic.

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.
"""

Choose a reason for hiding this comment

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

Style nit: We only use docstrings at beginning of a function/module/class, not inside function bodies.

@@ -6408,14 +6408,44 @@ def lookup_fully_qualified_or_none(self, fullname: str) -> SymbolTableNode \| Non
# TODO: support nested classes (but consider performance impact,
# 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:

Choose a reason for hiding this comment

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

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.

module, name = fullname.rsplit(".", maxsplit=1)
if module not in self.modules:

splitted_modules = fullname.rsplit(".")

Choose a reason for hiding this comment

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

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.

@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?

 # TODO: support nested classes (but consider performance impact,
 #       we might keep the module level only lookup for thing like 'builtins.int').

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.

Choose a reason for hiding this comment

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

Thanks for the updates! Looks good now.

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

2 participants