-
Notifications
You must be signed in to change notification settings - Fork 52
Implement LoopOutsider pattern
#816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4eb4a15
6e18200
5ef754b
37698c5
8512d28
4d0197a
dc18ba6
8069b5c
57999a3
a09a847
33de203
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,11 @@ | ||||||||||||||||||||||||||||||||||||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2019-2025 Aibolit | ||||||||||||||||||||||||||||||||||||||||||
| # SPDX-License-Identifier: MIT | ||||||||||||||||||||||||||||||||||||||||||
| from typing import List | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| from typing import List, Set | ||||||||||||||||||||||||||||||||||||||||||
| from aibolit.ast_framework import AST, ASTNodeType | ||||||||||||||||||||||||||||||||||||||||||
| from aibolit.types_decl import LineNumber | ||||||||||||||||||||||||||||||||||||||||||
| from aibolit.utils.ast_builder import build_ast | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| class LoopOutsider: | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -17,4 +21,62 @@ def value(self, filename) -> List[LineNumber]: | |||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||
| Returns the line number of loop outsiders found in file. | ||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||
| return [] | ||||||||||||||||||||||||||||||||||||||||||
| res = [] | ||||||||||||||||||||||||||||||||||||||||||
| ast = AST.build_from_javalang(build_ast(filename)) | ||||||||||||||||||||||||||||||||||||||||||
| loop_types = [ASTNodeType.WHILE_STATEMENT, ASTNodeType.FOR_STATEMENT, | ||||||||||||||||||||||||||||||||||||||||||
| ASTNodeType.DO_STATEMENT] | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| for loop_type in loop_types: | ||||||||||||||||||||||||||||||||||||||||||
| for loop_statement in ast.get_proxy_nodes(loop_type): | ||||||||||||||||||||||||||||||||||||||||||
| subtree = ast.get_subtree(loop_statement) | ||||||||||||||||||||||||||||||||||||||||||
| var_changes = self._find_variable_changes(subtree) | ||||||||||||||||||||||||||||||||||||||||||
| loop_vars_declarations = ( | ||||||||||||||||||||||||||||||||||||||||||
| self._find_loop_variable_declarations(subtree, | ||||||||||||||||||||||||||||||||||||||||||
| loop_statement, | ||||||||||||||||||||||||||||||||||||||||||
| loop_type)) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # Check if affected variables are not declared in loop_vars | ||||||||||||||||||||||||||||||||||||||||||
| for node in var_changes: | ||||||||||||||||||||||||||||||||||||||||||
| if node.member not in loop_vars_declarations: | ||||||||||||||||||||||||||||||||||||||||||
| res.append(node.line) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Guard against nodes without
- if node.member not in loop_vars_declarations:
- res.append(node.line)
+ var_name = getattr(node, "member", None)
+ if var_name and var_name not in loop_vars_declarations:
+ res.add(node.line)Couple with 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ivanovmg You're absolutely right! That's a much better approach. Explicitly checking the node type is more precise and safer than using Here's the improved code implementing your suggestion while also addressing the duplicate issue: def value(self, filename) -> List[LineNumber]:
"""
Returns the line number of loop outsiders found in file.
"""
res = set()
with open(filename, 'r', encoding='utf-8') as f:
content = f.read()
javalang_parsed = parse.parse(content)
ast = AST.build_from_javalang(javalang_parsed)
loop_types = [ASTNodeType.WHILE_STATEMENT, ASTNodeType.FOR_STATEMENT,
ASTNodeType.DO_STATEMENT]
for loop_type in loop_types:
for loop_statement in ast.get_proxy_nodes(loop_type):
subtree = ast.get_subtree(loop_statement)
var_changes = self._find_variable_changes(subtree)
loop_vars_declarations = (
self._find_loop_variable_declarations(subtree,
loop_statement,
loop_type))
# Check if affected variables are not declared in loop_vars
for node in var_changes:
if node.node_type == ASTNodeType.MEMBER_REFERENCE:
if node.member not in loop_vars_declarations:
res.add(node.line)
return sorted(res)Your approach is cleaner and more type-safe than the defensive
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SinbadTheSailor2005 can you comment on this? Is it a viable recommendation? If so, then maybe introduce this check as well: Original comment from the bot: #816 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||||||||||||||
| return sorted(res) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| def _find_variable_changes(self, ast: AST) -> Set: | ||||||||||||||||||||||||||||||||||||||||||
| """Find all variables that are modified in the code.""" | ||||||||||||||||||||||||||||||||||||||||||
| var_changes = set() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # Find variables affected by increment/decrement operations | ||||||||||||||||||||||||||||||||||||||||||
| for node in ast.get_proxy_nodes(ASTNodeType.MEMBER_REFERENCE): | ||||||||||||||||||||||||||||||||||||||||||
| if self._variable_is_affected(node): | ||||||||||||||||||||||||||||||||||||||||||
| var_changes.add(node) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # Find variables affected by assignments | ||||||||||||||||||||||||||||||||||||||||||
| for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT): | ||||||||||||||||||||||||||||||||||||||||||
| var_changes.add(node.expressionl) | ||||||||||||||||||||||||||||||||||||||||||
| return var_changes | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+55
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against assignments whose LHS isn’t a
- for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT):
- var_changes.add(node.expressionl)
+ for assign in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT):
+ lhs = assign.expressionl
+ if lhs.node_type == ASTNodeType.MEMBER_REFERENCE:
+ var_changes.add(lhs)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| def _find_loop_variable_declarations(self, ast: AST, loop_statement, | ||||||||||||||||||||||||||||||||||||||||||
| loop_type) -> Set: | ||||||||||||||||||||||||||||||||||||||||||
| """Find all variable declarations within the loop scope.""" | ||||||||||||||||||||||||||||||||||||||||||
| loop_vars_declarations = set() | ||||||||||||||||||||||||||||||||||||||||||
| subtree = ast.get_subtree(loop_statement) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # Find local variable declarations | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+59
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not call
- subtree = ast.get_subtree(loop_statement)
+ subtree = ast # `ast` is already the loop-local subtree📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
| for node in subtree.get_proxy_nodes( | ||||||||||||||||||||||||||||||||||||||||||
| ASTNodeType.LOCAL_VARIABLE_DECLARATION): | ||||||||||||||||||||||||||||||||||||||||||
| loop_vars_declarations.add(node.names[-1]) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # For for-loops, also check variable declarations in the loop header | ||||||||||||||||||||||||||||||||||||||||||
| if loop_type == ASTNodeType.FOR_STATEMENT: | ||||||||||||||||||||||||||||||||||||||||||
| for node_for in subtree.get_proxy_nodes( | ||||||||||||||||||||||||||||||||||||||||||
| ASTNodeType.VARIABLE_DECLARATION): | ||||||||||||||||||||||||||||||||||||||||||
| loop_vars_declarations.add(node_for.names[-1]) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+66
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Collect all declared names, not just the last one
- loop_vars_declarations.add(node.names[-1])
+ for declarator in node.names:
+ loop_vars_declarations.add(declarator)Apply the same pattern to the 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
| return loop_vars_declarations | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| def _variable_is_affected(self, node): | ||||||||||||||||||||||||||||||||||||||||||
| return ('--' in node.prefix_operators or '--' in | ||||||||||||||||||||||||||||||||||||||||||
| node.postfix_operators or | ||||||||||||||||||||||||||||||||||||||||||
| '++' in node.prefix_operators or '++' in | ||||||||||||||||||||||||||||||||||||||||||
| node.postfix_operators) | ||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,4 +8,4 @@ public void loopOutsiderAddAndInWhile() { | |
| x += 1; // here | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2019-2025 Aibolit | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| public class NoLoopOutsider { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for these new tests!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ivanovmg , look. if node.node_type == ASTNodeType.MEMBER_REFERENCE is unneccesary, since var_changes list constains objects of ASTNodeType.MEMBER_REFERENCE. Look the code snippets, where the algorithm fills the var_changes list # Find variables affected by assignments
for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT):
var_changes.add(node.expressionl)
for node in ast.get_proxy_nodes(ASTNodeType.MEMBER_REFERENCE):
if self._variable_is_affected(node):
var_changes.add(node) # addding MEMBER_REFERENCETo conclue, all objects in var_changes are gurantee to have .member propertie
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok then. Thank you! |
||
| public void noLoopOutsider() { | ||
| int x = 0; | ||
| Integer y = 0; | ||
| while (true) { | ||
|
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2019-2025 Aibolit | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| public class NoLoopOutsiderFakeIncrementing{ | ||
| public void noLoopOutsiderFakeIncrementing() { | ||
| int x = 0; | ||
| while (true) { | ||
| } | ||
| x++; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,11 @@ | |
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import os | ||
| import unittest | ||
| from pathlib import Path | ||
| from unittest import TestCase | ||
| from aibolit.patterns.loop_outsider.loop_outsider import LoopOutsider | ||
|
|
||
|
|
||
| @unittest.skip("Not implemented") | ||
| class LoopOutsiderTestCase(TestCase): | ||
| """ | ||
| @todo #138:30min Continue to implement loop_outsider | ||
|
|
@@ -21,104 +19,114 @@ class LoopOutsiderTestCase(TestCase): | |
| def test_find_loop_outsider_assignment_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderAssignmentInWhile.java")), | ||
| [5], | ||
| [8], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that the test suite does not have a single test for a good piece of code, where the pattern is not matched. |
||
| "Could not find loop outsider assignment in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_prefix_increment_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderPrefixIncrementInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with prefix increment in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_postfix_increment_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderPostfixIncrementInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with postfix increment in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_prefix_decrement_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderPrefixDecrementInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with prefix decrement in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_postfix_decrement_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderPostfixDecrementInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with postfix decrement in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_add_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderAddAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with add AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_subtract_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderSubtractAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with subtract AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_multiply_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderMultiplyAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with multiply AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_divide_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderDivideAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with divide AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_modulus_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderModulusAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with modulus AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_left_shift_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderLeftShiftAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with left shift AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_right_shift_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderRightShiftAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with right shift AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_bitwise_and_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderBitwiseAndInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with bitwise AND in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_bitwise_inclusive_or_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderBitwiseInclusiveOrInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with bitwise inclusive OR in while loop", | ||
| ) | ||
|
|
||
| def test_find_loop_outsider_bitwise_exclusive_or_in_while(self): | ||
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderBitwiseExclusiveOrInWhile.java")), | ||
| [5], | ||
| [8], | ||
| "Could not find loop outsider with bitwise exclusive OR in while loop", | ||
| ) | ||
|
|
||
| def test_should_not_detect_loop_outsider_when_variable_never_modified_outside_loop(self): | ||
| self.assertEqual(LoopOutsider().value(Path(self.dir_path, | ||
| "NoLoopOutsider.java")), [], | ||
| "Found unexisted loop outsider") | ||
|
|
||
| def test_should_not_detect_loop_outsider_when_incrementing_outside_loop(self): | ||
| self.assertEqual(LoopOutsider().value(Path(self.dir_path, | ||
| "NoLoopOutsiderFakeIncrementing.java")), [], | ||
| "Found unexisted loop outsider") | ||
Uh oh!
There was an error while loading. Please reload this page.