Better support for type annotations#16
Conversation
- Introduced `TypeCommentVisitor` to extract names from PEP 484 style type comments in function definitions, variable assignments, for loops, and with statements. - Updated `find_unused_imports` to include type comment names in the analysis. - Added comprehensive tests for various scenarios involving type comments, ensuring correct handling and precedence over PEP 526 annotations. This enhancement improves the tool's ability to analyze and manage type comments effectively, aligning with Python's type hinting standards.
…ing annotations - Updated the import analyzer to detect indirect attribute accesses in type comments and string annotations, allowing for more comprehensive analysis. - Introduced `TypeStringAttrUsage` to represent usages found in type comments and string annotations. - Modified `CrossFileAnalyzer` to collect and process these usages alongside regular code attribute accesses. - Enhanced the autofix functionality to rewrite type comments and string annotations to use direct imports. - Added tests to ensure correct detection and rewriting of indirect accesses in both type comments and string annotations. This improvement aligns with Python's type hinting standards and enhances the tool's capability to manage indirect imports effectively.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for PEP 484 type comments (e.g., # type: T) to the import analyzer, enabling detection and autofixing of indirect attribute accesses in both type comments and string annotations. The implementation extends the existing analysis capabilities to handle legacy Python 2-style type hints alongside modern PEP 526 annotations.
Changes:
- Added comprehensive type comment parsing with support for variable, function signature, per-argument, for-loop, and with-statement type comments
- Extended cross-file analysis to detect indirect attribute accesses in type comments and string annotations
- Enhanced autofix functionality to rewrite type strings when fixing indirect imports
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/type_comments_test.py | New comprehensive test suite covering all type comment scenarios including edge cases, PEP 526 precedence, and invalid syntax handling |
| tests/cross_file_test.py | Added integration tests for detecting and fixing indirect attribute accesses in type comments and string annotations |
| pyproject.toml | Disabled mypy "misc" error code for test files to allow type comment test strings |
| import_analyzer/_detection.py | Integrated type comment collection into unused import detection by enabling type_comments=True parsing |
| import_analyzer/_data.py | Added TypeStringUsage dataclass to represent type string usages and extended IndirectAttributeAccess to track them |
| import_analyzer/_cross_file.py | Extended cross-file analyzer to collect and process type string attribute usages alongside code usages |
| import_analyzer/_autofix.py | Enhanced autofix to locate and rewrite indirect accesses in type comments and string annotations |
| import_analyzer/_ast_helpers.py | Implemented TypeCommentVisitor for name extraction and TypeStringAttributeCollector for attribute access detection in type strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Search for the old_prefix in the line starting around col_offset | ||
| # This handles cases where col_offset might be approximate | ||
| search_start = max(0, ts_usage.col_offset - 10) |
There was a problem hiding this comment.
The search window of 10 characters (line 709) might not be sufficient for all cases. If the type comment has significant whitespace or formatting before the attribute access, the search could fail. Consider using a more robust approach, such as finding the comment marker "# type:" first and then searching from there, or widening the search window based on the line structure.
| # Search for the old_prefix in the line starting around col_offset | |
| # This handles cases where col_offset might be approximate | |
| search_start = max(0, ts_usage.col_offset - 10) | |
| # Search for the old_prefix in the type comment. | |
| # First, try to anchor on the "# type:" marker; if not found, | |
| # fall back to a wider window around the approximate col_offset. | |
| type_comment_marker = "# type:" | |
| marker_pos = line.find(type_comment_marker) | |
| if marker_pos != -1: | |
| search_start = marker_pos | |
| else: | |
| # Use a wider window around the approximate offset to improve robustness. | |
| search_start = max(0, ts_usage.col_offset - 50) |
| # Note: There could be multiple occurrences, handle each | ||
| start = 0 | ||
| while True: | ||
| pos = type_str.find(full_attr, start) | ||
| if pos == -1: | ||
| break | ||
|
|
||
| # Calculate absolute column positions | ||
| col_offset = base_col + pos | ||
| end_col_offset = col_offset + len(full_attr) | ||
|
|
||
| self.usages.append( | ||
| TypeStringAttrUsage( | ||
| root_name=root_name, | ||
| attr_path=attr_path, | ||
| lineno=lineno, | ||
| col_offset=col_offset, | ||
| end_col_offset=end_col_offset, | ||
| context=context, | ||
| ), | ||
| ) | ||
|
|
||
| # Move past this occurrence | ||
| start = pos + len(full_attr) | ||
| break # Only record once per AST node |
There was a problem hiding this comment.
The comment on line 1037 says "There could be multiple occurrences, handle each" but the code breaks after finding the first occurrence (line 1061). This is inconsistent - either the comment should be updated to say "Only record once per AST node" or the break should be removed if multiple occurrences should be handled. The current implementation only records the first occurrence.
| if isinstance(node, ast.Constant) and isinstance(node.value, str): | ||
| # The col_offset of the string literal (after the quote) | ||
| # For a string annotation like x: "models.User", the string | ||
| # starts after the opening quote | ||
| self._process_type_string( | ||
| node.value, | ||
| node.lineno, | ||
| node.col_offset + 1, # +1 to skip the opening quote | ||
| "string_annotation", | ||
| ) | ||
| elif isinstance(node, ast.Subscript): | ||
| self._process_string_annotation(node.slice, lineno) | ||
| self._process_string_annotation(node.value, lineno) | ||
| elif isinstance(node, ast.Tuple): | ||
| for elt in node.elts: | ||
| self._process_string_annotation(elt, lineno) | ||
| elif isinstance(node, ast.BinOp): | ||
| self._process_string_annotation(node.left, lineno) | ||
| self._process_string_annotation(node.right, lineno) |
There was a problem hiding this comment.
The lineno parameter in _process_string_annotation is defined but never used. It's passed through recursive calls but the actual line number comes from node.lineno (line 1184). This parameter should either be removed or the logic should be updated to use it when node.lineno is not available.
| if isinstance(node, ast.Constant) and isinstance(node.value, str): | |
| # The col_offset of the string literal (after the quote) | |
| # For a string annotation like x: "models.User", the string | |
| # starts after the opening quote | |
| self._process_type_string( | |
| node.value, | |
| node.lineno, | |
| node.col_offset + 1, # +1 to skip the opening quote | |
| "string_annotation", | |
| ) | |
| elif isinstance(node, ast.Subscript): | |
| self._process_string_annotation(node.slice, lineno) | |
| self._process_string_annotation(node.value, lineno) | |
| elif isinstance(node, ast.Tuple): | |
| for elt in node.elts: | |
| self._process_string_annotation(elt, lineno) | |
| elif isinstance(node, ast.BinOp): | |
| self._process_string_annotation(node.left, lineno) | |
| self._process_string_annotation(node.right, lineno) | |
| # Determine the best-known line number for this node and its children. | |
| # Prefer an explicitly passed-in lineno, otherwise fall back to the node's | |
| # own lineno attribute if available. | |
| effective_lineno = lineno or getattr(node, "lineno", None) | |
| if isinstance(node, ast.Constant) and isinstance(node.value, str): | |
| # The col_offset of the string literal (after the quote) | |
| # For a string annotation like x: "models.User", the string | |
| # starts after the opening quote | |
| self._process_type_string( | |
| node.value, | |
| getattr(node, "lineno", effective_lineno), | |
| getattr(node, "col_offset", 0) + 1, # +1 to skip the opening quote | |
| "string_annotation", | |
| ) | |
| elif isinstance(node, ast.Subscript): | |
| self._process_string_annotation(node.slice, effective_lineno) | |
| self._process_string_annotation(node.value, effective_lineno) | |
| elif isinstance(node, ast.Tuple): | |
| for elt in node.elts: | |
| self._process_string_annotation(elt, effective_lineno) | |
| elif isinstance(node, ast.BinOp): | |
| self._process_string_annotation(node.left, effective_lineno) | |
| self._process_string_annotation(node.right, effective_lineno) |
| def _split_type_args(self, args_str: str) -> list[str]: | ||
| """Split comma-separated type args, respecting brackets.""" | ||
| result: list[str] = [] | ||
| current: list[str] = [] | ||
| depth = 0 | ||
| for char in args_str: | ||
| if char == "[": | ||
| depth += 1 | ||
| elif char == "]": | ||
| depth -= 1 | ||
| elif char == "," and depth == 0: | ||
| result.append("".join(current)) | ||
| current = [] | ||
| continue | ||
| current.append(char) | ||
| if current: | ||
| result.append("".join(current)) | ||
| return result |
There was a problem hiding this comment.
The _split_type_args method is duplicated - it appears identically in both TypeCommentVisitor (lines 809-826) and TypeStringAttributeCollector (lines 1131-1148). This code duplication should be refactored into a shared helper function to improve maintainability.
| def _has_annotations( | ||
| self, | ||
| node: ast.FunctionDef | ast.AsyncFunctionDef, | ||
| ) -> bool: | ||
| """Check if function has any PEP 526 style annotations.""" | ||
| if node.returns: | ||
| return True | ||
| all_args = ( | ||
| node.args.args + node.args.posonlyargs + node.args.kwonlyargs | ||
| ) | ||
| for arg in all_args: | ||
| if arg.annotation: | ||
| return True | ||
| if node.args.vararg and node.args.vararg.annotation: | ||
| return True | ||
| if node.args.kwarg and node.args.kwarg.annotation: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
The _has_annotations method is duplicated - it appears identically in both TypeCommentVisitor (lines 858-876) and TypeStringAttributeCollector (lines 1150-1167). This code duplication should be refactored into a shared helper function to improve maintainability.
| original_source: Path # Where it's actually defined | ||
| is_same_package: bool # True if re-exporter is __init__.py of original's package | ||
| type_string_usages: list[TypeStringUsage] = field(default_factory=list) # Usages in type strings | ||
| is_same_package: bool = False # True if re-exporter is __init__.py of original's package |
There was a problem hiding this comment.
The default value for is_same_package field should use field(default=False) instead of a bare False for consistency with the type_string_usages field above it, which uses field(default_factory=list). While both approaches work, using field() consistently is better style for dataclasses with multiple default fields.
| is_same_package: bool = False # True if re-exporter is __init__.py of original's package | |
| is_same_package: bool = field(default=False) # True if re-exporter is __init__.py of original's package |
| # Note: There could be multiple occurrences, handle each | ||
| start = 0 | ||
| while True: | ||
| pos = type_str.find(full_attr, start) | ||
| if pos == -1: | ||
| break | ||
|
|
||
| # Calculate absolute column positions | ||
| col_offset = base_col + pos | ||
| end_col_offset = col_offset + len(full_attr) | ||
|
|
||
| self.usages.append( | ||
| TypeStringAttrUsage( | ||
| root_name=root_name, | ||
| attr_path=attr_path, | ||
| lineno=lineno, | ||
| col_offset=col_offset, | ||
| end_col_offset=end_col_offset, | ||
| context=context, | ||
| ), | ||
| ) | ||
|
|
||
| # Move past this occurrence | ||
| start = pos + len(full_attr) | ||
| break # Only record once per AST node | ||
|
|
There was a problem hiding this comment.
This assignment to 'start' is unnecessary as it is redefined before this value is used.
| # Note: There could be multiple occurrences, handle each | |
| start = 0 | |
| while True: | |
| pos = type_str.find(full_attr, start) | |
| if pos == -1: | |
| break | |
| # Calculate absolute column positions | |
| col_offset = base_col + pos | |
| end_col_offset = col_offset + len(full_attr) | |
| self.usages.append( | |
| TypeStringAttrUsage( | |
| root_name=root_name, | |
| attr_path=attr_path, | |
| lineno=lineno, | |
| col_offset=col_offset, | |
| end_col_offset=end_col_offset, | |
| context=context, | |
| ), | |
| ) | |
| # Move past this occurrence | |
| start = pos + len(full_attr) | |
| break # Only record once per AST node | |
| # Note: We only record the first occurrence per AST node | |
| pos = type_str.find(full_attr) | |
| if pos == -1: | |
| continue | |
| # Calculate absolute column positions | |
| col_offset = base_col + pos | |
| end_col_offset = col_offset + len(full_attr) | |
| self.usages.append( | |
| TypeStringAttrUsage( | |
| root_name=root_name, | |
| attr_path=attr_path, | |
| lineno=lineno, | |
| col_offset=col_offset, | |
| end_col_offset=end_col_offset, | |
| context=context, | |
| ), | |
| ) |
| collector = NameUsageCollector() | ||
| collector.visit(parsed) | ||
| self.used_names.update(collector.used_names) | ||
| except SyntaxError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except SyntaxError: | |
| except SyntaxError: | |
| # Invalid type comments are ignored; failing to parse them should not | |
| # affect name collection for the rest of the file. |
| self._extract_attrs_from_expr( | ||
| parsed.body, type_str, lineno, col_offset, context, | ||
| ) | ||
| except SyntaxError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except SyntaxError: | |
| except SyntaxError: | |
| # Malformed or non-Python type string; ignore and skip extraction. |
- Updated `StringAnnotationVisitor` to support parsing of various typing constructs, including `typing.cast()`, `TypeVar()`, `Literal[]`, `Annotated[]`, and `TypeAlias`. - Introduced special handling for string annotations and type comments, ensuring correct identification of annotation contexts. - Added tests for new behaviors, including edge cases and mixed type scenarios, to ensure robust detection of unused names in type annotations. This enhancement improves the tool's compliance with Python's type hinting standards and expands its capabilities in analyzing type annotations effectively.
cmyui
changed the title
# type: T comments# type: T comments and other annotations correctly
cmyui
changed the title
# type: T comments and other annotations correctlyThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Move past this occurrence | ||
| start = pos + len(full_attr) | ||
| break # Only record once per AST node |
There was a problem hiding this comment.
The while loop is intended to handle multiple occurrences of the same attribute access in a type string (as indicated by the comment on line 1135), but the break statement on line 1159 causes the loop to exit after finding only the first occurrence. This means if a type string contains multiple instances of the same module.attr pattern (e.g., "Tuple[models.User, models.User]"), only the first occurrence will be recorded.
Consider either removing the break statement to process all occurrences, or clarifying the intended behavior with updated comments.
| break # Only record once per AST node |
| # The col_offset of the string literal (after the quote) | ||
| # For a string annotation like x: "models.User", the string | ||
| # starts after the opening quote | ||
| self._process_type_string( | ||
| node.value, | ||
| node.lineno, | ||
| node.col_offset + 1, # +1 to skip the opening quote |
There was a problem hiding this comment.
The column offset calculation assumes all string annotations use single quotes (" or ') by adding +1 to skip the opening quote. However, for triple-quoted string annotations (""" or '''), this would be incorrect - it should add +3 instead. This could cause incorrect positioning when tracking attribute accesses in triple-quoted type annotations.
Consider checking the quote type by examining the source text at the node's position to determine the correct offset to add (1 for single/double quotes, 3 for triple quotes).
| # The col_offset of the string literal (after the quote) | |
| # For a string annotation like x: "models.User", the string | |
| # starts after the opening quote | |
| self._process_type_string( | |
| node.value, | |
| node.lineno, | |
| node.col_offset + 1, # +1 to skip the opening quote | |
| # The col_offset of the string literal points at the opening quote. | |
| # For a string annotation like x: "models.User", the string | |
| # content starts after the opening quote(s). Most literals use a | |
| # single quote character, but triple-quoted strings ("""...""" or | |
| # '''...''') use three, so we adjust the offset accordingly. | |
| opening_quote_len = 1 | |
| end_col = getattr(node, "end_col_offset", None) | |
| if end_col is not None: | |
| # Heuristic: a triple-quoted literal has four extra quote | |
| # characters compared to a single-quoted one (three opening + | |
| # three closing vs one opening + one closing). If the total | |
| # literal length is significantly larger than the value length, | |
| # treat it as likely triple-quoted and skip three characters. | |
| literal_len = end_col - node.col_offset | |
| if literal_len >= len(node.value) + 4: | |
| opening_quote_len = 3 | |
| self._process_type_string( | |
| node.value, | |
| node.lineno, | |
| node.col_offset + opening_quote_len, |
| collector = NameUsageCollector() | ||
| collector.visit(parsed) | ||
| self.used_names.update(collector.used_names) | ||
| except SyntaxError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except SyntaxError: | |
| except SyntaxError: | |
| # Invalid or non-type strings are expected here; ignore parse failures. |
| collector = NameUsageCollector() | ||
| collector.visit(parsed) | ||
| self.used_names.update(collector.used_names) | ||
| except SyntaxError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except SyntaxError: | |
| except SyntaxError: | |
| # Invalid type comment syntax; ignore and treat as having no type information. |
| self._extract_attrs_from_expr( | ||
| parsed.body, type_str, lineno, col_offset, context, | ||
| ) | ||
| except SyntaxError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except SyntaxError: | |
| except SyntaxError: | |
| # Ignore invalid type strings; type usage extraction is best-effort only. |
TypeStringAttrUsageto represent usages found in type comments and string annotations.CrossFileAnalyzerto collect and process these usages alongside regular code attribute accesses.This improvement aligns with Python's type hinting standards and enhances the tool's capability to manage indirect imports effectively.
Summary
Testing Process
Checklist
Closes #