Clean-up classes nested in functions#21478
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
It looks like this also fixes #13024 I am going to add a test case. |
|
Btw |
|
Diff from mypy_primer, showing the effect of this PR on open source code: zulip (https://github.com/zulip/zulip)
- zerver/lib/user_topics.py:265: error: Argument 1 to "map" has incompatible type "Callable[[RecipientTopicDict@252], Any]"; expected "Callable[[dict[str, Any]], Any]" [arg-type]
+ zerver/lib/user_topics.py:265: error: Argument 1 to "map" has incompatible type "Callable[[RecipientTopicDict], Any]"; expected "Callable[[dict[str, Any]], Any]" [arg-type]
- zerver/lib/user_topics.py:303: error: Argument 1 to "map" has incompatible type "Callable[[RecipientTopicDict@252], Any]"; expected "Callable[[dict[str, Any]], Any]" [arg-type]
+ zerver/lib/user_topics.py:303: error: Argument 1 to "map" has incompatible type "Callable[[RecipientTopicDict], Any]"; expected "Callable[[dict[str, Any]], Any]" [arg-type]
|
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up! Looks good overall, but I have a few concerns about cases where we no longer include a line number.
| def f() -> None: | ||
| class X: | ||
| ... | ||
| x # E: Name "x" is not defined |
There was a problem hiding this comment.
Lower case vs upper case X. If intended, maybe add comment, since they look pretty similar.
There was a problem hiding this comment.
I am going to rename x to undefined.
| A | ||
| TupleType( | ||
| tuple[Any, fallback=__main__.N@2]) | ||
| tuple[Any, fallback=__main__.namedtuple@N]) |
There was a problem hiding this comment.
Since we no longer have a line number, what happens if we have multiple namedtuple('N', ...') base classes in a single file?
There was a problem hiding this comment.
I was going to say that this can't happen because there can't be two classes with the same name, but apparently we use the string name (i.e. N, not A) in such cases. I think a proper solution would be to use something like A@base1, etc.
To be clear I don't want to use line number for this disambiguation as well, since we can get weird things like Foo@7@7 if we need to apply both (e.g. namedtuple base class for a class nested in function).
| # The second one is needed to properly serialize any classes nested in functions. | ||
| # TODO: make sure the daemon works well with these rules. | ||
| if self.is_nested_within_func_scope(): | ||
| class_def.fullname = f"{self.cur_mod_id}.{name}@{line}" |
There was a problem hiding this comment.
This name generation logic is duplicated in a few places -- maybe move it to a helper function?
| name += "@" + str(call.line) | ||
| # * This is a local (function or method level) named tuple, this case is | ||
| # handled by basic_new_typeinfo(). | ||
| name = "namedtuple@" + name |
There was a problem hiding this comment.
As discussed in my other comment, I wonder if preserving the line number would be helpful here.
|
|
||
| if var_name is None: | ||
| # Give it unique name in case a TypedDict() call is used as a base class. | ||
| name = "TypedDict@" + name |
There was a problem hiding this comment.
This looks similar to namedtuple case -- would the line number be helpful here, in case there are multiple base classes in the same file?
2 participants
Fixes #6422
Fixes #13024
TBH the current situation is embarrassing. I decided to finally clean this all up and unify various cases. Now we have same simple rules used by all classes, both regular (first three) and magic:
defn.nameonlydefn.fullname.One = NamedTuple("Other", ...)etc) always use thevar_namefor all purposes.