sourcery-ai / sourcery

Instant AI code reviews
https://sourcery.ai
MIT License
1.55k stars 68 forks source link

Very strange nonsensical refactoring suggestion. #295

Closed threadreaper closed 1 year ago

threadreaper commented 1 year ago

Checklist

Description

I got a really weird refactoring suggestion from Sourcery tonight. I have the following class in the project I'm working on:

class BinOp(Expression):
    """Defines a binary operation expression.

    Attributes:
        ttype (str): The type of expression
        left (Expression): The expression's lefthand term
        oper (Token): The expression's operator
        right (Expression): The expression's righthand term"""

    def __init__(self: object, left: Expression, op: Token, right: Expression) -> None:
        """Constructor for binary operation expression objects.

        Args:
            left (Expression): The expression's lefthand term
            op (Token): The expression's operator
            right (Expression): The expression's righthand term"""

        super().__init__()
        self.ttype = 'binary'
        self.left = left
        self.oper = op
        self.right = right

    def accept(self: object, visitor: object) -> None:
        """Callback to the visitor's binary operation method.

        Args:
            visitor (object): reference to the visitor object."""

        visitor.visit_binop(self)

Sourcery is triggering on the blank line right after the docstring in the accept() method, with the following message: Sourcery - Remove return or yield statements found outside function definitions

The suggested refactoring is:

Function BinOp.accept refactored with the following changes:

Remove return or yield statements found outside function definitions 

-    def accept(self: object, visitor: object) -> None:
-        """Callback to the visitor's binary operation method.
+    def _is_at_end(self: object) -> bool:
+        """Look ahead and check if the next token is the end of the input.

-        Args:
-            visitor (object): reference to the visitor object."""
-
-        visitor.visit_binop(self)
+        Returns:
+            bool: True if the next token is an end of input token, False otherwise"""

The _is_at_end() method belongs to a whole different class, and Sourcery wants to insert that method declaration and its docstring in place of the whole block of the accept() method here. If I delete the blank line after the docstring in the accept method, the suggestion goes away. I'm not sure what could be triggering it, because there are no yield or return statements here, and if there were, they would be inside of a function definition anyway.

I will attach an excerpt from sourcery.log with what appears to be the relevant lines. sourcery log.txt

I really like Sourcery! This is the first time I've had any sort of an issue. Let me know if there's any more information I can provide that may be helpful.

Debug Information

IDE Version: VSCode v1.72.2

Sourcery Version: Sourcery 0.12.12

Operating system and Version: Windows 10

Hellebore commented 1 year ago

@threadreaper thanks for raising this - definitely pretty weird.

Would you be able to post or email the whole file (or a minimal example to reproduce the issue)?

threadreaper commented 1 year ago

Unfortunately this happened in something I'm actively working on, and there have been a lot of changes to the file since I posted this. I'll do a little fiddling and see if I can reproduce the issue, but I have my doubts.

Hellebore commented 1 year ago

Will close for now - please do reopen if it reoccurs or there are any more details.