Traverse ParamSpec prefix where we should#19800
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
sterliakov
left a comment
sterliakov
left a comment
There was a problem hiding this comment.
Nice catch! I grepped for \.upper_bound at the HEAD of this PR and inspected the matches, and I haven't found any missing cases (except for one that probably shouldn't be changed as process_param_spec_def looks like something that should only run for definitions, although I can't find where it is actually called).
| def visit_param_spec(self, typ: ParamSpecType) -> None: | ||
| typ.upper_bound.accept(self) | ||
| typ.default.accept(self) | ||
| typ.prefix.accept(self) |
There was a problem hiding this comment.
Should NodeReplaceVisitor above do the same? More importantly, why isn't its process_param_spec_def actually ever called (as well as process_type_var_tuple_def)?
There was a problem hiding this comment.
This leads to #14872 where process_param_spec_def was added, but apparently not connected to anything? It should probably be...
There was a problem hiding this comment.
ParamSpec definitions cannot have prefixes, it is purely mypy-internal thing to represent Concatenate in the type system. Also I noticed that process_param_spec_def() and process_type_var_tuple_def() are not called anywhere. Probably they should be called from visit_class_def(). But this is definitely a topic for a separate issue/PR.
There was a problem hiding this comment.
Yep, I remember what a PSpec prefix means:) I just wasn't able to make sure that process_param_spec_def is actually called on definitions and not on other ParamSpec occurrences
2 participants
Fixes #18087
I started from fixing an incremental crash from
fixup.py, but then noticed we don't traverseParamSpecprefix in multiple places where we should, so I fixed most of those as well.