Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented May 30, 2025

PR Type

Bug fix


Description

  • Remove nested function visits in FunctionDef

  • Prevent nested functions from being counted

  • Simplify visitor logic for FunctionDef nodes


Changes walkthrough 📝

Relevant files
Bug fix
functions_to_optimize.py
Skip nested functions in visitor                                                 

codeflash/discovery/functions_to_optimize.py

  • Deleted generic_visit call inside visit_FunctionDef
  • Stops traversing into nested functions
  • Keeps only top-level functions with returns
  • +0/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Recursion in generic_visit

    The custom generic_visit appends to ast_path but does not explicitly call super().generic_visit(node), which may prevent traversal of child nodes. Verify that recursion still occurs as intended.

    def generic_visit(self, node: ast.AST) -> None:
        if isinstance(node, (FunctionDef, AsyncFunctionDef, ClassDef)):
            self.ast_path.append(FunctionParent(node.name, node.__class__.__name__))
    Missing AsyncFunctionDef override

    Nested async functions aren’t excluded because only visit_FunctionDef was changed. Consider overriding visit_AsyncFunctionDef similarly to skip nested async defs.

    def visit_FunctionDef(self, node: FunctionDef) -> None:
        # Check if the function has a return statement and add it to the list
        if function_has_return_statement(node) and not function_is_a_property(node):
            self.functions.append(
    AST path cleanup

    ast_path entries are pushed in generic_visit but no corresponding pop is shown. Ensure the parent stack is correctly restored after visiting a node.

    def generic_visit(self, node: ast.AST) -> None:
        if isinstance(node, (FunctionDef, AsyncFunctionDef, ClassDef)):
            self.ast_path.append(FunctionParent(node.name, node.__class__.__name__))

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    restore traversal and manage path

    Ensure that after appending to ast_path you actually traverse the node’s children
    and then pop the entry to avoid an ever-growing stack and to continue visiting other
    nodes. Without calling super().generic_visit, child nodes won’t be visited, and
    without pop, your path tracking will break.

    codeflash/discovery/functions_to_optimize.py [98-100]

     def generic_visit(self, node: ast.AST) -> None:
         if isinstance(node, (FunctionDef, AsyncFunctionDef, ClassDef)):
             self.ast_path.append(FunctionParent(node.name, node.__class__.__name__))
    +        super().generic_visit(node)
    +        self.ast_path.pop()
    +    else:
    +        super().generic_visit(node)
    Suggestion importance[1-10]: 10

    __

    Why: The suggestion restores AST child traversal and ensures the path stack is properly popped, fixing a critical bug where nodes were neither visited nor cleaned up.

    High
    General
    guarantee path cleanup

    Wrap the append/pop logic in a try/finally block to guarantee that ast_path.pop()
    runs even if an exception occurs during traversal. This prevents the stack from
    becoming inconsistent on errors.

    codeflash/discovery/functions_to_optimize.py [98-100]

     def generic_visit(self, node: ast.AST) -> None:
         if isinstance(node, (FunctionDef, AsyncFunctionDef, ClassDef)):
             self.ast_path.append(FunctionParent(node.name, node.__class__.__name__))
    +        try:
    +            super().generic_visit(node)
    +        finally:
    +            self.ast_path.pop()
    +    else:
    +        super().generic_visit(node)
    Suggestion importance[1-10]: 8

    __

    Why: Wrapping traversal in a try/finally block ensures ast_path.pop() always runs, preventing stack corruption even on errors.

    Medium
    selectively traverse non–function nodes

    If you want to skip nested functions but still traverse other statements inside a
    function, manually visit only non-function children. This preserves other analyses
    without counting nested defs.

    codeflash/discovery/functions_to_optimize.py [91-96]

     def visit_FunctionDef(self, node: FunctionDef) -> None:
         # Check if the function has a return statement and add it to the list
         if function_has_return_statement(node) and not function_is_a_property(node):
             self.functions.append(
                 FunctionToOptimize(function_name=node.name, file_path=self.file_path, parents=self.ast_path[:])
             )
    +    for child in node.body:
    +        if not isinstance(child, (FunctionDef, AsyncFunctionDef)):
    +            self.visit(child)
    Suggestion importance[1-10]: 7

    __

    Why: Manually iterating over node.body and skipping nested functions restores necessary traversal without counting nested defs, improving overall analysis.

    Medium

    @KRRT7 KRRT7 enabled auto-merge May 31, 2025 00:39
    @openhands-ai
    Copy link

    openhands-ai bot commented May 31, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • end-to-end-test

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #258

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    @KRRT7 KRRT7 merged commit e18dcde into main Jun 2, 2025
    16 of 17 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants