Conversation
WalkthroughPR replaces a stub with AST-driven analysis: Changes
Sequence DiagramsequenceDiagram
participant User
participant BidirectIndex
participant FileSystem
participant AST
participant Detector
User->>BidirectIndex: value(filepath)
BidirectIndex->>FileSystem: read(filepath)
FileSystem-->>BidirectIndex: content
BidirectIndex->>AST: ast.parse(content)
AST-->>BidirectIndex: AST
BidirectIndex->>Detector: Detector().visit(AST)
rect rgb(235,245,255)
note over Detector: Traverse AST per function\ntrack declarations & ops, classify ops, detect bidirectional vars
Detector->>Detector: visit_FunctionDef (init context)
Detector->>Detector: _find_variable_declarations
Detector->>Detector: visit_AugAssign / visit_Assign / visit_UnaryOp (record ops)
Detector->>Detector: _get_*_operation_type (classify)
Detector->>Detector: _check_bidirectional_variables (aggregate results)
end
Detector-->>BidirectIndex: bidirect_vars() -> List[LineNumber]
BidirectIndex-->>User: results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 5-5: F401 'typing.Union' imported but unused
[error] 7-7: E302 expected 2 blank lines, found 1
[error] 27-27: W291 trailing whitespace
[error] 29-29: W291 trailing whitespace
[error] 33-33: W291 trailing whitespace
[error] 35-35: W291 trailing whitespace
[error] 44-44: W293 blank line contains whitespace
[error] 51-51: W293 blank line contains whitespace
[error] 54-54: W293 blank line contains whitespace
[error] 57-57: W291 trailing whitespace
[error] 60-60: W291 trailing whitespace
[error] 63-63: W291 trailing whitespace
[error] 65-65: W291 trailing whitespace
[error] 72-72: W293 blank line contains whitespace
[error] 73-73: W293 blank line contains whitespace
[error] 74-74: W293 blank line contains whitespace
[error] 87-87: W291 trailing whitespace
[error] 105-105: W291 trailing whitespace
[error] 106-106: E129 visually indented line with same indent as next logical line
[error] 106-106: W291 trailing whitespace
[error] 111-111: W291 trailing whitespace
[error] 114-114: W291 trailing whitespace
[error] 117-117: W291 trailing whitespace
[error] 119-119: W293 blank line contains whitespace
[error] 120-120: F821 undefined name 'LineNumber'
[error] 123-123: E302 expected 2 blank lines, found 1
[error] 127-127: W293 blank line contains whitespace
[error] 130-130: W293 blank line contains whitespace
🪛 GitHub Actions: make
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 120-120: NameError: name 'LineNumber' is not defined (occurs in BidirectIndexDetector.get_bidirect_variables annotation during pytest collection)
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[warning] 27-27: Trailing whitespace (trailing-whitespace)
[warning] 29-29: Trailing whitespace (trailing-whitespace)
[warning] 33-33: Trailing whitespace (trailing-whitespace)
[warning] 35-35: Trailing whitespace (trailing-whitespace)
[warning] 44-44: Trailing whitespace (trailing-whitespace)
[warning] 51-51: Trailing whitespace (trailing-whitespace)
[warning] 54-54: Trailing whitespace (trailing-whitespace)
[warning] 57-57: Trailing whitespace (trailing-whitespace)
[warning] 60-60: Trailing whitespace (trailing-whitespace)
[warning] 63-63: Trailing whitespace (trailing-whitespace)
[warning] 65-65: Trailing whitespace (trailing-whitespace)
[warning] 72-72: Trailing whitespace (trailing-whitespace)
[warning] 73-73: Trailing whitespace (trailing-whitespace)
[warning] 87-87: Trailing whitespace (trailing-whitespace)
[warning] 105-105: Trailing whitespace (trailing-whitespace)
[warning] 106-106: Trailing whitespace (trailing-whitespace)
[warning] 111-111: Trailing whitespace (trailing-whitespace)
[warning] 114-114: Trailing whitespace (trailing-whitespace)
[warning] 117-117: Trailing whitespace (trailing-whitespace)
[warning] 119-119: Trailing whitespace (trailing-whitespace)
[warning] 127-127: Trailing whitespace (trailing-whitespace)
[warning] 130-130: Trailing whitespace (trailing-whitespace)
[warning] 105-105: R1701: Consider merging these isinstance calls to isinstance(stmt, (ast.AnnAssign, ast.Assign, ast.AugAssign)) (consider-merging-isinstance)
[warning] 5-5: W0611: Unused Union imported from typing (unused-import)
🪛 GitHub Actions: ruff
aibolit/patterns/bidirect_index/bidirect_index.py
[warning] 5-5: F401 [*] typing.Union imported but unused
[warning] 83-83: Q000 [*] Double quotes found but single quotes preferred
[warning] 85-85: Q000 [*] Double quotes found but single quotes preferred
[warning] 90-90: Q000 [*] Double quotes found but single quotes preferred
[warning] 92-92: Q000 [*] Double quotes found but single quotes preferred
[warning] 98-98: Q000 [*] Double quotes found but single quotes preferred
[warning] 98-98: Q000 [*] Double quotes found but single quotes preferred
[error] 120-120: F821 Undefined name LineNumber
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 81-86: invalid-return-type: Return type does not match returned value. def _get_operation_type(self, op) -> str: may return None.
[error] 88-93: invalid-return-type: Return type does not match returned value. def _get_binop_operation_type(self, op) -> str: may return None.
[error] 118-118: invalid-return-type: Return type does not match returned value. def _check_bidirectional_variables(self, node): may return None while return type suggests int.
[error] 120-120: unresolved-reference: Name 'LineNumber' used when not defined in type annotation List[LineNumber].
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
103-118: Simplify isinstance check and remove trailing whitespace- if (isinstance(stmt, ast.Assign) or isinstance(stmt, ast.AnnAssign) or - isinstance(stmt, ast.AugAssign)): + if isinstance(stmt, (ast.Assign, ast.AnnAssign, ast.AugAssign)):Also trim trailing spaces on the
targets = [stmt.target]andreturn stmt.linenolines.
Note:ast.walkflattens nested scopes—verify that capturing declarations in inner functions or comprehensions is acceptable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
aibolit/patterns/bidirect_index/bidirect_index.py (10)
7-7: Add required blank line before class definition.PEP 8 requires two blank lines before top-level class definitions.
Apply this diff:
+ class BidirectIndex:
26-36: Remove trailing whitespace.Multiple lines contain trailing whitespace which violates PEP 8 and causes linter errors.
Apply this diff:
if not os.path.exists(filename): - return [] + return [] with open(filename, 'r', encoding='utf-8') as file: - source_code = file.read() + source_code = file.read() try: tree = ast.parse(source_code) except SyntaxError: - return [] + return [] detector = BidirectIndexDetector() - detector.visit(tree) + detector.visit(tree) return detector.get_bidirect_variables()
38-49: Fix class spacing and trailing whitespace.Multiple formatting issues need to be addressed: missing blank line before class definition and trailing whitespace.
Apply this diff:
+ class LineNumber: def __init__(self, line: int, variable: str): self.line = line self.variable = variable - + def __repr__(self): return f"LineNumber(line={self.line}, variable='{self.variable}')" - + def __eq__(self, other): if not isinstance(other, LineNumber): return False return self.line == other.line and self.variable == other.variable
51-56: Add required blank line and remove trailing whitespace.PEP 8 requires two blank lines before top-level class definitions, and line 56 contains trailing whitespace.
Apply this diff:
+ class BidirectIndexDetector(ast.NodeVisitor): def __init__(self): self.bidirect_variables: List[LineNumber] = [] self.current_method: str = None self.method_operations: Dict[str, Dict[str, Set[str]]] = {} - +
57-63: Remove trailing whitespace.Line 63 contains trailing whitespace.
Apply this diff:
self._check_bidirectional_variables(node) self.current_method = None - +
64-66: Enable AST traversal for class bodies.The empty
visit_ClassDefoverride prevents traversal into class definitions, so methods inside classes won't be analyzed for bidirectional index patterns. This significantly limits the pattern detector's effectiveness.Apply this diff to enable class body traversal:
def visit_ClassDef(self, node): - pass - + self.generic_visit(node) +
67-77: Remove trailing whitespace.Multiple lines contain trailing whitespace.
Apply this diff:
def visit_AugAssign(self, node): if self.current_method is None: - return + return if isinstance(node.target, ast.Name): var_name = node.target.id - operation = self._get_operation_type(node.op) + operation = self._get_operation_type(node.op) if operation: if var_name not in self.method_operations[self.current_method]: - self.method_operations[self.current_method][var_name] = set() + self.method_operations[self.current_method][var_name] = set() self.method_operations[self.current_method][var_name].add(operation) - +
78-91: Remove trailing whitespace and fix indentation.Multiple lines contain trailing whitespace, and line 86 has incorrect visual indentation.
Apply this diff:
def visit_Assign(self, node): if self.current_method is None: return for target in node.targets: if isinstance(target, ast.Name): var_name = target.id - if (isinstance(node.value, ast.BinOp) and - isinstance(node.value.left, ast.Name) and - node.value.left.id == var_name): + if (isinstance(node.value, ast.BinOp) and + isinstance(node.value.left, ast.Name) and + node.value.left.id == var_name): operation = self._get_binop_operation_type(node.value.op) if operation: if var_name not in self.method_operations[self.current_method]: self.method_operations[self.current_method][var_name] = set() self.method_operations[self.current_method][var_name].add(operation)
93-99: Fix return type annotation.The method can return
Nonebut is annotated to returnstr, causing type checking errors.Apply this diff:
-def _get_operation_type(self, op) -> str: +def _get_operation_type(self, op) -> str | None: if isinstance(op, ast.Add): return 'increment' elif isinstance(op, ast.Sub): return 'decrement' return None - +
100-106: Fix return type annotation and eliminate code duplication.The method has two issues:
- It can return
Nonebut is annotated to returnstr(type checking error)- It duplicates the logic of
_get_operation_typeentirelyApply this diff to fix both issues:
-def _get_binop_operation_type(self, op) -> str: - if isinstance(op, ast.Add): - return 'increment' - elif isinstance(op, ast.Sub): - return 'decrement' - return None +def _get_binop_operation_type(self, op) -> str | None: + return self._get_operation_type(op)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 7-7: E302 expected 2 blank lines, found 1
[error] 27-27: W291 trailing whitespace
[error] 29-29: W291 trailing whitespace
[error] 33-33: W291 trailing whitespace
[error] 35-35: W291 trailing whitespace
[error] 38-38: E302 expected 2 blank lines, found 1
[error] 42-42: W293 blank line contains whitespace
[error] 45-45: W293 blank line contains whitespace
[error] 51-51: E302 expected 2 blank lines, found 1
[error] 56-56: W293 blank line contains whitespace
[error] 63-63: W293 blank line contains whitespace
[error] 66-66: W293 blank line contains whitespace
[error] 69-69: W291 trailing whitespace
[error] 72-72: W291 trailing whitespace
[error] 75-75: W291 trailing whitespace
[error] 77-77: W293 blank line contains whitespace
[error] 84-84: W291 trailing whitespace
[error] 85-85: W291 trailing whitespace
[error] 86-86: E129 visually indented line with same indent as next logical line
[error] 99-99: W293 blank line contains whitespace
[error] 117-117: W291 trailing whitespace
[error] 118-118: E129 visually indented line with same indent as next logical line
[error] 118-118: W291 trailing whitespace
[error] 123-123: W291 trailing whitespace
[error] 126-126: W291 trailing whitespace
[error] 129-129: W291 trailing whitespace
[error] 131-131: W293 blank line contains whitespace
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 27-27: Trailing whitespace (trailing-whitespace)
[error] 29-29: Trailing whitespace (trailing-whitespace)
[error] 33-33: Trailing whitespace (trailing-whitespace)
[error] 35-35: Trailing whitespace (trailing-whitespace)
[error] 42-42: Trailing whitespace (trailing-whitespace)
[error] 45-45: Trailing whitespace (trailing-whitespace)
[error] 56-56: Trailing whitespace (trailing-whitespace)
[error] 63-63: Trailing whitespace (trailing-whitespace)
[error] 66-66: Trailing whitespace (trailing-whitespace)
[error] 69-69: Trailing whitespace (trailing-whitespace)
[error] 72-72: Trailing whitespace (trailing-whitespace)
[error] 75-75: Trailing whitespace (trailing-whitespace)
[error] 77-77: Trailing whitespace (trailing-whitespace)
[error] 84-84: Trailing whitespace (trailing-whitespace)
[error] 85-85: Trailing whitespace (trailing-whitespace)
[error] 99-99: Trailing whitespace (trailing-whitespace)
[warning] 117-117: R1701: Consider merging these isinstance calls to isinstance(stmt, (ast.AnnAssign, ast.Assign, ast.AugAssign)) (consider-merging-isinstance)
[error] 118-118: Trailing whitespace (trailing-whitespace)
[error] 123-123: Trailing whitespace (trailing-whitespace)
[error] 126-126: Trailing whitespace (trailing-whitespace)
[error] 129-129: Trailing whitespace (trailing-whitespace)
[error] 131-131: Trailing whitespace (trailing-whitespace)
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 93-99: invalid-return-type: Return type does not match returned value. def _get_operation_type(self, op) -> str: Expected str because of return type, but returns None.
[error] 100-106: invalid-return-type: Return type does not match returned value. def _get_binop_operation_type(self, op) -> str: Expected str because of return type, but returns None.
[error] 130-131: invalid-return-type: Return type does not match returned value. return None. Expected int because of return type in _find_variable_declaration.
[error] 115-116: invalid-return-type: Return type does not match returned value. def _find_variable_declaration(self, node, var_name: str) -> int: Expected int because of return type.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
107-113: LGTM!The logic correctly identifies variables that have both increment and decrement operations within the same method and records their declaration lines.
132-133: LGTM!The getter method correctly returns the collected bidirectional variables.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
67-69: Do not stop traversal atClassDef.Reintroducing an empty
visit_ClassDefmeans the visitor never descends into class bodies, so every method defined inside a class (the common case for “methods”) is ignored. Bidirectional index detection therefore fails for class methods.Call
self.generic_visit(node)(or remove the override) so class members are analyzed.def visit_ClassDef(self, node): - pass + self.generic_visit(node)
118-122: Merge the repeatedisinstancechecks.Pylint fails the pipeline (R1701) because the three
isinstancecalls are not combined. Rewrite the condition to use a single tuple-based check.- for stmt in ast.walk(node): - if (isinstance(stmt, ast.Assign) or isinstance(stmt, ast.AnnAssign) or - isinstance(stmt, ast.AugAssign)): + for stmt in ast.walk(node): + if isinstance(stmt, (ast.Assign, ast.AnnAssign, ast.AugAssign)):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 120-120: pylint: R1701: Consider merging these isinstance calls to isinstance(stmt, (ast.AnnAssign, ast.Assign, ast.AugAssign)) (consider-merging-isinstance)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: latexmk
- GitHub Check: make
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
60-65: Nested function handling still loses outer context.A previous review comment correctly identified that nested
FunctionDefnodes cause the outer function context to be lost. When the inner function exits,current_methodis set toNone, so any operations after the nested function in the outer scope are ignored.Apply the previously suggested fix to preserve context:
def visit_FunctionDef(self, node): + previous_method = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.generic_visit(node) self._check_bidirectional_variables(node) - self.current_method = None + self.current_method = previous_method
🧹 Nitpick comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
96-108: Eliminate duplicate code.The methods
_operation_typeand_binop_operation_typehave identical implementations.Apply this diff to eliminate duplication:
def _binop_operation_type(self, op) -> str | None: - if isinstance(op, ast.Add): - return 'increment' - elif isinstance(op, ast.Sub): - return 'decrement' - return None + return self._operation_type(op)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 37-37: pylint: W0212 Access to a protected member _bidirect_variables of a client class (protected-access).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (5)
aibolit/patterns/bidirect_index/bidirect_index.py (5)
40-51: LGTM!The
LineNumberclass is well-implemented with proper equality and string representation methods.
67-68: Verify that skipping class bodies is intentional.The empty
visit_ClassDefmethod prevents the AST visitor from descending into class definitions, meaning methods inside classes won't be analyzed. A previous review comment suggested enabling traversal by callingself.generic_visit(node)and was marked as addressed, but the code remains unchanged.If methods inside classes should be analyzed, apply this diff:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)If skipping class bodies is intentional (e.g., the pattern only applies to module-level functions), please clarify the design decision.
70-79: LGTM!The augmented assignment handling correctly tracks increment/decrement operations per variable.
81-94: LGTM!The assignment handling correctly identifies self-modifying patterns like
x = x + 1.
118-132: Review the fallback declaration logic.Lines 129-131 return the first occurrence of any
ast.Namenode matching the variable name if no explicit assignment is found. This fallback may not represent the actual declaration and could return a usage rather than a definition.Consider whether this fallback is needed or if only explicit declarations should be returned:
for stmt in ast.walk(node): if isinstance(stmt, ast.Name) and stmt.id == var_name: return stmt.lineno - return None + return None # Only return explicit declarations, not arbitrary usagesIf the fallback is intentional to handle edge cases (e.g., function parameters), please clarify the design rationale.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
aibolit/patterns/bidirect_index/bidirect_index.py (3)
129-131: Consider removing the fallback logic.The fallback returns the first occurrence of any
ast.Namenode with matching identifier, which may be a reference in an expression rather than a declaration. This could lead to incorrect line numbers being reported.If variables without explicit declarations should be ignored, remove the fallback:
for stmt in ast.walk(node): if isinstance(target, ast.Name) and target.id == var_name: return stmt.lineno - for stmt in ast.walk(node): - if isinstance(stmt, ast.Name) and stmt.id == var_name: - return stmt.lineno return NoneOtherwise, document why returning the first usage is acceptable for this pattern detector.
60-65: Fix nested function context loss.The current implementation resets
self.current_method = Noneafter visiting a function, which causes operations in the outer function (after a nested function definition) to be skipped becausevisit_AugAssignandvisit_Assignreturn early whencurrent_method is None.Apply this diff to preserve the outer function context:
def visit_FunctionDef(self, node): + previous_method = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.generic_visit(node) self._check_bidirectional_variables(node) - self.current_method = None + self.current_method = previous_methodBased on past review comments.
67-68: Enable traversal of class bodies.The empty
visit_ClassDefprevents the visitor from descending into class definitions, so methods and nested functions inside classes won't be analyzed.Apply this diff to continue traversing class bodies:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)This allows the detector to analyze methods within classes (via
visit_FunctionDef) while maintaining the current per-function tracking design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (8)
aibolit/patterns/bidirect_index/bidirect_index.py (8)
4-5: LGTM!The imports are clean and appropriately scoped for AST traversal and type annotations.
27-37: LGTM!The implementation correctly handles file reading, AST parsing with error recovery, and detector invocation.
40-51: LGTM!The
LineNumberclass is well-structured with appropriate equality and string representation methods.
54-59: LGTM!The initialization correctly sets up the data structures needed for tracking operations per method.
70-79: LGTM!The augmented assignment handling correctly identifies increment/decrement operations and tracks them per variable.
81-94: LGTM!The assignment pattern detection correctly identifies self-referential increment/decrement operations like
x = x + 1.
110-116: LGTM!The bidirectional variable detection correctly identifies variables with both increment and decrement operations.
134-135: LGTM!The public API method correctly returns the detected bidirectional variables.
|
@yegor256 please check PR |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
67-68: Enable traversal into class bodies.The empty
visit_ClassDefprevents the visitor from descending into class definitions, so methods inside classes are never analyzed. This will miss bidirectional index patterns in class methods.Based on past review comments.
Apply this diff to enable class body traversal:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)
60-65: Restore outer method context after visiting nested functions.When a nested function is visited, line 65 sets
self.current_method = None. After returning to the outer function, subsequent statements execute withcurrent_method is None, causing their operations to be skipped (see lines 71-72 and 82-83). This produces false negatives for bidirectional variables when operations appear after nested function definitions.Based on past review comments.
Apply this diff to preserve the outer method context:
def visit_FunctionDef(self, node): + previous_method = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.generic_visit(node) self._check_bidirectional_variables(node) - self.current_method = None + self.current_method = previous_method
🧹 Nitpick comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
103-104: Consider removing redundant wrapper method.The
_binop_operation_typemethod is now just a one-line delegation to_operation_type. You could simplify by callingself._operation_type(node.value.op)directly on line 90.
114-128: Refine _find_variable_declaration fallback logic.The fallback (lines 125–127) returns the first use of
var_name, not its declaration, leading to incorrect line numbers (e.g., for parameters). Consider handlingast.FunctionDef.argsexplicitly or removing this fallback so only real declarations are reported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
aibolit/ast_framework/ast_node.py (1)
line(41-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: make
- GitHub Check: latexmk
🔇 Additional comments (9)
aibolit/patterns/bidirect_index/bidirect_index.py (9)
1-5: LGTM!The imports are minimal and all necessary for the AST-based implementation.
13-37: LGTM!The implementation correctly handles edge cases (missing files, syntax errors) and follows a clean workflow: parse AST → run detector → return results.
40-51: LGTM!The
LineNumberclass is well-implemented with proper__repr__for debugging and__eq__for comparison.
54-58: LGTM!The data structures are well-designed to track per-method variable operations and results.
70-79: LGTM!Correctly handles augmented assignments (
x += 1,x -= 1) within method scope.
81-94: LGTM!Correctly identifies the pattern
x = x + 1andx = x - 1within method scope.
96-101: LGTM!Correctly maps AST operators to increment/decrement operations.
106-112: LGTM!Correctly identifies variables that have both increment and decrement operations within the same method.
130-131: LGTM!Simple getter method to retrieve the collected bidirectional variables.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
60-65: Restore context after visiting nested functions.When a nested function is visited,
self.current_methodis set to the inner function's name, then cleared toNoneon exit. This causes the outer function to lose its context—any statements after the nested function execute withcurrent_method is None, so their operations are ignored. This produces false negatives for bidirectional variables that have operations both before and after nested functions.Apply this diff to preserve the outer function's context:
def visit_FunctionDef(self, node): + previous_method = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.generic_visit(node) self._check_bidirectional_variables(node) - self.current_method = None + self.current_method = previous_methodBased on past review comments.
67-68: Enable traversal into class bodies.The empty
visit_ClassDefoverride prevents the visitor from descending into class definitions. This means methods defined inside classes are never analyzed for bidirectional index patterns, significantly limiting the detector's scope.Apply this diff:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ruff
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 57-57: ruff: Q000 - Double quotes found but single quotes preferred.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: make
- GitHub Check: latexmk
🔇 Additional comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
125-127: The fallback behavior can incorrectly identify variable references as declarations.The concern is valid. Lines 125-127 use
ast.walk(node)which traverses all descendant nodes and will return the firstast.Namematchingvar_name, including references (not just declarations). For example, if a variable is compared or used in a function call before any assignment, the fallback would incorrectly report that reference line.However, this code is currently incomplete (all tests are skipped with "Not implemented" marker on line 24). Since there is no test coverage and no production usage, either:
- Remove the fallback entirely if only explicit assignments should be reported, or
- Replace it with explicit parameter search if function parameters need to be included (as suggested in the review).
Without clarity on the intended behavior, recommend removing the problematic fallback and adding a comment explaining why only explicit assignments (lines 118-124) are reported.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
60-65: Restorecurrent_methodafter nested function traversal.Nested
FunctionDefnodes setself.current_methodto the inner name and reset it toNoneon exit. When control returns to the outer function, subsequent statements execute withcurrent_method is None, so their operations are skipped. The detector therefore misses increments/decrements that appear after an inner function, producing false negatives.Apply this diff to preserve the previous method context:
def visit_FunctionDef(self, node): + previous_method = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.generic_visit(node) self._check_bidirectional_variables(node) - self.current_method = None + self.current_method = previous_methodBased on past review comments.
67-68: Enable AST traversal for class bodies.The empty
visit_ClassDefoverride stops descent into class definitions, so methods inside classes won't be analyzed. This means the detector will miss bidirectional variables in class methods entirely.Apply this diff to continue visiting class members:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)Based on past review comments.
🧹 Nitpick comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
125-128: Reconsider the fallback logic for finding variable declarations.The fallback logic (lines 125-128) returns the line of the first
ast.Namenode matching the variable name, which may be a usage rather than a declaration. This could lead to incorrect line numbers being reported.Consider whether this fallback is necessary or should be removed to only report variables with explicit declarations:
for stmt in ast.walk(node): if isinstance(stmt, (ast.AnnAssign, ast.Assign, ast.AugAssign)): targets = [] if isinstance(stmt, ast.Assign): targets = stmt.targets elif isinstance(stmt, (ast.AnnAssign, ast.AugAssign)): targets = [stmt.target] for target in targets: if isinstance(target, ast.Name) and target.id == var_name: return stmt.lineno - for stmt in ast.walk(node): - if isinstance(stmt, ast.Name) and stmt.id == var_name: - return stmt.lineno return NoneThis ensures that only actual declarations are reported, avoiding confusion when a variable is first used before being assigned.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 46-46: SyntaxError: invalid syntax reported by flake8 at line 46:58 (E999).
[error] 46-46: flake8: E999 SyntaxError: invalid syntax.
🪛 GitHub Actions: make
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 46-46: SyntaxError: invalid syntax due to incorrect string literal formatting in return expression: return f'LineNumber(line={self.line}, variable='{self.variable}')'
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 46-46: Parsing failed: 'invalid syntax (aibolit.patterns.bidirect_index.bidirect_index, line 46)' (syntax-error)
🪛 GitHub Actions: ruff
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 46-46: invalid-syntax: Simple statements must be separated by newlines or semicolons
[error] 46-46: invalid-syntax: Syntax error near f-string due to mismatched quotes: return f'LineNumber(line={self.line}, variable='{self.variable}')'
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 46-46: Invalid syntax in f-string: nested quotes in return expression 'return f'LineNumber(line={self.line}, variable='{self.variable}')''.
[error] 46-46: SyntaxError: invalid syntax at line 46 due to unmatched quotes inside f-string around self.variable.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (3)
aibolit/patterns/bidirect_index/bidirect_index.py (3)
1-5: LGTM!The new imports (
astandListfromtyping) are necessary for the AST-based implementation and are used correctly throughout the file.
27-37: LGTM!The implementation correctly replaces the stub with AST-based analysis. Error handling for missing files and syntax errors is appropriate, returning an empty list in both cases.
57-57: Fix quote style to match project convention.Line 57 uses double quotes but the project enforces single quotes per the ruff Q000 rule.
Apply this diff:
- self.current_method = '' + self.current_method = NoneNote: Using
Noneinstead of an empty string is more semantically correct for "no method context" and matches the check on lines 71 and 82.Based on past review comments.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
125-128: Fallback may return usage instead of declaration.The fallback loop returns the first
ast.Nameoccurrence, which may be a reference rather than a declaration. While this provides a line number when no explicit assignment is found, it may be semantically incorrect.Consider removing the fallback to only report variables with clear declarations:
for stmt in ast.walk(node): if isinstance(stmt, (ast.AnnAssign, ast.Assign, ast.AugAssign)): targets = [] if isinstance(stmt, ast.Assign): targets = stmt.targets elif isinstance(stmt, (ast.AnnAssign, ast.AugAssign)): targets = [stmt.target] for target in targets: if isinstance(target, ast.Name) and target.id == var_name: return stmt.lineno - for stmt in ast.walk(node): - if isinstance(stmt, ast.Name) and stmt.id == var_name: - return stmt.lineno return NoneThis ensures only true declarations are reported.
🧹 Nitpick comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
103-104: Consider eliminating the wrapper method.Since
_binop_operation_typesimply delegates to_operation_type, you could call_operation_typedirectly fromvisit_Assign(line 90) and remove this method entirely.Apply this diff to remove the wrapper:
- def _binop_operation_type(self, op) -> str | None: - return self._operation_type(op) - def _check_bidirectional_variables(self, node):And update line 90:
- operation = self._binop_operation_type(node.value.op) + operation = self._operation_type(node.value.op)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: make
- GitHub Check: latexmk
🔇 Additional comments (8)
aibolit/patterns/bidirect_index/bidirect_index.py (8)
27-37: LGTM! Well-structured AST parsing with proper error handling.The implementation correctly handles file existence, syntax errors, and delegates to the detector. The use of explicit encoding is good practice.
40-51: LGTM! Clean data class implementation.The
LineNumberclass properly implements__repr__and__eq__, enabling meaningful comparison and debugging output.
55-58: LGTM! Proper initialization.The detector state is correctly initialized for tracking bidirectional variables across methods.
70-79: LGTM! Correct handling of augmented assignments.The method properly tracks
+=and-=operations for variables, correctly initializing the operation set when first encountering each variable.
81-94: LGTM! Correctly detectsx = x ± npatterns.The method properly identifies regular assignments that increment or decrement a variable (e.g.,
x = x + 1), correctly filtering for cases where the assignment target matches the left operand of the binary operation.
96-101: LGTM! Clean operation type mapping.The method correctly classifies addition as increment and subtraction as decrement, with proper type annotation.
106-112: LGTM! Correct bidirectional pattern detection.The method properly identifies variables that have both increment and decrement operations within the same function, recording their declaration line.
130-131: LGTM! Simple accessor method.The public method correctly exposes the collected results with proper type annotation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/patterns/bidirect_index/test_bidirect_index.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/patterns/bidirect_index/test_bidirect_index.py (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
BidirectIndex(8-37)
🪛 GitHub Actions: make
test/patterns/bidirect_index/test_bidirect_index.py
[error] 8-8: ImportError: cannot import name 'BidirectIndex' from 'aibolit.patterns.bidirect_index' during test collection. Ensure BidirectIndex is exported in the module.
🪛 GitHub Actions: pylint
test/patterns/bidirect_index/test_bidirect_index.py
[error] 8-8: E0611: No name 'BidirectIndex' in module 'aibolit.patterns.bidirect_index' (no-name-in-module).
🪛 GitHub Actions: ty
test/patterns/bidirect_index/test_bidirect_index.py
[error] 8-8: unresolved-import: Module 'aibolit.patterns.bidirect_index' has no member 'BidirectIndex' (import error).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
| from unittest import TestCase | ||
| import unittest | ||
| from aibolit.patterns.bidirect_index.bidirect_index import BidirectIndex | ||
| from aibolit.patterns.bidirect_index import BidirectIndex |
There was a problem hiding this comment.
Fix the missing export in __init__.py.
The import is failing because BidirectIndex is not exported from the aibolit.patterns.bidirect_index package. You need to add the export to aibolit/patterns/bidirect_index/__init__.py.
Add this to aibolit/patterns/bidirect_index/__init__.py:
from aibolit.patterns.bidirect_index.bidirect_index import BidirectIndex
__all__ = ['BidirectIndex']🧰 Tools
🪛 GitHub Actions: make
[error] 8-8: ImportError: cannot import name 'BidirectIndex' from 'aibolit.patterns.bidirect_index' during test collection. Ensure BidirectIndex is exported in the module.
🪛 GitHub Actions: pylint
[error] 8-8: E0611: No name 'BidirectIndex' in module 'aibolit.patterns.bidirect_index' (no-name-in-module).
🪛 GitHub Actions: ty
[error] 8-8: unresolved-import: Module 'aibolit.patterns.bidirect_index' has no member 'BidirectIndex' (import error).
🤖 Prompt for AI Agents
In test/patterns/bidirect_index/test_bidirect_index.py around line 8, the test
import fails because BidirectIndex is not exported from the package; fix
aibolit/patterns/bidirect_index/__init__.py by importing the class from
aibolit.patterns.bidirect_index.bidirect_index and adding it to __all__ (i.e.,
add a from ... import BidirectIndex line and set __all__ = ['BidirectIndex'] so
the symbol is exported).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
60-65: Preserve outer function context during nested function traversal.Setting
current_method = Noneafter visiting a nested function causes the outer function's context to be lost. Subsequent operations in the outer function will be skipped (lines 71-72, 82-83 checkif self.current_method is None), resulting in false negatives.For example:
def outer(): x = 0 x += 1 # tracked def inner(): pass # current_method is now None x -= 1 # NOT tracked - false negativeApply this diff to preserve the outer context:
def visit_FunctionDef(self, node): + previous_method = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.generic_visit(node) self._check_bidirectional_variables(node) - self.current_method = None + self.current_method = previous_method
67-68: Restore AST traversal for class bodies.The empty
visit_ClassDefprevents the visitor from descending into class bodies. Any methods defined inside classes will not be analyzed for bidirectional index patterns. If this was intentional, document the limitation in the method docstring (lines 14-26).Apply this diff to enable class method analysis:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: flake8 check failed: SyntaxError: invalid syntax (E999) at line 55. Command 'flake8 --max-line-length=120 .' exited with code 1.
🪛 GitHub Actions: make
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: SyntaxError: invalid syntax in init signature. Likely due to incorrect type annotations in 'def init(self, vars=[]: List, method='': str, ops={}: Dict)'.
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: Parsing failed: 'invalid syntax (aibolit.patterns.bidirect_index.bidirect_index, line 55)' (syntax-error)
🪛 GitHub Actions: ruff
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: invalid-syntax: Expected ',', found ':'
[error] 55-55: invalid-syntax: Parameter without a default cannot follow a parameter with a default
[error] 55-55: invalid-syntax: Expected ',', found ':'
[error] 55-55: invalid-syntax: Parameter without a default cannot follow a parameter with a default
[error] 55-55: invalid-syntax: Expected ',', found ':'
[error] 55-55: invalid-syntax: Parameter without a default cannot follow a parameter with a default
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 35-35: Step 'uvx ty==0.0.1-alpha.8 check' failed. missing-argument: No arguments provided for required parameters List, str, Dict of bound method __init__.
[error] 55-55: Step 'uvx ty==0.0.1-alpha.8 check' failed. invalid-syntax: def init(self, vars=[]: List, method='': str, ops={}: Dict): Expected ',', found ':'
[error] 55-55: Step 'uvx ty==0.0.1-alpha.8 check' failed. invalid-syntax: def init(self, vars=[]: List, method='': str, ops={}: Dict): Parameter without a default cannot follow a parameter with a default
[error] 55-55: Step 'uvx ty==0.0.1-alpha.8 check' failed. invalid-syntax: def init(self, vars=[]: List, method='': str, ops={}: Dict): Expected ',', found ':'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (6)
aibolit/patterns/bidirect_index/bidirect_index.py (6)
40-51: LGTM!The
LineNumberclass is well-implemented with proper initialization, representation, and equality methods. The f-string syntax is correct.
70-79: LGTM!The
visit_AugAssignmethod correctly identifies augmented assignments (+=,-=) and tracks increment/decrement operations for variables.
81-94: LGTM!The
visit_Assignmethod correctly identifies assignment patterns likex = x + 1and tracks them as increment/decrement operations.
96-104: LGTM!The
_operation_typemethod correctly maps AST operation types to increment/decrement strings, and_binop_operation_typeappropriately delegates to it, eliminating code duplication.
106-112: LGTM!The
_check_bidirectional_variablesmethod correctly identifies variables that have both increment and decrement operations and records their declaration lines.
130-131: LGTM!The
bidirect_varsmethod correctly returns the list of detected bidirectional variables.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
27-37: Critical: Python AST parser cannot parse Java source files.The implementation uses Python's
ast.parse()to analyze source files, but the test files intest/patterns/bidirect_index/are Java files (as evidenced by past review comments mentioningBidirectIndexIncreaseDecrease.java). Python's AST parser will always raise aSyntaxErrorwhen attempting to parse Java syntax, causing this method to return an empty list for all Java files.Other patterns in the codebase use the
javalanglibrary for Java analysis. Apply this diff to use the correct parser:import os import ast +import javalang from typing import List, DictThen update the
valuemethod:def value(self, filename: str | os.PathLike): """ Finds if a variable is being incremented and decremented within the same method :param filename: filename to be analyzed :return: list of LineNumber with the variable declaration lines - - @todo #139:30min Implement bidirect index pattern - If the same numeric variable is incremented and decremented within the same method, - it's a pattern. A numeric variable should either be always growing or decreasing. - Bi-directional index is confusing. The method must return a list with the line numbers - of the variables that match this pattern. After implementation, activate tests in - test_bidirect_index.py """ if not os.path.exists(filename): return [] with open(filename, 'r', encoding='utf-8') as file: source_code = file.read() try: - tree = ast.parse(source_code) + tree = javalang.parse.parse(source_code) except SyntaxError: return [] - detector = BidirectIndexDetector([], '', {}) - detector.visit(tree) + detector = BidirectIndexDetector() + # Implement Java AST traversal logic here return detector.bidirect_vars()Note: The
BidirectIndexDetectorclass also needs to be rewritten to work withjavalangAST nodes instead of Python'sastmodule. The current implementation (inheriting fromast.NodeVisitor) is incompatible with Java parsing.
67-68: Enable traversal into class bodies.The empty
visit_ClassDefmethod prevents the AST visitor from descending into class definitions, meaning methods defined inside classes will not be analyzed for bidirectional index patterns.Apply this diff:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)If skipping class bodies is intentional, document this limitation in the class docstring (lines 14-26).
Based on past review comments.
🧹 Nitpick comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
103-104: Consider removing redundant method.The
_binop_operation_typemethod simply delegates to_operation_typewithout adding any functionality. Consider removing it and calling_operation_typedirectly at the call site (line 90).Apply this diff:
node.value.left.id == var_name): - operation = self._binop_operation_type(node.value.op) + operation = self._operation_type(node.value.op) if operation:Then remove the redundant method:
- def _binop_operation_type(self, op) -> str | None: - return self._operation_type(op) -Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: flake8: SyntaxError: non-default argument follows default argument in ./aibolit/patterns/bidirect_index/bidirect_index.py:55.
🪛 GitHub Actions: make
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: SyntaxError: non-default argument follows default argument in init signature: def init(self, vars: List, method: str = '', ops: Dict):
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: Parsing failed: non-default argument follows default argument (syntax error in bidirect_index.py).
🪛 GitHub Actions: ruff
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: invalid-syntax: Parameter without a default cannot follow a parameter with a default
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 55-55: Syntax error: init parameter list has a non-default parameter after a default parameter in 'def init(self, vars: List, method: str = '', ops: Dict):'. Parameter 'ops' has no default following a default parameter 'method'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (3)
aibolit/patterns/bidirect_index/bidirect_index.py (3)
40-51: LGTM: Well-structured data class.The
LineNumberclass correctly encapsulates line and variable information with proper__repr__and__eq__implementations for debugging and comparison.
70-94: LGTM: Correct operation tracking logic.The implementation correctly identifies both augmented assignments (
x += 1,x -= 1) and regular assignments with binary operations (x = x + 1,x = x - 1). The early returns whencurrent_method is Noneare appropriate guards.Note: Effectiveness depends on fixing the context preservation in
visit_FunctionDef(lines 60-65) and enabling class traversal invisit_ClassDef(lines 67-68).
130-131: LGTM: Clean accessor method.The
bidirect_varsmethod provides proper public access to the detected bidirectional variables.
| def __init__(self, vars: List, method: str = '', ops: Dict): | ||
| self.bidirect_variables = vars | ||
| self.current_method = method | ||
| self.method_operations = ops |
There was a problem hiding this comment.
Fix parameter order and use proper defaults for mutable types.
The __init__ signature has a critical syntax error: non-default parameter ops cannot follow default parameter method. Additionally, the mutable parameters should use None as defaults and be initialized inside the method to avoid shared state across instances.
Apply this diff:
- def __init__(self, vars: List, method: str = '', ops: Dict):
+ def __init__(self, vars: List = None, method: str = '', ops: Dict = None):
- self.bidirect_variables = vars
+ self.bidirect_variables = vars if vars is not None else []
self.current_method = method
- self.method_operations = ops
+ self.method_operations = ops if ops is not None else {}And update the call site at line 35:
- detector = BidirectIndexDetector([], '', {})
+ detector = BidirectIndexDetector()🧰 Tools
🪛 GitHub Actions: flake8
[error] 55-55: flake8: SyntaxError: non-default argument follows default argument in ./aibolit/patterns/bidirect_index/bidirect_index.py:55.
🪛 GitHub Actions: make
[error] 55-55: SyntaxError: non-default argument follows default argument in init signature: def init(self, vars: List, method: str = '', ops: Dict):
🪛 GitHub Actions: pylint
[error] 55-55: Parsing failed: non-default argument follows default argument (syntax error in bidirect_index.py).
🪛 GitHub Actions: ruff
[error] 55-55: invalid-syntax: Parameter without a default cannot follow a parameter with a default
🪛 GitHub Actions: ty
[error] 55-55: Syntax error: init parameter list has a non-default parameter after a default parameter in 'def init(self, vars: List, method: str = '', ops: Dict):'. Parameter 'ops' has no default following a default parameter 'method'.
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 55 to 58, the
__init__ signature places a non-default parameter (ops) after a default
parameter (method) and uses mutable default values; change the signature so all
mutable defaults are None and initialized inside the method (e.g., vars -> None,
ops -> None) and ensure parameter ordering is valid (either move method to the
end or make ops optional) — then inside __init__ set self.bidirect_variables =
vars or [] and self.method_operations = ops or {} and keep self.current_method =
method; also update the call site at line 35 to pass the new parameters either
by position matching the new order or, preferably, by using keyword arguments
(method=..., ops=...) to avoid breaking behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
114-128: Fallback logic may return usage instead of declaration.The fallback logic (lines 125-127) returns the first occurrence of any
ast.Namenode matching the variable, which may be a usage site rather than the actual declaration line. This could lead to incorrect line numbers in results.Consider removing the fallback to only return explicit declarations:
for stmt in ast.walk(node): if isinstance(stmt, (ast.AnnAssign, ast.Assign, ast.AugAssign)): targets = [] if isinstance(stmt, ast.Assign): targets = stmt.targets elif isinstance(stmt, (ast.AnnAssign, ast.AugAssign)): targets = [stmt.target] for target in targets: if isinstance(target, ast.Name) and target.id == var_name: return stmt.lineno - for stmt in ast.walk(node): - if isinstance(stmt, ast.Name) and stmt.id == var_name: - return stmt.lineno return NoneAlternatively, if the fallback is intentional, add a comment explaining why returning the first usage is acceptable.
67-68: Methods inside classes are not analyzed.A past review comment flagged this issue as addressed, but the code still contains only
pass, which prevents the AST visitor from traversing into class bodies. Any methods defined inside classes will not be analyzed for bidirectional index patterns.Apply this diff to enable class traversal:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)If skipping class bodies is intentional, add a docstring to the
BidirectIndexDetectorclass explaining that only top-level functions are processed.
🧹 Nitpick comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
20-25: Remove the outdated TODO comment.The bidirectional index pattern has now been implemented, so this TODO is obsolete.
Apply this diff to remove the outdated comment:
- @todo #139:30min Implement bidirect index pattern - If the same numeric variable is incremented and decremented within the same method, - it's a pattern. A numeric variable should either be always growing or decreasing. - Bi-directional index is confusing. The method must return a list with the line numbers - of the variables that match this pattern. After implementation, activate tests in - test_bidirect_index.py
55-58: Consider using None defaults for better initialization.While the current code works, passing empty mutable objects
[], '', {}from the caller (line 35) is unusual. The constructor could useNonedefaults and initialize internally to avoid requiring callers to pass empty collections.Apply this diff:
- def __init__(self, vars: List, method: str, ops: Dict): - self.bidirect_variables = vars - self.current_method = method - self.method_operations = ops + def __init__(self, vars: List = None, method: str = '', ops: Dict = None): + self.bidirect_variables = vars if vars is not None else [] + self.current_method = method + self.method_operations = ops if ops is not None else {}Then update line 35:
- detector = BidirectIndexDetector([], '', {}) + detector = BidirectIndexDetector()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: make
- GitHub Check: latexmk
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
68-69: Enable class body traversal or document the limitation.The empty
visit_ClassDefprevents descending into class bodies, so methods defined inside classes won't be analyzed for bidirectional index patterns. A past review comment noted this was addressed, but the current code still contains onlypass.If analyzing class methods is intended, apply this diff:
def visit_ClassDef(self, node): - pass + self.generic_visit(node)Otherwise, document this limitation in the docstring (lines 14-26) to clarify that only top-level and nested functions (not class methods) are analyzed.
126-128: Fallback may return usage line instead of declaration line.The fallback logic returns the first occurrence of any
ast.Namenode matching the variable, which may be a usage site rather than the actual declaration. This could lead to incorrect line numbers in results.Consider removing the fallback to only return explicit declarations:
- for stmt in ast.walk(node): - if isinstance(stmt, ast.Name) and stmt.id == var_name: - return stmt.lineno return NoneAlternatively, if the fallback is necessary for variables that lack explicit declarations (e.g., function parameters), add a clarifying comment:
for stmt in ast.walk(node): if isinstance(stmt, ast.Name) and stmt.id == var_name: + # Fallback: return first usage if no explicit declaration found (e.g., for parameters) return stmt.lineno return NoneBased on past review comments.
🧹 Nitpick comments (2)
aibolit/patterns/bidirect_index/bidirect_index.py (2)
35-35: Simplify detector instantiation with default parameters.The detector is always instantiated with the same empty values
([], '', {}). Consider adding default parameters toBidirectIndexDetector.__init__and calling it without arguments here.Apply this diff:
- detector = BidirectIndexDetector([], '', {}) + detector = BidirectIndexDetector()And update the
__init__signature at line 55:- def __init__(self, vars: List, method: str, ops: Dict): + def __init__(self, vars: List | None = None, method: str = '', ops: Dict | None = None): self.bidirect_variables = vars if vars is not None else [] self.current_method = method self.method_operations = ops if ops is not None else {}
46-46: Use single quotes for consistency with project style.The f-string uses double quotes, but the project enforces single quotes per the ruff Q000 rule (as evidenced by past review comments).
Apply this diff:
- return f"LineNumber(line={self.line}, variable='{self.variable}')" + return f'LineNumber(line={self.line}, variable="{self.variable}")'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (5)
aibolit/patterns/bidirect_index/bidirect_index.py (5)
4-5: LGTM! Imports are appropriate.The added imports support the new AST-based analysis functionality.
55-58: Already addressed in the review comment for line 35.
60-66: LGTM! Nested function context is properly preserved.The method correctly saves and restores
current_methodcontext, preventing false negatives when analyzing nested functions.
71-95: LGTM! Visit methods correctly track operations.The
visit_AugAssignandvisit_Assignmethods properly identify and track increment/decrement operations on variables, distinguishing between augmented assignments (x += 1) and compound assignments (x = x + 1).
107-113: LGTM! Bidirectional detection logic is correct.The method correctly identifies variables that have both increment and decrement operations within the same function scope, recording their declaration lines as results.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
54-63: Support async functions.Analyze
async defthe same as regular functions.Apply:
def visit_FunctionDef(self, node): prev = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.method_variables[self.current_method] = {} self._find_variable_declarations(node) self.generic_visit(node) self._check_bidirectional_variables() self.current_method = prev + + def visit_AsyncFunctionDef(self, node): + # Treat async functions like regular ones + return self.visit_FunctionDef(node)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 5-5: F401 'typing.Set' imported but unused
[error] 48-48: E241 multiple spaces after ':'
[error] 59-59: W291 trailing whitespace
[error] 84-84: W291 trailing whitespace
[error] 94-94: W291 trailing whitespace
[error] 106-106: W291 trailing whitespace
[error] 130-130: W291 trailing whitespace
[error] 147-147: W293 blank line contains whitespace
[error] 150-150: W293 blank line contains whitespace
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 59-59: Trailing whitespace (trailing-whitespace)
[error] 84-84: Trailing whitespace (trailing-whitespace)
[error] 94-94: Trailing whitespace (trailing-whitespace)
[error] 106-106: Trailing whitespace (trailing-whitespace)
[error] 130-130: Trailing whitespace (trailing-whitespace)
[error] 147-147: Trailing whitespace (trailing-whitespace)
[error] 150-150: Trailing whitespace (trailing-whitespace)
[error] 28-28: E1120: No value for argument 'bvars' in constructor call (no-value-for-parameter)
[error] 28-28: E1120: No value for argument 'method' in constructor call (no-value-for-parameter)
[error] 28-28: E1120: No value for argument 'ops' in constructor call (no-value-for-parameter)
[error] 28-28: E1120: No value for argument 'mvars' in constructor call (no-value-for-parameter)
[error] 5-5: W0611: Unused Set imported from typing (unused-import)
🪛 GitHub Actions: ruff
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 5-5: F401 'Set' imported but unused. 1 fixable with the --fix option (ruff).
[error] 1-1: Ruff detected 1 error in the file. Command: 'ruff check .'
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 28-28: missing-argument: No arguments provided for required parameters bvars, method, ops, mvars of bound method __init__
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
13-13: I need to check the pyproject.toml file directly from the repository to confirm the minimum Python version.Based on my comprehensive analysis:All CI workflows are configured for Python 3.11 only.
The
str | os.PathLikeunion syntax (introduced in PEP 604) is supported in Python 3.10 and later. The project's GitHub Actions workflows consistently specify Python 3.11, and no version constraint files for earlier Python versions were found in the repository.Given that:
- All workflows target Python 3.11
- No configuration files specifying Python < 3.10 support exist
- Python 3.11 fully supports the union type syntax
The annotation is correct for the project's supported versions.
| tree = ast.parse(source_code) | ||
| except SyntaxError: | ||
| return [] | ||
| detector = BidirectIndexDetector() |
There was a problem hiding this comment.
Fix constructor signature (defaults, typing) and call-site; also resolve unused Set and E241.
BidirectIndexDetector.__init__ requires 4 args but BidirectIndexDetector() is called with none. Also there’s extra spacing after : and Set is currently unused. Make params optional with sane defaults and initialize internally; annotate ops to use Set so lint passes. Call‑site stays unchanged.
Apply:
class BidirectIndexDetector(ast.NodeVisitor):
- def __init__(self, bvars: List, method: Optional[str], ops, mvars: Dict):
- self.bidirect_variables = bvars
- self.current_method = method
- self.method_operations = ops
- self.method_variables = mvars
+ def __init__(
+ self,
+ bvars: Optional[List[LineNumber]] = None,
+ method: Optional[str] = None,
+ ops: Optional[Dict[str, Dict[str, Set[str]]]] = None,
+ mvars: Optional[Dict[str, Dict[str, int]]] = None,
+ ):
+ self.bidirect_variables = bvars if bvars is not None else []
+ self.current_method = method
+ self.method_operations = ops if ops is not None else {}
+ self.method_variables = mvars if mvars is not None else {}This resolves: ty “missing-argument”, flake8 E241, and ruff/flake8 unused Set. Based on pipeline_failures.
Also applies to: 48-53
🧰 Tools
🪛 GitHub Actions: pylint
[error] 28-28: E1120: No value for argument 'bvars' in constructor call (no-value-for-parameter)
[error] 28-28: E1120: No value for argument 'method' in constructor call (no-value-for-parameter)
[error] 28-28: E1120: No value for argument 'ops' in constructor call (no-value-for-parameter)
[error] 28-28: E1120: No value for argument 'mvars' in constructor call (no-value-for-parameter)
🪛 GitHub Actions: ty
[error] 28-28: missing-argument: No arguments provided for required parameters bvars, method, ops, mvars of bound method __init__
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
aibolit/patterns/bidirect_index/bidirect_index.py (3)
82-90: Optional: classify x += -1 and x -= -1 correctly.Current logic only uses the operator, so
x += -1is flagged as increment. Inspect RHS sign.Apply:
- def visit_AugAssign(self, node): + def visit_AugAssign(self, node): if self.current_method is None or not isinstance(node.target, ast.Name): return var_name = node.target.id - operation = self._get_aug_operation_type(node.op) + operation = self._get_aug_operation_type(node) @@ - def _get_aug_operation_type(self, op) -> Optional[str]: - """Get operation type for augmented assignment""" - if isinstance(op, ast.Add): - return 'increment' - elif isinstance(op, ast.Sub): - return 'decrement' + def _get_aug_operation_type(self, node: ast.AugAssign) -> Optional[str]: + """Get operation type for augmented assignment, accounting for negative RHS.""" + op = node.op + if isinstance(op, ast.Add): + # x += -k => decrement; x += +k => increment + if isinstance(node.value, ast.UnaryOp) and isinstance(node.value.op, ast.USub): + return 'decrement' + return 'increment' + if isinstance(op, ast.Sub): + # x -= -k => increment; x -= +k => decrement + if isinstance(node.value, ast.UnaryOp) and isinstance(node.value.op, ast.USub): + return 'increment' + return 'decrement' return NoneAlso applies to: 114-121
38-40: Style: use single quotes in repr (ruff Q000).- def __repr__(self): - return f"LineNumber(line={self.line}, variable='{self.variable}')" + def __repr__(self): + return f'LineNumber(line={self.line}, variable={self.variable!r})'
20-27: Optional: guard against I/O errors when opening files.Return empty on
OSErrorto match missing-file behavior.- with open(filename, 'r', encoding='utf-8') as file: - source_code = file.read() + try: + with open(filename, 'r', encoding='utf-8') as file: + source_code = file.read() + except OSError: + return []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 28-28: pylint: E1120 No value for argument 'bvars' in constructor call (no-value-for-parameter)
[error] 28-28: pylint: E1120 No value for argument 'method' in constructor call (no-value-for-parameter)
[error] 28-28: pylint: E1120 No value for argument 'ops' in constructor call (no-value-for-parameter)
[error] 28-28: pylint: E1120 No value for argument 'mvars' in constructor call (no-value-for-parameter)
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 28-28: missing-argument: No arguments provided for required parameters bvars, method, ops, mvars of bound method __init__
🔇 Additional comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
13-13: Verify Python version; adjust union type if < 3.10.
str | os.PathLikerequires 3.10+. If the project supports 3.8/3.9, switch to typing.Union.If needed, apply:
-from typing import List, Dict, Optional +from typing import List, Dict, Optional, Union @@ - def value(self, filename: str | os.PathLike) -> List['LineNumber']: + def value(self, filename: Union[str, os.PathLike]) -> List['LineNumber']:Also applies to: 5-5
| def visit_FunctionDef(self, node): | ||
| prev = self.current_method | ||
| self.current_method = node.name | ||
| self.method_operations[self.current_method] = {} | ||
| self.method_variables[self.current_method] = {} | ||
| self._find_variable_declarations(node) | ||
| self.generic_visit(node) | ||
| self._check_bidirectional_variables() | ||
| self.current_method = prev | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Async functions are ignored. Mirror FunctionDef for AsyncFunctionDef.
Without this, async methods won’t be analyzed.
Apply:
def visit_FunctionDef(self, node):
prev = self.current_method
self.current_method = node.name
self.method_operations[self.current_method] = {}
self.method_variables[self.current_method] = {}
self._find_variable_declarations(node)
self.generic_visit(node)
self._check_bidirectional_variables()
self.current_method = prev
+
+ def visit_AsyncFunctionDef(self, node):
+ prev = self.current_method
+ self.current_method = node.name
+ self.method_operations[self.current_method] = {}
+ self.method_variables[self.current_method] = {}
+ self._find_variable_declarations(node)
+ self.generic_visit(node)
+ self._check_bidirectional_variables()
+ self.current_method = prev🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 54-63, async
functions are currently ignored because only visit_FunctionDef is implemented;
add support for AsyncFunctionDef by implementing visit_AsyncFunctionDef that
mirrors visit_FunctionDef (i.e., save prev current_method, set current_method to
node.name, initialize method_operations and method_variables for that name, call
_find_variable_declarations(node), call generic_visit(node), call
_check_bidirectional_variables(), and restore prev) or alternatively have
visit_AsyncFunctionDef delegate to visit_FunctionDef so async methods are
analyzed the same way as regular functions.
| def _find_variable_declarations(self, node): | ||
| """Find all variable declarations in the function""" | ||
| for child in ast.walk(node): | ||
| if isinstance(child, ast.Assign): | ||
| for target in child.targets: | ||
| if isinstance(target, ast.Name): | ||
| var_name = target.id | ||
| if var_name not in self.method_variables[self.current_method]: | ||
| self.method_variables[self.current_method][var_name] = child.lineno | ||
| elif isinstance(child, ast.AnnAssign) and isinstance(child.target, ast.Name): | ||
| var_name = child.target.id | ||
| if var_name not in self.method_variables[self.current_method]: | ||
| self.method_variables[self.current_method][var_name] = child.lineno | ||
| elif isinstance(child, ast.AugAssign) and isinstance(child.target, ast.Name): | ||
| var_name = child.target.id | ||
| if var_name not in self.method_variables[self.current_method]: | ||
| self.method_variables[self.current_method][var_name] = child.lineno | ||
|
|
There was a problem hiding this comment.
Scope leak: declaration scan includes nested functions/classes.
ast.walk(node) traverses into inner scopes; outer method gets inner declarations. Limit scan to current function body.
Apply:
- def _find_variable_declarations(self, node):
- """Find all variable declarations in the function"""
- for child in ast.walk(node):
+ def _find_variable_declarations(self, node):
+ """Find variable declarations in the current function (exclude nested scopes)."""
+ def walk_without_new_scopes(n):
+ for ch in ast.iter_child_nodes(n):
+ if isinstance(ch, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Lambda)):
+ continue
+ yield ch
+ yield from walk_without_new_scopes(ch)
+ for child in walk_without_new_scopes(node):
if isinstance(child, ast.Assign):
for target in child.targets:
if isinstance(target, ast.Name):
var_name = target.id
if var_name not in self.method_variables[self.current_method]:
self.method_variables[self.current_method][var_name] = child.lineno
elif isinstance(child, ast.AnnAssign) and isinstance(child.target, ast.Name):
var_name = child.target.id
if var_name not in self.method_variables[self.current_method]:
self.method_variables[self.current_method][var_name] = child.lineno
elif isinstance(child, ast.AugAssign) and isinstance(child.target, ast.Name):
var_name = child.target.id
if var_name not in self.method_variables[self.current_method]:
self.method_variables[self.current_method][var_name] = child.lineno🤖 Prompt for AI Agents
aibolit/patterns/bidirect_index/bidirect_index.py around lines 64 to 81: the
current use of ast.walk(node) leaks declarations from nested functions/classes
into the outer method; replace the walk with a scan limited to the current
function's top-level body (iterate node.body) and ignore nested FunctionDef and
ClassDef nodes, handling Assign, AnnAssign and AugAssign only at that top level
and extracting Name targets as before so inner-scope variables are not recorded
for the outer method.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
64-81: Scope leak: declaration scan traverses into nested scopes. Limit to current function.
ast.walk(node)includes inner functions/classes; declarations bleed into the outer method.- def _find_variable_declarations(self, node): - """Find all variable declarations in the function""" - for child in ast.walk(node): + def _find_variable_declarations(self, node): + """Find variable declarations in the current function (exclude nested scopes).""" + def walk_no_new_scopes(n): + for ch in ast.iter_child_nodes(n): + if isinstance(ch, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef, ast.Lambda)): + continue + yield ch + yield from walk_no_new_scopes(ch) + for child in walk_no_new_scopes(node): if isinstance(child, ast.Assign): for target in child.targets: if isinstance(target, ast.Name): var_name = target.id if var_name not in self.method_variables[self.current_method]: self.method_variables[self.current_method][var_name] = child.lineno elif isinstance(child, ast.AnnAssign) and isinstance(child.target, ast.Name): var_name = child.target.id if var_name not in self.method_variables[self.current_method]: self.method_variables[self.current_method][var_name] = child.lineno elif isinstance(child, ast.AugAssign) and isinstance(child.target, ast.Name): var_name = child.target.id if var_name not in self.method_variables[self.current_method]: self.method_variables[self.current_method][var_name] = child.lineno
🧹 Nitpick comments (4)
aibolit/patterns/bidirect_index/bidirect_index.py (4)
148-154: Remove dead helper_is_positive_number(now unused).It’s only referenced by code removed above; keep code lean.
- def _is_positive_number(self, node) -> bool: - """Check if node represents a positive number""" - if isinstance(node, ast.Constant): - return isinstance(node.value, (int, float)) and node.value > 0 - elif isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.UAdd): - return self._is_positive_number(node.operand) - return False
54-63: Analyze async functions too. Mirror FunctionDef for AsyncFunctionDef.Without this, async defs are skipped.
def visit_FunctionDef(self, node): prev = self.current_method self.current_method = node.name self.method_operations[self.current_method] = {} self.method_variables[self.current_method] = {} self._find_variable_declarations(node) self.generic_visit(node) self._check_bidirectional_variables() self.current_method = prev + + def visit_AsyncFunctionDef(self, node): + prev = self.current_method + self.current_method = node.name + self.method_operations[self.current_method] = {} + self.method_variables[self.current_method] = {} + self._find_variable_declarations(node) + self.generic_visit(node) + self._check_bidirectional_variables() + self.current_method = prev
92-103: Support annotated assignments (AnnAssign).
x: int = x + 1currently ignored.def visit_Assign(self, node): if self.current_method is None: return for target in node.targets: if isinstance(target, ast.Name): var_name = target.id operation = self._get_assign_operation_type(node.value, var_name) if operation: if var_name not in self.method_operations[self.current_method]: self.method_operations[self.current_method][var_name] = set() self.method_operations[self.current_method][var_name].add(operation) + + def visit_AnnAssign(self, node): + if self.current_method is None or node.value is None: + return + if isinstance(node.target, ast.Name): + var_name = node.target.id + operation = self._get_assign_operation_type(node.value, var_name) + if operation: + if var_name not in self.method_operations[self.current_method]: + self.method_operations[self.current_method][var_name] = set() + self.method_operations[self.current_method][var_name].add(operation)
25-26: Minor polish: improve parse diagnostics; unify quotes.
- Pass filename to
ast.parsefor better error messages.- Use single quotes in
__repr__to match project style.- tree = ast.parse(source_code) + tree = ast.parse(source_code, filename=str(filename)) @@ - return f"LineNumber(line={self.line}, variable='{self.variable}')" + return f'LineNumber(line={self.line}, variable={self.variable!r})'Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aibolit/patterns/bidirect_index/bidirect_index.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: flake8
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 155-155: flake8: W293 blank line contains whitespace.
[error] 163-163: flake8: W293 blank line contains whitespace.
[error] 1-1: flake8 reported E/W style issues; linting failed.
🪛 GitHub Actions: pylint
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 155-155: Trailing whitespace (trailing-whitespace)
[error] 163-163: Trailing whitespace (trailing-whitespace)
[error] 28-28: No value for argument 'bvars' in constructor call (no-value-for-parameter)
[error] 28-28: No value for argument 'method' in constructor call (no-value-for-parameter)
[error] 28-28: No value for argument 'ops' in constructor call (no-value-for-parameter)
[error] 28-28: No value for argument 'mvars' in constructor call (no-value-for-parameter)
[error] 122-122: Too many return statements (7/6) (too-many-return-statements)
🪛 GitHub Actions: ty
aibolit/patterns/bidirect_index/bidirect_index.py
[error] 28-28: missing-argument: No arguments provided for required parameters bvars, method, ops, mvars of bound method __init__.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: latexmk
🔇 Additional comments (1)
aibolit/patterns/bidirect_index/bidirect_index.py (1)
13-31: Optional: early-return is fine; consider PathLike typing consistency.
filename: str | os.PathLikeis 3.10+. If project targets older Python, switch toUnion[str, os.PathLike]withfrom __future__ import annotationsor keep current if 3.10+.What Python versions does Aibolit target currently?
| def __init__(self, bvars: List, method: Optional[str], ops, mvars: Dict): | ||
| self.bidirect_variables = bvars | ||
| self.current_method = method | ||
| self.method_operations = ops | ||
| self.method_variables = mvars | ||
|
|
There was a problem hiding this comment.
Blocker: constructor requires args; call site passes none. Fix signature and defaults.
Make parameters optional with safe defaults to unblock CI; also add type for ops using Set.
Apply:
-from typing import List, Dict, Optional
+from typing import List, Dict, Optional, Set
@@
-class BidirectIndexDetector(ast.NodeVisitor):
- def __init__(self, bvars: List, method: Optional[str], ops, mvars: Dict):
- self.bidirect_variables = bvars
- self.current_method = method
- self.method_operations = ops
- self.method_variables = mvars
+class BidirectIndexDetector(ast.NodeVisitor):
+ def __init__(
+ self,
+ bvars: Optional[List['LineNumber']] = None,
+ method: Optional[str] = None,
+ ops: Optional[Dict[str, Dict[str, Set[str]]]] = None,
+ mvars: Optional[Dict[str, Dict[str, int]]] = None,
+ ):
+ self.bidirect_variables = bvars if bvars is not None else []
+ self.current_method = method
+ self.method_operations = ops if ops is not None else {}
+ self.method_variables = mvars if mvars is not None else {}Also applies to: 28-30, 5-5
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 48-53 (also
apply same change at lines 28-30 and 5-5): the __init__ currently requires
bvars, method, ops, mvars but some call sites pass no arguments; change the
signature to make all parameters optional with safe defaults (e.g., bvars:
Optional[List] = None -> store [] when None, method: Optional[str] = None, ops:
Optional[Set] = None -> store set() when None and add typing Set import, mvars:
Optional[Dict] = None -> store {} when None) and update attribute assignments to
use those defaults so construction without args works and ops has the correct
Set type.
| def visit_UnaryOp(self, node): | ||
| if self.current_method is None or not isinstance(node.operand, ast.Name): | ||
| return | ||
| var_name = node.operand.id | ||
| operation = self._get_unary_operation_type(node.op) | ||
| if operation: | ||
| if var_name not in self.method_operations[self.current_method]: | ||
| self.method_operations[self.current_method][var_name] = set() | ||
| self.method_operations[self.current_method][var_name].add(operation) | ||
|
|
There was a problem hiding this comment.
Unary +/− is not a mutation; remove detection. Also clears trailing whitespace warnings.
This produces false positives (e.g., y = -x). Delete visit_UnaryOp and _get_unary_operation_type. The removal also fixes flake8 W293 at Lines 155 and 163.
- def visit_UnaryOp(self, node):
- if self.current_method is None or not isinstance(node.operand, ast.Name):
- return
- var_name = node.operand.id
- operation = self._get_unary_operation_type(node.op)
- if operation:
- if var_name not in self.method_operations[self.current_method]:
- self.method_operations[self.current_method][var_name] = set()
- self.method_operations[self.current_method][var_name].add(operation)
@@
- def _get_unary_operation_type(self, op) -> Optional[str]:
- """Get operation type for unary operations"""
- if isinstance(op, ast.UAdd):
- return 'increment'
- elif isinstance(op, ast.USub):
- return 'decrement'
- return None
-Also applies to: 156-163, 155-155, 163-163
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 104-113
(visit_UnaryOp) and the helper _get_unary_operation_type (around lines 155-163),
remove the entire visit_UnaryOp method and the _get_unary_operation_type
function because unary +/− are not mutations and cause false positives; also
delete any trailing whitespace on lines 155 and 163 to fix flake8 W293. Ensure
all references to _get_unary_operation_type are removed or updated so there are
no unresolved calls.
| def _get_assign_operation_type(self, value, var_name: str) -> Optional[str]: | ||
| """Get operation type for regular assignment""" | ||
| if isinstance(value, ast.BinOp): | ||
| if (isinstance(value.left, ast.Name) and value.left.id == var_name): | ||
| if isinstance(value.op, ast.Add): | ||
| return 'increment' | ||
| elif isinstance(value.op, ast.Sub): | ||
| return 'decrement' | ||
| elif (isinstance(value.right, ast.Name) and value.right.id == var_name and | ||
| isinstance(value.op, ast.Add)): | ||
| return 'increment' | ||
| elif (isinstance(value.right, ast.Name) and value.right.id == var_name and | ||
| isinstance(value.op, ast.Sub)): | ||
| return 'decrement' | ||
| elif (isinstance(value, ast.BinOp) and | ||
| isinstance(value.left, ast.Name) and value.left.id == var_name and | ||
| isinstance(value.op, ast.Add) and | ||
| self._is_positive_number(value.right)): | ||
| return 'increment' | ||
| elif (isinstance(value, ast.BinOp) and | ||
| isinstance(value.left, ast.Name) and value.left.id == var_name and | ||
| isinstance(value.op, ast.Sub) and | ||
| self._is_positive_number(value.right)): | ||
| return 'decrement' | ||
| return None |
There was a problem hiding this comment.
Fix decrement misclassification and reduce returns to satisfy pylint.
x = y - xis not a decrement ofx.- Current function has too many returns; simplify logic and remove unreachable blocks.
- def _get_assign_operation_type(self, value, var_name: str) -> Optional[str]:
- """Get operation type for regular assignment"""
- if isinstance(value, ast.BinOp):
- if (isinstance(value.left, ast.Name) and value.left.id == var_name):
- if isinstance(value.op, ast.Add):
- return 'increment'
- elif isinstance(value.op, ast.Sub):
- return 'decrement'
- elif (isinstance(value.right, ast.Name) and value.right.id == var_name and
- isinstance(value.op, ast.Add)):
- return 'increment'
- elif (isinstance(value.right, ast.Name) and value.right.id == var_name and
- isinstance(value.op, ast.Sub)):
- return 'decrement'
- elif (isinstance(value, ast.BinOp) and
- isinstance(value.left, ast.Name) and value.left.id == var_name and
- isinstance(value.op, ast.Add) and
- self._is_positive_number(value.right)):
- return 'increment'
- elif (isinstance(value, ast.BinOp) and
- isinstance(value.left, ast.Name) and value.left.id == var_name and
- isinstance(value.op, ast.Sub) and
- self._is_positive_number(value.right)):
- return 'decrement'
- return None
+ def _get_assign_operation_type(self, value, var_name: str) -> Optional[str]:
+ """Classify x = x +/- y (or y + x). 'y - x' is NOT a decrement of x."""
+ if not isinstance(value, ast.BinOp):
+ return None
+ left_is_var = isinstance(value.left, ast.Name) and value.left.id == var_name
+ right_is_var = isinstance(value.right, ast.Name) and value.right.id == var_name
+ if isinstance(value.op, ast.Add) and (left_is_var or right_is_var):
+ return 'increment'
+ if isinstance(value.op, ast.Sub) and left_is_var:
+ return 'decrement'
+ return NoneAlso applies to: 92-103
🧰 Tools
🪛 GitHub Actions: pylint
[error] 122-122: Too many return statements (7/6) (too-many-return-statements)
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 122-146 (and
also apply the same change to 92-103): the function misclassifies expressions
where the target variable appears on the right (e.g. x = y - x) as
increments/decrements and contains too many early returns; change the logic so
you only treat operations as increment/decrement when the target variable is the
left operand of the BinOp (i.e., value.left is the var_name) and the operator
matches (Add => 'increment', Sub => 'decrement'), include the positive-number
checks using self._is_positive_number(value.right) for those left-side cases,
remove the branches that check value.right == var_name, and consolidate the
function to assign the result to a single op_type variable and return it once at
the end to satisfy pylint.
| elif isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.UAdd): | ||
| return self._is_positive_number(node.operand) | ||
| return False | ||
|
|
There was a problem hiding this comment.
Fix trailing whitespace flagged by flake8 W293.
Trim whitespace on blank lines.
Apply a whitespace fix or run formatter (e.g., ruff/black) to clear W293 on these lines.
Also applies to: 163-163
🧰 Tools
🪛 GitHub Actions: flake8
[error] 155-155: flake8: W293 blank line contains whitespace.
🪛 GitHub Actions: pylint
[error] 155-155: Trailing whitespace (trailing-whitespace)
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 155 and 163
there are blank lines containing trailing whitespace (flake8 W293); remove the
trailing spaces on those blank lines so they are truly empty, or run an
automatic formatter/whitespace fixer (e.g., ruff or black) to strip trailing
whitespace across the file and re-check flake8.
1 participant
Closes the issue #742
Summary by CodeRabbit
New Features
Bug Fixes
Tests