Implement LoopOutsider pattern #816
Conversation
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoopOutsider
participant JavaParser
participant ASTFramework
User->>LoopOutsider: value(filename)
LoopOutsider->>JavaParser: Parse Java file
JavaParser-->>LoopOutsider: AST
LoopOutsider->>ASTFramework: Build AST
LoopOutsider->>LoopOutsider: Identify loops (for, while, do)
LoopOutsider->>LoopOutsider: Collect declared variables in loops
LoopOutsider->>LoopOutsider: Find modified variables (++, --, assignments)
LoopOutsider->>LoopOutsider: Compare modified vs declared variables
LoopOutsider-->>User: Return sorted line numbers of loop outsiders
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Poem
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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 (
|
|
@SinbadTheSailor2005 Hey there! 👋 Just a heads up about your branch name " |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
test/patterns/loop_outsider/test_loop_outsider.py (1)
4-6:⚠️ Potential issueDrop unused
unittestimport to silence Ruff/flake8-import unittest import os import unittest(or simply delete the first occurrence).
This clearsF401andW0611.🧰 Tools
🪛 Ruff (0.11.9)
5-5:
unittestimported but unusedRemove unused import:
unittest(F401)
🪛 GitHub Actions: ruff
[error] 5-5: Ruff F401:
unittestimported but unused. Remove unused import.🪛 GitHub Actions: flake8
[error] 5-5: flake8: 'unittest' imported but unused (F401)
🪛 GitHub Actions: pylint
[warning] 5-5: Unused import unittest (unused-import) (pylint W0611)
🧹 Nitpick comments (1)
test/patterns/loop_outsider/test_loop_outsider.py (1)
11-12: Trim superfluous blank lineExcess empty lines violate E303 and hinder readability.
- - +🧰 Tools
🪛 GitHub Actions: flake8
[error] 12-12: flake8: too many blank lines (3) (E303)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
aibolit/patterns/loop_outsider/loop_outsider.py(2 hunks)test/patterns/loop_outsider/LoopOutsiderAddAndInWhile.java(1 hunks)test/patterns/loop_outsider/test_loop_outsider.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ruff
test/patterns/loop_outsider/test_loop_outsider.py
[error] 5-5: Ruff F401: unittest imported but unused. Remove unused import.
aibolit/patterns/loop_outsider/loop_outsider.py
[error] 8-8: Ruff F401: aibolit.ast_framework.ast_node.ASTNode imported but unused. Remove unused import.
🪛 GitHub Actions: flake8
test/patterns/loop_outsider/test_loop_outsider.py
[error] 5-5: flake8: 'unittest' imported but unused (F401)
[error] 12-12: flake8: too many blank lines (3) (E303)
aibolit/patterns/loop_outsider/loop_outsider.py
[error] 8-8: flake8: 'aibolit.ast_framework.ast_node.ASTNode' imported but unused (F401)
[error] 20-20: flake8: 'LoopOutsider.value' is too complex (11) (C901)
[error] 54-54: flake8: too many blank lines (3) (E303)
[error] 66-66: flake8: too many blank lines (2) (E303)
[error] 74-74: flake8: too many blank lines (3) (E303)
🪛 GitHub Actions: pylint
test/patterns/loop_outsider/test_loop_outsider.py
[warning] 5-5: Unused import unittest (unused-import) (pylint W0611)
aibolit/patterns/loop_outsider/loop_outsider.py
[warning] 26-26: Using open without explicitly specifying an encoding (unspecified-encoding) (pylint W1514)
[warning] 77-77: The if statement can be replaced with 'return bool(test)' (simplifiable-if-statement) (pylint R1703)
[warning] 5-5: Third party import 'javalang.parse' should be placed before first party import 'aibolit.types_decl.LineNumber' (wrong-import-order) (pylint C0411)
[warning] 7-7: Imports from package aibolit are not grouped (ungrouped-imports) (pylint C0412)
[warning] 8-8: Unused ASTNode imported from aibolit.ast_framework.ast_node (unused-import) (pylint W0611)
🪛 Ruff (0.11.9)
aibolit/patterns/loop_outsider/loop_outsider.py
8-8: aibolit.ast_framework.ast_node.ASTNode imported but unused
Remove unused import: aibolit.ast_framework.ast_node.ASTNode
(F401)
77-82: Return the condition directly
Inline condition
(SIM103)
🪛 Pylint (3.3.7)
aibolit/patterns/loop_outsider/loop_outsider.py
[refactor] 77-82: The if statement can be replaced with 'return bool(test)'
(R1703)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: make
- GitHub Check: latexmk
🔇 Additional comments (2)
aibolit/patterns/loop_outsider/loop_outsider.py (1)
76-82: Inline the boolean for conciseness- if ('--' in node.prefix_operators or '--' in node.postfix_operators or - '++' in node.prefix_operators or '++' in - node.postfix_operators): - return True - else: - return False + return any(op in ("++", "--") for op in (*node.prefix_operators, + *node.postfix_operators))[ suggest_nitpick ]
🧰 Tools
🪛 Ruff (0.11.9)
77-82: Return the condition directly
Inline condition
(SIM103)
🪛 Pylint (3.3.7)
[refactor] 77-82: The if statement can be replaced with 'return bool(test)'
(R1703)
🪛 GitHub Actions: pylint
[warning] 77-77: The if statement can be replaced with 'return bool(test)' (simplifiable-if-statement) (pylint R1703)
test/patterns/loop_outsider/LoopOutsiderAddAndInWhile.java (1)
11-11: File EOF newline removal is fineNo functional impact; nothing to change.
| if node.member not in loop_vars_declarations: | ||
| res.append(node.line) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against nodes without .member and avoid duplicates
ASSIGNMENT.expression may be a MemberReference or a ReferenceType; not all have a .member attribute.
Additionally, appending directly to a list yields duplicates.
- 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 res: set[int] = set() at the top and replace the final return sorted(res).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) |
🤖 Prompt for AI Agents
In aibolit/patterns/loop_outsider/loop_outsider.py around lines 69 to 71, the
code assumes all nodes have a .member attribute, which can cause errors if they
don't, and appends line numbers directly to a list causing duplicates. Fix this
by first checking if the node has a .member attribute before accessing it, and
change res to a set to avoid duplicates. At the top of the function, initialize
res as a set of integers, and before returning, convert res to a sorted list.
There was a problem hiding this comment.
Instead of using getattr as suggested by the bot, I suppose would be better to explicitly check that node.node_type == ASTNodeType.MEMBER_REFERNCE, because the reflection adds another level of complexity.
There was a problem hiding this comment.
@ivanovmg You're absolutely right! That's a much better approach. Explicitly checking the node type is more precise and safer than using getattr. Only MEMBER_REFERENCE nodes are guaranteed to have the .member attribute, so filtering by node type is the correct solution.
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 getattr pattern. It also makes the intent clearer - we only want to process member reference nodes since those represent variable access patterns.
There was a problem hiding this comment.
@SinbadTheSailor2005 can you comment on this? Is it a viable recommendation? If so, then maybe introduce this check as well: if node.node_type == ASTNodeType.MEMBER_REFERENCE.
Original comment from the bot: #816 (comment)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
aibolit/patterns/loop_outsider/loop_outsider.py (3)
57-57:⚠️ Potential issueCritical attribute typo still present.
The typo
node.expressionlremains unfixed and will cause anAttributeErrorat runtime, making the detector unusable.- var_changes.add(node.expressionl) + var_changes.add(node.expression)
34-34:⚠️ Potential issueVariable changes collected globally instead of per-loop.
The current implementation collects variable changes from the entire AST rather than the current loop's scope, causing false positives when multiple loops exist.
- var_changes = self._find_variable_changes(ast) + var_changes = self._find_variable_changes(subtree)Additionally, update the
_find_variable_changesmethod signature:- def _find_variable_changes(self, ast: AST) -> Set: + def _find_variable_changes(self, subtree: AST) -> Set:And update the method body to use
subtreeinstead ofast:- for node in ast.get_proxy_nodes(ASTNodeType.MEMBER_REFERENCE): + for node in subtree.get_proxy_nodes(ASTNodeType.MEMBER_REFERENCE): @@ - for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT): + for node in subtree.get_proxy_nodes(ASTNodeType.ASSIGNMENT):
41-42: 🛠️ Refactor suggestionMissing guard for
.memberattribute and duplicate line numbers.The code assumes all nodes have a
.memberattribute and doesn't prevent duplicate line numbers in results.+ res = set() - res = [] @@ - 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) @@ - return sorted(res) + return sorted(list(res))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
aibolit/patterns/loop_outsider/loop_outsider.py(2 hunks)test/patterns/loop_outsider/test_loop_outsider.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/patterns/loop_outsider/test_loop_outsider.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (3)
aibolit/patterns/loop_outsider/loop_outsider.py (3)
25-25: File encoding properly specified.Good fix for the encoding specification to avoid issues on non-UTF-8 systems.
3-7: Import organization improved.The unused
ASTNodeimport has been removed and import ordering now correctly places third-party imports before first-party imports.
34-42:⚠️ Potential issueType mismatch between variable changes and declarations.
The logic compares node objects from
var_changeswith variable names fromloop_vars_declarations, which will always fail the comparison.The
_find_variable_changesmethod returns a set of nodes, but_find_loop_variable_declarationsreturns a set of variable names. Extract variable names from nodes for proper comparison:var_changes = self._find_variable_changes(subtree) loop_vars_declarations = ( self._find_loop_variable_declarations(ast, 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) + var_name = getattr(node, "member", None) + if var_name and var_name not in loop_vars_declarations: + res.add(node.line)Likely an incorrect or invalid review comment.
|
@ivanovmg can you please review this one? |
|
@SinbadTheSailor2005 thank you for the PR! If you reference the issue like "Closes #XXX" in the PR description, it will be automatically linked with the corresponding ticket. |
There was a problem hiding this comment.
@SinbadTheSailor2005 thank you for the PR! A couple of comments from my side.
Overall looks good to me, but may you still look through my comments and maybe slightly update your code?
|
|
||
| return loop_vars_declarations | ||
|
|
||
| def variableIsAffected(self, node): |
There was a problem hiding this comment.
I suggest that you mark it as protected by starting with an underscore. And it may be a good idea to use snake notation.
| def variableIsAffected(self, node): | |
| def _variable_is_affected(self, node): |
| with open(filename, 'r', encoding='utf-8') as f: | ||
| content = f.read() | ||
| javalang_parsed = parse.parse(content) | ||
| ast = AST.build_from_javalang(javalang_parsed) |
There was a problem hiding this comment.
This is the preferred way of parsing the AST from the filepath, which is used throughout the project:
| with open(filename, 'r', encoding='utf-8') as f: | |
| content = f.read() | |
| javalang_parsed = parse.parse(content) | |
| ast = AST.build_from_javalang(javalang_parsed) | |
| ast = AST.build_from_javalang(build_ast(filename)) |
I suggest that you use this instead.
For example, take a look here: https://github.com/cqfn/aibolit/blob/master/aibolit/patterns/mutable_index/mutable_index.py#L23
| self.assertEqual( | ||
| LoopOutsider().value(Path(self.dir_path, "LoopOutsiderAssignmentInWhile.java")), | ||
| [5], | ||
| [8], |
There was a problem hiding this comment.
I noticed that the test suite does not have a single test for a good piece of code, where the pattern is not matched.
I suggest that you add this test (here, or an extra puzzle).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
aibolit/patterns/loop_outsider/loop_outsider.py (1)
25-44: 🛠️ Refactor suggestionUse a
setforresto avoid duplicates and simplify sortingCollecting into a list will report the same line multiple times when the same variable is modified more than once. A
setalso makes membership checks O(1).- res = [] + res: set[int] = set() ... - res.append(node.line) + res.add(node.line) ... - return sorted(res) + return sorted(res)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
aibolit/patterns/loop_outsider/loop_outsider.py(2 hunks)test/patterns/loop_outsider/NoLoopOutsider.java(1 hunks)test/patterns/loop_outsider/NoLoopOutsiderFakeIncrementing.java(1 hunks)test/patterns/loop_outsider/test_loop_outsider.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/patterns/loop_outsider/NoLoopOutsider.java
🚧 Files skipped from review as they are similar to previous changes (1)
- test/patterns/loop_outsider/test_loop_outsider.py
🧰 Additional context used
🧠 Learnings (1)
aibolit/patterns/loop_outsider/loop_outsider.py (1)
Learnt from: SinbadTheSailor2005
PR: cqfn/aibolit#816
File: aibolit/patterns/loop_outsider/loop_outsider.py:0-0
Timestamp: 2025-06-08T11:50:22.636Z
Learning: In the aibolit codebase, ASTNodeType.ASSIGNMENT nodes have an attribute called `expressionl` (with 'l' suffix), not `expression`. This is confirmed in `aibolit/ast_framework/_auxiliary_data.py` and used consistently throughout existing patterns like mutable_index and many_primary_ctors.
🪛 Ruff (0.11.9)
aibolit/patterns/loop_outsider/loop_outsider.py
6-6: javalang.parse imported but unused
Remove unused import: javalang.parse
(F401)
🪛 GitHub Actions: pylint
aibolit/patterns/loop_outsider/loop_outsider.py
[warning] 6-6: Pylint warning W0611: Unused import 'parse' imported from javalang (unused-import)
🪛 GitHub Actions: flake8
aibolit/patterns/loop_outsider/loop_outsider.py
[error] 6-6: flake8: 'javalang.parse' imported but unused (F401)
🪛 GitHub Actions: ruff
aibolit/patterns/loop_outsider/loop_outsider.py
[error] 6-6: Ruff: javalang.parse imported but unused (F401). Remove unused import.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (1)
test/patterns/loop_outsider/NoLoopOutsiderFakeIncrementing.java (1)
4-11: Test file looks goodThe snippet accurately represents an unreachable post-loop increment and is appropriate as a negative test case for the detector.
| for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT): | ||
| var_changes.add(node.expressionl) | ||
| return var_changes |
There was a problem hiding this comment.
Guard against assignments whose LHS isn’t a MemberReference
assignment.expressionl can be an ArraySelector, This, etc.
Dereferencing .member later will raise AttributeError.
- 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT): | |
| var_changes.add(node.expressionl) | |
| return var_changes | |
| for assign in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT): | |
| lhs = assign.expressionl | |
| if lhs.node_type == ASTNodeType.MEMBER_REFERENCE: | |
| var_changes.add(lhs) | |
| return var_changes |
🤖 Prompt for AI Agents
In aibolit/patterns/loop_outsider/loop_outsider.py around lines 56 to 58, the
code adds node.expressionl to var_changes without checking its type, but
expressionl can be various types like ArraySelector or This, not just
MemberReference. To fix this, add a type check to ensure node.expressionl is an
instance of MemberReference before adding it to var_changes, preventing
AttributeError when accessing .member later.
| 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]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Collect all declared names, not just the last one
LOCAL_VARIABLE_DECLARATION.names is a list of VariableDeclarator; grabbing [-1]
misses earlier declarators in a multi-declaration statement (int a, b;) and
returns the node object rather than the string identifier.
- loop_vars_declarations.add(node.names[-1])
+ for declarator in node.names:
+ loop_vars_declarations.add(declarator)Apply the same pattern to the VARIABLE_DECLARATION branch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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]) | |
| for node in subtree.get_proxy_nodes( | |
| ASTNodeType.LOCAL_VARIABLE_DECLARATION): | |
| for declarator in node.names: | |
| loop_vars_declarations.add(declarator) | |
| # 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): | |
| for declarator in node_for.names: | |
| loop_vars_declarations.add(declarator) |
🤖 Prompt for AI Agents
In aibolit/patterns/loop_outsider/loop_outsider.py lines 67 to 76, the code only
adds the last declared variable name from the names list, which misses earlier
declared variables and adds node objects instead of string identifiers. Update
both the LOCAL_VARIABLE_DECLARATION and VARIABLE_DECLARATION branches to iterate
over all items in the names list and add each variable's string identifier to
loop_vars_declarations.
| 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 |
There was a problem hiding this comment.
Do not call get_subtree on a node that is external to the passed AST
ast already represents the subtree for the current loop.
Calling ast.get_subtree(loop_statement) again will raise networkx.exception.NetworkXError because loop_statement belongs to the original graph.
- subtree = ast.get_subtree(loop_statement)
+ subtree = ast # `ast` is already the loop-local subtree📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| 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) | |
| + subtree = ast # `ast` is already the loop-local subtree | |
| # Find local variable declarations |
🤖 Prompt for AI Agents
In aibolit/patterns/loop_outsider/loop_outsider.py around lines 60 to 66, remove
the call to ast.get_subtree(loop_statement) because ast already represents the
subtree for the current loop. Using get_subtree on loop_statement, which belongs
to the original AST, causes a NetworkXError. Instead, operate directly on ast
without extracting a subtree again.
Done.
@ivanovmg , please check |
ivanovmg
left a comment
ivanovmg
left a comment
There was a problem hiding this comment.
@SinbadTheSailor2005 @yegor256 it looks good to me. There are a couple of comments from the bot. But I suppose, it is good to go.
| if node.member not in loop_vars_declarations: | ||
| res.append(node.line) | ||
|
|
There was a problem hiding this comment.
@SinbadTheSailor2005 can you comment on this? Is it a viable recommendation? If so, then maybe introduce this check as well: if node.node_type == ASTNodeType.MEMBER_REFERENCE.
Original comment from the bot: #816 (comment)
| // SPDX-FileCopyrightText: Copyright (c) 2019-2025 Aibolit | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| public class NoLoopOutsider { |
There was a problem hiding this comment.
Thank you for these new tests!
There was a problem hiding this comment.
@ivanovmg , look.
The check
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_REFERENCE # Find variables affected by assignments
for node in ast.get_proxy_nodes(ASTNodeType.ASSIGNMENT):
var_changes.add(node.expressionl) # expressionl is ASTNodeType.MEMBER_REFERENCE too!
To conclue, all objects in var_changes are gurantee to have .member propertie
|
@SinbadTheSailor2005 please, make the title of this PR more meaningful |
SinbadTheSailor2005
changed the title
SinbadTheSailor2005
changed the title
Loop outsider
SinbadTheSailor2005
changed the title
Loop outsider Loop Outsider Closes #759
SinbadTheSailor2005
changed the title
Loop Outsider Closes #759 Loop Outsider
@yegor256, fixed |
SinbadTheSailor2005
changed the title
Loop Outsider LoopOutsider pattern
|
@rultor merge |
1 similar comment
|
@rultor merge |
@SinbadTheSailor2005 @yegor256 The pull request is closed already, so I can't merge it. |
|
@ivanovmg 🎉 Great job on your review! You've earned an impressive 18 points: 12 base points plus 5 bonus points for your 22 insightful comments. Your dedication is paying off – your total balance is now 256 points. Keep up the excellent work! 💪 |
|
@SinbadTheSailor2005 Great work on your contribution! 🎉 You've earned +16 points: +16 base, +6 for 133 hits-of-code, and -6 for exceeding comment limit (33 > 8). Keep up the momentum! Your current balance is -12. We appreciate your effort and look forward to more quality submissions! |
|
@yegor256 , I noticed that my GitHub profile is not showing up in the list of contributors. Could you please add me? |
5 participants
Closes #759
Implement loop outsider detector which works with FOR, WHILE, and DO statements
Also fix typo in tests: line number counts from class declaration, not from the beginning of the file
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Chores