Skip to content

Improve find referenced symbols#116

Merged
robmck1995 merged 33 commits intomainfrom
rob
Jan 23, 2025
Merged

Improve find referenced symbols#116
robmck1995 merged 33 commits intomainfrom
rob

Conversation

@robmck1995
Copy link
Copy Markdown
Contributor

@robmck1995 robmck1995 commented Jan 22, 2025

Description

  • Added a new method get_symbol_and_references to improve the retrieval of symbols and their references.
  • Introduced a static CALLABLE_TYPES for multiple languages and an is_callable method to check if a symbol is callable.
  • Enhanced error handling by adding a new error type RecursionLimitExceeded.
  • Refactored and simplified the logic for checking external or callable definitions, including adding a recursive depth limit to prevent excessive recursion.
  • Added once_cell as a dependency to the project.

Changes walkthrough

Relevant files
Enhancement
client.rs
Add method for symbol and references retrieval                                 

lsproxy/src/ast_grep/client.rs

  • Added a new method get_symbol_and_references for retrieving symbols
    and their references.
  • Improved symbol and references retrieval logic.
  • +11/-0   
    types.rs
    Add callable types and is_callable method                                           

    lsproxy/src/ast_grep/types.rs

  • Introduced a static CALLABLE_TYPES for multiple languages.
  • Added is_callable method to check if a symbol is callable.
  • +69/-0   
    manager.rs
    Refactor and enhance definition resolution logic                             

    lsproxy/src/lsp/manager/manager.rs

  • Refactored logic for checking external or callable definitions.
  • Added recursive depth limit to definition resolution.
  • Simplified symbol match and references retrieval.
  • +122/-177
    Error handling
    error.rs
    Add error handling for recursion limit                                                 

    lsproxy/src/handlers/error.rs

    • Added error handling for RecursionLimitExceeded.
    +3/-0     
    Dependencies
    Cargo.toml
    Add once_cell dependency                                                                             

    lsproxy/Cargo.toml

    • Added once_cell dependency.
    +1/-0     
    💡 Usage Guide

    Checking Your Pull Request

    Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

    Talking to CodeAnt AI

    Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

    @codeant-ai ask: Your question here
    

    This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

    Retrigger review

    Ask CodeAnt AI to review the PR again, by typing:

    @codeant-ai: review
    

    Check Your Repository Health

    To analyze the health of your code repository, visit our dashboard at app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

    @codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Jan 22, 2025
    @codeant-ai
    Copy link
    Copy Markdown
    Contributor

    codeant-ai Bot commented Jan 22, 2025

    Pull Request Feedback 🔍

    🔒 No security issues identified
    ⚡ Recommended areas for review

    Error Handling
    The new method get_symbol_and_references uses Box<dyn std::error::Error> for error handling. Consider using a more specific error type to improve error handling and debugging.

    Static Initialization
    The static CALLABLE_TYPES is initialized with a large number of entries. Ensure that this initialization is efficient and does not impact performance negatively.

    Recursion Limit
    The method resolve_definition_chain introduces a recursion limit with MAX_DEPTH. Ensure that this limit is appropriate for all use cases and consider making it configurable if necessary.

    Comment thread lsproxy/src/ast_grep/client.rs Outdated
    Comment on lines +62 to +63
    let symbol_match = self.get_symbol_match_from_position(file_name, position).await?;
    let references = self.get_references_contained_in_symbol_match(file_name, &symbol_match, full_scan).await?;
    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.

    Suggestion: Consider handling the case where get_symbol_match_from_position or get_references_contained_in_symbol_match might return an error, to provide more context or a specific error message. [best practice]

    Suggested change
    let symbol_match = self.get_symbol_match_from_position(file_name, position).await?;
    let references = self.get_references_contained_in_symbol_match(file_name, &symbol_match, full_scan).await?;
    let symbol_match = self.get_symbol_match_from_position(file_name, position).await.map_err(|e| format!("Error getting symbol match: {}", e))?;
    let references = self.get_references_contained_in_symbol_match(file_name, &symbol_match, full_scan).await.map_err(|e| format!("Error getting references: {}", e))?;

    Comment thread lsproxy/src/handlers/error.rs Outdated
    Comment on lines +28 to +30
    Self::RecursionLimitExceeded(msg) => HttpResponse::BadRequest().json(ErrorResponse {
    error: format!("Recursion limit exceeded: {}", msg)
    }),
    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.

    Suggestion: Ensure that the RecursionLimitExceeded error is handled consistently with other errors, potentially by logging or monitoring for better traceability. [maintainability]

    Suggested change
    Self::RecursionLimitExceeded(msg) => HttpResponse::BadRequest().json(ErrorResponse {
    error: format!("Recursion limit exceeded: {}", msg)
    }),
    Self::RecursionLimitExceeded(msg) => {
    log::error!("Recursion limit exceeded: {}", msg);
    HttpResponse::BadRequest().json(ErrorResponse {
    error: format!("Recursion limit exceeded: {}", msg)
    })
    }

    @robmck1995 robmck1995 marked this pull request as ready for review January 23, 2025 13:28
    @robmck1995 robmck1995 merged commit 9bd7d01 into main Jan 23, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    size:L This PR changes 100-499 lines, ignoring generated files

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants