james-ansley / qchecker Goto Github PK

Currently, duplicate statement detection in conditionals is only detected in if/else statements. There should be a substructure to check for duplicate statements across more complex if/elif/else statements.

For example, the following code should entirely be matched by a single substructure:

if x == 1:
    print("Block 1")
    do_something(x)
elif x == 2:
    print("Block 2")
    do_something(x)
else:
    print("Block 3")
    do_something(x)

This could be done, for example, with the addition of a DuplicateIfElifElseStatement and a corresponding statements substructure. This should be designed in a way to handle variable numbers of elif blocks.

The descriptions module not only allows for custom descriptions to be used but can also be used for language support. In order for this to fully support non-English languages, pattern names (and possibly technical descriptions) should be handled by this module giving users of this package full control over all pattern content.

It is unclear whether this module should be completely changed to more directly indicate multi-language support with explicit references to language codes etc, or whether it should keep its name and general setup just with additional features.

IfReturnBool substructure matches an if-else block with a non-empty else that is followed by a return bool.

from qchecker.substructures import IfReturnBool

code = """
def search(root, value):
    if x:
        return True
    else:
        print("Whoops! a wild side effect has appeared!")
    return False
"""
match, = IfReturnBool.iter_matches(code)
print(match)
print(match.text_range.grab_range(code))

Produces:

Match("If Return Bool", "Looks like you are returning the [...]", TextRange(3,4->7,16))
if x:
    return True
else:
    print("Whoops! a wild side effect has appeared!")
return False

This should not be matched

The NestedIf substructure matches an if-statement nested inside an elif statement.

from qchecker.substructures import NestedIf

code = r'''
def foo(x):
    if 0 <= x < 5:
        do_something(x)
    elif 5 <= x < 10:
        if x % 2 == 0:
            do_another_thing(x)
'''

match, = NestedIf.iter_matches(code)
print(match)
print(match.text_range.grab_range(code))

Produces:

Match("Nested If", "Looks like you are trying to test [...]", TextRange(5,4->7,31))
elif 5 <= x < 10:
    if x % 2 == 0:
        do_another_thing(x)

The name and description mention nesting two if-statements inside one another which might be confusing for students.
A separate ElifIf substructure should be made to align with the current NestedIf and ElseIf substructures

DuplicateIfElseStatement and SeveralDuplicateIfElseStatements incorrectly identify elif blocks as if blocks. This leads them to make incorrect non-equivalent refactoring suggestions.

For example, with DuplicateIfElseStatement:

from qchecker.substructures import DuplicateIfElseStatement

code = r'''
if x == 1:
    and_now_for_something_completely_different(x)
elif x == 2:
    do_something(x)
    print("A side-effect")
else:
    do_something_else(x)
    print("A side-effect")
'''

match, = DuplicateIfElseStatement.iter_matches(code)
print(match)
print(match.text_range.grab_range(code))

Produces:

Match("Duplicate If/Else Statement", "It looks like the code at the end [...]", TextRange(4,0->9,26))
elif x == 2:
    do_something(x)
    print("A side-effect")
else:
    do_something_else(x)
    print("A side-effect")

The suggestion that the duplicate statement should be moved outside the elif and else body is incorrect and these substructures should ignore this case.

Empty Elif blocks are matched with the EmptyIfBody substructure

from qchecker.substructures import EmptyIfBody

code = r'''
def foo(x):
    if 0 <= x < 5:
        do_something(x)
    elif 5 <= x < 10:
        ...
    else:
        do_something_else(x)
'''

match, = EmptyIfBody.iter_matches(code)
print(match)
print(match.text_range.grab_range(code))

Produces:

Match("Empty If Body", "Looks like the body of this if [...]", TextRange(5,4->6,11))
elif 5 <= x < 10:
    ...

Since the match name and description only mention if-blocks, matching an elif block may be confusing.
A new EmptyElifBody substructure should be added to make the distinction clearer.

WhileAsFor incorrectly identifies while loops that possibly mutate elements in the while test as being replaceable with a for loop.

from qchecker.substructures import WhileAsFor

code = r'''
while x < len(y):
    y.pop()
    x += 1
    print('do something')
'''

match, = WhileAsFor.iter_matches(code)
print(match)
print(match.text_range.grab_range(code))

Produces:

Match("While as For", "It looks like this while loop [...]", TextRange(2,0->5,25))
while x < len(y):
    y.pop()
    x += 1
    print('do something')

This method should ignore cases where there are method calls on the names in the while body, additional reassignments, etc.

RepeatedAddition and RepeatedMultiplication will incorrectly identify adjacent names as being added/multiplied when they are not.

from qchecker.substructures import RepeatedAddition, RepeatedMultiplication

code = r'''
y * x + x
y ** x * x * x
'''.strip()

match, = RepeatedAddition.iter_matches(code)
match1, = RepeatedMultiplication.iter_matches(code)
print(match)
print(match1)

Yields:

Match("Repeated Addition", "It looks like you are repeating [...]", TextRange(1,0->1,9))
Match("Repeated Multiplication", "It looks like you are repeating [...]", TextRange(2,0->2,14))

These should not be matched. It is likely the hacky regex based approach may need to be replaced with something more robust.