New semantic analyzer: fix deserialization of generated classes #6627
Conversation
There was a problem hiding this comment.
Not sure if this is best way to fix the issue, but I didn't look very carefully. Left a few additional idea.
It's okay to also merge this as is and create a follow-up issue about cleaning this up, since the whole concept of nested classes leaking into module symbol tables could be improved.
| @@ -2803,6 +2803,10 @@ def process_typevar_parameters(self, args: List[Expression], | |||
| def basic_new_typeinfo(self, name: str, basetype_or_fallback: Instance) -> TypeInfo: | |||
| class_def = ClassDef(name, Block([])) | |||
| class_def.fullname = self.qualified_name(name) | |||
| if self.is_func_scope() and not self.type: | |||
There was a problem hiding this comment.
This would be better as an if/else statement.
mypy/newsemanal/semanal.py
Outdated
| if self.is_func_scope() and not self.type: | ||
| # Full names of generated classes should always be prefixed with the module names | ||
| # even if they are nested in a function, since these classes will be (de-)serialized. | ||
| class_def.fullname = self.cur_mod_id + '.' + class_def.fullname |
There was a problem hiding this comment.
It looks like this can collide with another class name. Maybe you should use the @line suffix that we use elsewhere?
There was a problem hiding this comment.
This should be already done by class generation code. I will add a comment.
| class C: ... | ||
| from collections import namedtuple | ||
| def test() -> None: | ||
| NT = namedtuple('BadName', ['x', 'y']) |
There was a problem hiding this comment.
I wonder if we could instead not serialize the class, since it's only visible within the function body.
Another idea would be to ignore the 'BadName' argument here and treat it as NT. Would this fix the issue?
There was a problem hiding this comment.
Actually, this is a good idea. A type can leak to outer scope only from a method, so we can just don't serialize the class for non-method functions.
There was a problem hiding this comment.
As we discussed privately, we will stick to just fixing the full name for now.
|
The issue for this is essentially #6422, but this is only partial fix. |
In new semantic analyzer,
qualified_name()returns a bare name if called inside a top-level (non-method) function. I am not sure for motivation and can't say it is generally wrong. Here is the hot-fix for a case where this is definitely wrong: generated classes.