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

minor: Migrate (un)wrap_return_type assists to use SyntaxEditor #18524

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

Conversation

Giga-Bowser
Copy link
Contributor

@Giga-Bowser Giga-Bowser commented Nov 17, 2024

Part of #15710 and #18285

This PR also includes some small fixes to SyntaxEditor, in particular a fix to the problem with affected_range outlined here.

I also updated unwrap_return_type to use placeholders when unwrapping None. I had tried to do this with the mutable tree API before and it was kind of a pain so it's nice to see improvements like this :)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2024
@Giga-Bowser Giga-Bowser force-pushed the migrate-wrap-unwrap-return branch 2 times, most recently from f2c9293 to 8542c04 Compare November 17, 2024 21:16
Copy link
Contributor

@DropDemBits DropDemBits left a comment

Choose a reason for hiding this comment

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

I can't add a comment to the file in question, but it would be good also add a test in syntax_editor::tests testing that token edits in a parent node won't be considered ancestors of edits in the child node:

#[test]
fn test_replace_token_in_parent() {
    let parent_fn = make::fn_(
        None,
        make::name("it"),
        None,
        None,
        make::param_list(None, []),
        make::block_expr([], Some(make::expr_unit())),
        Some(make::ret_type(make::ty_unit())),
        false,
        false,
        false,
        false,
    );

    let mut editor = SyntaxEditor::new(parent_fn.syntax().clone());

    if let Some(ret_ty) = parent_fn.ret_type() {
        editor.delete(ret_ty.syntax().clone());

        if let Some(SyntaxElement::Token(token)) = ret_ty.syntax().next_sibling_or_token() {
            if token.kind().is_trivia() {
                editor.delete(token);
            }
        }
    }

    if let Some(tail) = parent_fn.body().unwrap().tail_expr() {
        editor.delete(tail.syntax().clone());
    }

    let edit = editor.finish();

    let expect = expect![[r#"
        fn it() {

        }"#]];
    expect.assert_eq(&edit.new_root.to_string());
}

@@ -73,7 +73,7 @@ pub(super) fn apply_edits(editor: SyntaxEditor) -> SyntaxEdit {
})
.all(|(l, r)| {
get_node_depth(l.target_parent()) != get_node_depth(r.target_parent())
|| l.target_range().intersect(r.target_range()).is_none()
|| (l.target_range().end() <= r.target_range().start())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also do

Suggested change
|| (l.target_range().end() <= r.target_range().start())
|| l.target_range().intersect(r.target_range()).is_none_or(|it| it.is_empty())

TextRange::intersect returns an empty range if the ranges are touching but not overlapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that but figured I'd rather just be straightforward with what's being checked especially since the ranges are sorted so there's no need to check if l starts inside of r

crates/syntax/src/syntax_editor/edit_algo.rs Outdated Show resolved Hide resolved
crates/syntax/src/ast/syntax_factory/constructors.rs Outdated Show resolved Hide resolved
crates/syntax/src/ast/syntax_factory/constructors.rs Outdated Show resolved Hide resolved
crates/syntax/src/ast/syntax_factory/constructors.rs Outdated Show resolved Hide resolved
crates/syntax/src/ast/syntax_factory/constructors.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/wrap_return_type.rs Outdated Show resolved Hide resolved
@DropDemBits
Copy link
Contributor

Forgot to mention that the final tabstop snippet for unwrap_return_type should be after the last placeholder node instead of the last placeholder being considered the end of the snippet mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants