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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes to column number reporting #7078
Conversation
Previously many types didn't have column numbers. This gives correct column number of simple types like `Foo` and `List[Foo]` in Python 3 type annotations. For type comments we cheat a bit and point them to the surrounding statement or definition, which is still a small improvement. For some types we may still be missing column numbers, but this covers the most common cases.
This only affects new semantic analyzer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good! I only have minor comments.
self.node_stack = [] # type: List[AST] | ||
self.assume_str_is_unicode = assume_str_is_unicode | ||
|
||
def convert_column(self, column: int) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a docstring what is the intended use of this method?
@@ -3516,6 +3517,7 @@ def analyze_type_application(self, expr: IndexExpr) -> None: | |||
base = expr.base | |||
expr.analyzed = TypeApplication(base, types) | |||
expr.analyzed.line = expr.line | |||
expr.analyzed.column = expr.column |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are couple more similar places: process_typevar_declaration()
and process_newtype_declaration()
not sure if they are important.
test-data/unit/check-classes.test
Outdated
main:7: error: Type argument "__main__.G[builtins.int]" of "G" must be a subtype of "builtins.str" | ||
main:7: error: Type argument "builtins.int" of "G" must be a subtype of "builtins.str" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add --show-column-numbers
in this test? (IIUC columns caused the change of order.)
|
||
class function: pass | ||
|
||
property = object() # Dummy definition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two spaces before comment?
Previously many types and AST nodes didn't have
(good) column numbers. This fixes several such issues.
In particular, this gives correct column number of simple
types like
Foo
andList[Foo]
in Python 3 typeannotations.
For type comments we cheat a bit and point them to the
surrounding statement or definition, which is still a
small improvement.
For some types we may still be missing column numbers,
but this covers the most common cases.