Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: go-to-def and find-references on control-flow keywords #17542

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

roife
Copy link
Member

@roife roife commented Jul 4, 2024

fix #17517.

This PR implements go-to-definition and find-references functionalities for control flow keywords, which is similar to the behaviors in the highlight-related module. Besides, this PR also fixes some incorrect behaviors in highlight-related.

Changes

  1. Support for go-to-definition on control flow keywords:
    This PR introduces functionality allowing users to navigate to the definition of control flow keywords (return, break, continue).
    Commit: 7ab3407..560c098.

  2. Bug fixes and refactoring in highlight-related:

    • Handling return/break/continue within try_blocks:
      This PR adjusted the behavior of these keywords when they occur within try_blocks. When encounter these keywords, the program should exit the outer function or loop which containing the try_blocks, rather than the try_blocks itself; while the ? will cause the program to exit try_blocks.
      Commit: 6b7bb36.
    • Support highlighting keywords in macro expansion for highlight-related:
      Commit: 7e61f74.
    • Detailed description for the bug fixes
      • The previous implementation of preorder_expr incorrectly treated try_blocks as new contexts, thereby r-a will not continue to traverse inner return and break/continue statements. To resolve this, a new function preorder_expr_with_ctx_checker has been added, allowing users to specify which expressions to skip.
        • For example, when searching for the ? in the context, r-a should skip try_blocks where the ? insides just works for try_blocks. But when search for the return keyword, r-a should collect both the return keywords inside and outside the try_blocks
      • Thus, this PR added WalkExpandedExprCtx (builder pattern). It offers the following improvements: customizable context skipping, maintenance of loop depth (for break/continue), and handling macro expansion during traversal.
  3. Support for find-references on control flow keywords:
    This PR enables users to find all references to control flow keywords.
    Commit: 2262ab7.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2024
crates/ide/src/goto_definition.rs Show resolved Hide resolved
crates/ide/src/goto_definition.rs Show resolved Hide resolved
crates/ide/src/goto_definition.rs Show resolved Hide resolved
@@ -153,6 +153,22 @@ impl NavigationTarget {
)
}

pub(crate) fn from_expr(
Copy link
Member

Choose a reason for hiding this comment

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

Hm actually I think this is the wrong way to go about this, let's construct these manually in the relevant modules and instead expose the orig_range_with_focus functions to the rest of the crate

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

One thing to look out for is the turning FileRange into TextRange after upmapping. All of those need to verify that the file id of the FileRange is in fact the same as the file we are doing the feature stuff in, as include! has its input in a different file than where the call is. Generally speaking, dropping the file id of a FileRange yielded from upmapping without checks can easily cause issues

};
let focus_range = InFile::new(file_id, focus_token.text_range())
.original_node_file_range_opt(db)
.map(|(frange, _)| frange.range);
Copy link
Member

Choose a reason for hiding this comment

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

Needs to filter by frange.file_id == nav.file_id


InFile::new(file_id, text_range.unwrap())
.original_node_file_range_opt(db)
.map(|(frange, _)| frange.range)
Copy link
Member

Choose a reason for hiding this comment

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

Needs to filer by frange.file_id == highlight_references::file_id (that is the file_id of the file the feature is running on)

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More IDE features for control flow keywords
3 participants