Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 139 additions & 11 deletions aibolit/patterns/bidirect_index/bidirect_index.py
Original file line number Diff line number Diff line change
@@ -1,25 +1,153 @@
# SPDX-FileCopyrightText: Copyright (c) 2019-2025 Aibolit
# SPDX-License-Identifier: MIT
import os
import re


class BidirectIndex:
"""
This class analyzes Java source code to find line numbers where a variable
is used as a bidirectional index.
A bidirectional index is defined as a variable that:
- is assigned a value (with or without type declaration) within a method or block,
- is incremented and decremented somewhere in its scope,
- increments/decrements inside a for-loop with a local variable of the same name
are ignored (to avoid "fake" cases).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe indent it to match the indentation of the corresponding item

Suggested change
- increments/decrements inside a for-loop with a local variable of the same name
are ignored (to avoid "fake" cases).
- increments/decrements inside a for-loop with a local variable of the same name
are ignored (to avoid "fake" cases).


The typical use-case: detect patterns like `i = 0; ... ++i; ... --i;`
in Java code, while ignoring manipulations of `i` inside
loops where `i` is a local loop variable (e.g., `for (int i = 0; ...) { ... }`).

Usage:
idx = BidirectIndex()
result = idx.value('MyClass.java')
# result is a list of line numbers matching the described pattern
"""

def __init__(self):
pass

def value(self, filename: str | os.PathLike):
@staticmethod
def value(filename: str | os.PathLike):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the interface described here: #814 (related to #813).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, in this implementation you totally relied on the literate source code of the file, rather than the abstract syntax tree.
Although, your solution covers all test cases, it goes against the direction on the unification of the Pattern interface.

I suppose that if this is a working solution, then having resolved the comments, it may be merged with a puzzle to refactor it into using an ast_framework.AST rather than a filename as an input parameter.
@AntonProkopyev can you suggest the way forward here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest against use of a static method, at least in this particular case, where it is a public method.

Suggested change
@staticmethod
def value(filename: str | os.PathLike):
def value(self, filename: str | os.PathLike):

"""
Analyze the given Java file and return a sorted list of line numbers where a variable
is used as a bidirectional index as per the definition above.

Args:
filename: Path to the Java source file.

Returns:
List[int]: Sorted list of line numbers where bidirectional indices are found.
"""
Finds if a variable is being incremented and decremented within the same method
with open(filename, encoding='utf-8') as f:
lines = f.readlines()

result = []
for m_start, m_end in BidirectIndex.find_methods(lines):
BidirectIndex.analyze_block(lines, m_start + 1, m_end, result)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe use instance methods, rather than static methods?

Suggested change
for m_start, m_end in BidirectIndex.find_methods(lines):
BidirectIndex.analyze_block(lines, m_start + 1, m_end, result)
for m_start, m_end in self.find_methods(lines):
self.analyze_block(lines, m_start + 1, m_end, result)

Or, if reasonable, we can extract functions, that detect the pattern, but make BidirectIndex.value simply call this function.

return sorted(result)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

:param filename: filename to be analyzed
:return: list of LineNumber with the variable declaration lines
@staticmethod
def find_methods(lines):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider removing staticmethod everywhere, or extracting a related functionality into separate functions (https://github.com/cqfn/aibolit/pull/842/files#r2176314046).

Suggested change
@staticmethod
def find_methods(lines):
def _find_methods(self, lines):

"""
Find the start and end line indices (0-based) for each Java method in the file.
"""
res = []
brace = 0
mstart = None
for idx, line in enumerate(lines):
if re.search(r'\bvoid\b\s+\w+\s*\(.*\)\s*{', line):
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if mstart is None:
mstart = idx
brace = 0
brace += line.count('{')
brace -= line.count('}')
if mstart is not None and brace == 0:
res.append((mstart, idx))
mstart = None
return res

@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
@staticmethod
def analyze_block(lines, start, end, result):
"""
Recursively analyze a block of code between 'start' and 'end' (inclusive).
"""
i = start
while i <= end:
line = lines[i]
typed_decl = re.match(r'\s*(int|long|byte|short)\s+(\w+)\s*=', line)
untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line) \
if not typed_decl else None
var = typed_decl.group(2) if typed_decl else (untyped_decl.group(1)
if untyped_decl else None)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve untyped variable detection to avoid false positives.

The current regex for untyped declarations could match invalid patterns. In Java, variable reassignments must be preceded by a declaration. Consider checking if the variable was previously declared.

-            untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line) \
-                if not typed_decl else None
+            # Only match simple reassignments, not method calls or field access
+            untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^=;]+;', line) \
+                if not typed_decl else None
📝 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.

