Refactor patterns in config to accept ast: AST#832
Conversation
|
""" WalkthroughThis update refactors pattern detection logic to ensure all patterns accept an AST parameter, removing legacy file path and regex-based processing. It updates the configuration, pattern implementations, and associated tests to standardize AST usage, and removes obsolete type ignore comments and test decorators. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant ASTBuilder
participant Pattern
Test->>ASTBuilder: build_ast(file_path)
ASTBuilder-->>Test: AST
Test->>Pattern: value(AST)
Pattern-->>Test: [Line Numbers]
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@ivanovmg please take a look here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
aibolit/patterns/assign_null_finder/assign_null_finder.py (1)
14-20: Guard against missing attributes & use a constant
assignment.value/declarator.initializerare accessed directly. If the AST ever contains anASSIGNMENT-node without avalueattribute (e.g. malformed tree),AttributeErrorwill bubble up and stop pattern evaluation.Also, the literal
'null'is hard-coded twice.+_JAVA_NULL = 'null' # single source of truth ... - if self._is_null_literal(assignment.value): + if getattr(assignment, 'value', None) and self._is_null_literal(assignment.value): ... - if self._is_null_literal(declarator.initializer): + if getattr(declarator, 'initializer', None) and self._is_null_literal(declarator.initializer): ... - return node is not None and node.node_type == ASTNodeType.LITERAL and node.value == 'null' + return ( + node is not None + and node.node_type == ASTNodeType.LITERAL + and node.value == _JAVA_NULL + )aibolit/config.py (1)
170-183: Minor: annotate lambdas for clarityNow that
# type: ignorewas removed, mypy will happily infer the lambda, but adding an explicit return type improves IDE hints:'make': lambda -> Pattern: P20(5)test/integration/test_patterns_and_metrics.py (1)
23-25: Avoid rebuilding the AST for every pattern
AST.build_from_javalang(build_ast(filepath))is executed once per pattern, causing O(N²) parsing in a big suite.Cache the AST per file:
def _check_pattern(p_info, filepath, _cache={}): ast = _cache.setdefault( filepath, AST.build_from_javalang(build_ast(filepath)) ) pattern = p_info['make']() pattern_result = pattern.value(ast)test/patterns/test_assign_null/test_find_assign_null.py (1)
15-17: Useos.path.joinfor portabilityString concatenation with
'/'breaks on Windows paths.- ast = AST.build_from_javalang(build_ast(self.cur_dir + '/several.java')) + ast = AST.build_from_javalang(build_ast(os.path.join(self.cur_dir, 'several.java')))Apply similarly to the other two tests.
test/patterns/var_decl_diff/test_var_decl_diff.py (1)
15-18: Factor out duplicated setup logic to a helper to DRY up testsEach test repeats:
- Build AST from
<n>.java- Instantiate
VarDeclarationDistance- Call
valueExtracting a small helper (e.g.
_run_case(self, filename, threshold)) will reduce duplication and make future maintenance easier.def test_good_class(self): - ast = AST.build_from_javalang(build_ast(self.cur_dir + '/1.java')) - pattern = VarDeclarationDistance(lines_th=2) - lines = pattern.value(ast) - self.assertEqual(lines, []) + self.assertEqual( + self._run_case('1.java', 2), + [] + ) @@ def _run_case(self, java_file: str, th: int): - pass # to be implemented + ast_path = self.cur_dir / java_file + ast = AST.build_from_javalang(build_ast(ast_path)) + return VarDeclarationDistance(lines_th=th).value(ast)Besides readability, this single point of construction makes it trivial to change the AST-building pipeline again without touching every test.
Also applies to: 21-24, 27-30, 33-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
aibolit/config.py(3 hunks)aibolit/patterns/assign_null_finder/assign_null_finder.py(1 hunks)aibolit/patterns/var_decl_diff/var_decl_diff.py(2 hunks)test/config/test_config.py(0 hunks)test/integration/test_patterns_and_metrics.py(2 hunks)test/patterns/test_assign_null/test_find_assign_null.py(1 hunks)test/patterns/var_decl_diff/test_var_decl_diff.py(1 hunks)
💤 Files with no reviewable changes (1)
- test/config/test_config.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/patterns/test_assign_null/test_find_assign_null.py (3)
aibolit/ast_framework/ast.py (2)
AST(31-388)build_from_javalang(37-43)aibolit/patterns/assign_null_finder/assign_null_finder.py (2)
NullAssignment(11-23)value(12-20)aibolit/utils/ast_builder.py (1)
build_ast(11-12)
aibolit/patterns/assign_null_finder/assign_null_finder.py (3)
aibolit/ast_framework/ast.py (2)
AST(31-388)get_proxy_nodes(176-179)aibolit/ast_framework/ast_node_type.py (1)
ASTNodeType(7-90)aibolit/ast_framework/ast_node.py (1)
line(41-69)
aibolit/patterns/var_decl_diff/var_decl_diff.py (3)
aibolit/ast_framework/ast.py (3)
AST(31-388)traverse(100-115)get_subtrees(72-93)aibolit/ast_framework/ast_node.py (2)
ASTNode(18-174)line(41-69)aibolit/ast_framework/ast_node_type.py (1)
ASTNodeType(7-90)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: make
- GitHub Check: latexmk
🔇 Additional comments (1)
aibolit/patterns/var_decl_diff/var_decl_diff.py (1)
26-33: Negative line distance not handled
__line_diffdoesrange(declaration_line + 1, usage_line).
Ifusage_line <= declaration_line(e.g. variable used in its own initializer), the range is empty and the pattern silently ignores the case. Explicitly guard for readability:- lines_range = set(range(declaration_line + 1, usage_line)) - return len(lines_range.difference(empty_lines)) + if usage_line <= declaration_line: + return 0 + return len( + set(range(declaration_line + 1, usage_line)).difference(empty_lines) + )
| def collect_declaration_or_usage(node: ASTNode): | ||
| node_name = self.__node_name(node) | ||
| if node_name: | ||
| if node.node_type == ASTNodeType.VARIABLE_DECLARATOR: | ||
| name_to_declaration_line[node_name] = node.line | ||
| elif ( | ||
| node_name in name_to_declaration_line and | ||
| node_name not in name_to_first_usage_line | ||
| ): | ||
| name_to_first_usage_line[node_name] = node.line | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable shadowing may overwrite first declaration
When traversing the method, every VARIABLE_DECLARATOR overwrites name_to_declaration_line.
For nested blocks a later re-declaration will mask an outer one, so the distance for the original variable is lost.
Consider storing only the first declaration encountered for each name.
🤖 Prompt for AI Agents
In aibolit/patterns/var_decl_diff/var_decl_diff.py around lines 51 to 61, the
current code overwrites the declaration line for a variable each time a new
VARIABLE_DECLARATOR node with the same name is found, causing variable shadowing
to lose the original declaration line. To fix this, modify the code to store the
declaration line only if the variable name is not already present in
name_to_declaration_line, thus preserving the first declaration encountered for
each variable name.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
aibolit/patterns/var_decl_diff/var_decl_diff.py (1)
56-61: Variable shadowing still overwrites first declaration
Re-declarations inside nested blocks replace the original entry inname_to_declaration_line, losing the correct distance for the first declaration – exactly the issue flagged in the previous review.- name_to_declaration_line[node_name] = node.line + # Preserve the first declaration encountered + name_to_declaration_line.setdefault(node_name, node.line)
🧹 Nitpick comments (1)
aibolit/patterns/var_decl_diff/var_decl_diff.py (1)
31-33: Memory-heavy diff calculation – consider arithmetic instead of set ops
set(range(declaration_line + 1, usage_line))allocates O(N) integers per variable.
You can compute the same distance in O(K) where K = number of empty lines in the span:raw_span = usage_line - declaration_line - 1 empty_in_span = sum(1 for l in empty_lines if declaration_line < l < usage_line) return raw_span - empty_in_span
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
aibolit/patterns/assign_null_finder/assign_null_finder.py(1 hunks)aibolit/patterns/var_decl_diff/var_decl_diff.py(2 hunks)test/patterns/test_assign_null/test_find_assign_null.py(1 hunks)test/patterns/var_decl_diff/test_var_decl_diff.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/patterns/var_decl_diff/test_var_decl_diff.py
- test/patterns/test_assign_null/test_find_assign_null.py
- aibolit/patterns/assign_null_finder/assign_null_finder.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: latexmk
- GitHub Check: e2e-test
- GitHub Check: make
| lines_with_nodes = set() | ||
| ast.traverse(lambda node: lines_with_nodes.add(node.line)) | ||
| if not lines_with_nodes: | ||
| return set() | ||
| max_line = max(lines_with_nodes) | ||
| all_lines = set(range(1, max_line + 1)) |
There was a problem hiding this comment.
Guard against None line numbers – current logic can crash
node.line may be None for synthetic or malformed nodes.
Adding such values to lines_with_nodes causes max() to raise TypeError (cannot compare NoneType and int) or, if all values are None, range() will break.
Patch:
- ast.traverse(lambda node: lines_with_nodes.add(node.line))
+ # Skip nodes without a concrete line number to avoid mixing
+ # `None` with integers which breaks `max()` below.
+ ast.traverse(
+ lambda node: lines_with_nodes.add(node.line)
+ if getattr(node, "line", None) is not None
+ else None
+ )
+ if not lines_with_nodes:
+ return set()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In aibolit/patterns/var_decl_diff/var_decl_diff.py around lines 36 to 41, the
code adds node.line values to lines_with_nodes without checking if node.line is
None, which can cause max() and range() to fail. Fix this by filtering out None
values before adding to lines_with_nodes, ensuring only valid integer line
numbers are included to prevent TypeError and range errors.
|
|
||
| def _is_null_assignment(self, node: ASTNode) -> bool: | ||
| return ( | ||
| node.node_type == ASTNodeType.ASSIGNMENT and |
There was a problem hiding this comment.
@AntonProkopyev I failed to identify the original reason for two loops. I believe that your original implementation was more straightforward than this. I suggest that this is reverted back.
ivanovmg
left a comment
ivanovmg
left a comment
There was a problem hiding this comment.
@yegor256 it looks good to me.
@AntonProkopyev thank you for the PR! It is a positive implementation of the way forward on the unification of the interface!
|
@AntonProkopyev thanks for your contribution! |
|
@ivanovmg 🎉 Awesome job on the review! You've just earned 12 points, boosting your total to 294 points. Keep up the great work and watch your score soar! 🚀 |
|
@AntonProkopyev Thank you for your contribution! We appreciate your effort. Your code submission has earned you +8 points: +16 as a base reward, with a -8 deduction due to the high number of hits-of-code (217 ≥ 200). While we value your productivity, please remember to keep your contributions concise and focused. Your current balance stands at +43. Keep up the good work, and we look forward to your future contributions! |
4 participants
Refactored patterns
Enabled patterns
Closes: #830
Summary by CodeRabbit