Suggested change
untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^;]+;', line) \
if not typed_decl else None
var = typed_decl.group(2) if typed_decl else (untyped_decl.group(1)
if untyped_decl else None)
# Only match simple reassignments, not method calls or field access
untyped_decl = re.match(r'^\s*(\w+)\s*=\s*[^=;]+;', line) \
if not typed_decl else None
var = typed_decl.group(2) if typed_decl else (untyped_decl.group(1)
if untyped_decl else None)
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 61 to 64, the
regex used to detect untyped variable declarations may produce false positives
by matching invalid patterns. To fix this, enhance the logic to verify that the
variable identified by the regex was previously declared before treating it as a
valid untyped declaration. This can be done by maintaining a record of declared
variables and checking against it before assigning the variable from the
untyped_decl match.

if var:
j = i + 1
while j <= end:
line_ = lines[j]
if re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_):
break
j += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Check for both typed and untyped redeclarations when finding variable scope.

The current implementation only checks for typed redeclarations, missing cases where the variable is reassigned without a type.

-                    if re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_):
+                    # Check for both typed declarations and simple reassignments
+                    if (re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_) or
+                        re.match(r'^\s*' + re.escape(var) + r'\s*=\s*[^=;]+;', line_)):
📝 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.

Suggested change
if re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_):
break
j += 1
# Check for both typed declarations and simple reassignments
if (re.match(r'\s*(int|long|byte|short)\s+' + re.escape(var) + r'\s*=', line_) or
re.match(r'^\s*' + re.escape(var) + r'\s*=\s*[^=;]+;', line_)):
break
j += 1
🤖 Prompt for AI Agents
In aibolit/patterns/bidirect_index/bidirect_index.py around lines 69 to 71, the
code only checks for typed redeclarations of variables but misses untyped
reassignments. Update the condition to also detect lines where the variable is
reassigned without a type, such as simple assignments, by adding a check for
untyped redeclarations alongside the existing typed check.

for_blocks = BidirectIndex.find_for_blocks(lines, var, i + 1, j)
inc_outside, dec_outside = (
BidirectIndex.count_inc_dec(lines, var, for_blocks, i + 1, j))
if inc_outside and dec_outside:
Comment thread
coderabbitai[bot] marked this conversation as resolved.
result.append(i + 1)
if '{' in line:
brace, block_start = 1, i
i += 1
while i <= end:
brace += lines[i].count('{')
brace -= lines[i].count('}')
if brace == 0:
BidirectIndex.analyze_block(lines, block_start + 1, i - 1, result)
break
i += 1
else:
i += 1

@staticmethod
def find_for_blocks(lines, var, start, end):
"""
Find all ranges of for-loops that declare a local variable with the given name.
"""
for_blocks = []
k = start
while k < end:
line_ = lines[k]
for_decl = re.match(r'\s*for\s*\(\s*int\s+' + re.escape(var) + r'\s*=', line_)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if for_decl:
brace = line_.count('{') - line_.count('}')
bstart = k
k += 1
while k < end and brace > 0:
brace += lines[k].count('{') - lines[k].count('}')
k += 1
for_blocks.append((bstart, k - 1))
continue
k += 1
return for_blocks

@staticmethod
def count_inc_dec(lines, var, for_blocks, start, end):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that all methods apart from value are made internal by adding a leading underscore. Or, if you gravitate towards static methods, then better extract functions, outside the class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/cqfn/aibolit/pull/842/files#r2176330476

Suggested change
@staticmethod
def count_inc_dec(lines, var, for_blocks, start, end):
def _count_inc_dec(self, lines, var, for_blocks, start, end):

"""
Count increments and decrements of variable `var` outside any for-blocks.
"""
return []
inc_outside = 0
dec_outside = 0
k = start
while k < end:
in_for = any(bstart <= k <= bend for (bstart, bend) in for_blocks)
if not in_for:
line_ = lines[k]
if re.search(
r'(\+\+' + re.escape(var) + r'|' + re.escape(var) + r'\+\+|' +
re.escape(var) + r'\s*\+=\s*1\b|' +
re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*\+\s*1\b)', line_):
inc_outside += 1
if re.search(
r'(--' + re.escape(var) + r'|' + re.escape(var) + r'--|' +
re.escape(var) + r'\s*-=\s*1\b|' +
re.escape(var) + r'\s*=\s*' + re.escape(var) + r'\s*-\s*1\b)', line_):
dec_outside += 1
k += 1
return inc_outside, dec_outside
2 changes: 0 additions & 2 deletions test/patterns/bidirect_index/test_bidirect_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@
import os.path
from pathlib import Path
from unittest import TestCase
import unittest
from aibolit.patterns.bidirect_index.bidirect_index import BidirectIndex


@unittest.skip('Not implemented')
class BidirectIndexTestCase(TestCase):
dir_path = Path(os.path.realpath(__file__)).parent

Expand Down
Loading