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 3 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());
}

crates/syntax/src/syntax_editor/edit_algo.rs Outdated Show resolved Hide resolved
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.

@Giga-Bowser Giga-Bowser force-pushed the migrate-wrap-unwrap-return branch 2 times, most recently from 842bcdc to 9f5cc26 Compare November 21, 2024 05:04
@bors
Copy link
Contributor

bors commented Dec 5, 2024

☔ The latest upstream changes (presumably #18483) made this pull request unmergeable. Please resolve the merge conflicts.

@Giga-Bowser Giga-Bowser force-pushed the migrate-wrap-unwrap-return branch 2 times, most recently from 21e9566 to b9db9e9 Compare December 6, 2024 14:33
@rustbot rustbot added has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2024
@rustbot

This comment has been minimized.

@Giga-Bowser Giga-Bowser force-pushed the migrate-wrap-unwrap-return branch from 97c1ee7 to f62ef42 Compare December 6, 2024 14:37
@rustbot rustbot removed has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2024
@Giga-Bowser
Copy link
Contributor Author

I noticed that in #18551, the following turbofish_generic_arg_list constructor was added to SyntaxFactory

pub fn turbofish_generic_arg_list(
    &self,
    args: impl IntoIterator<Item = ast::GenericArg> + Clone,
) -> ast::GenericArgList {
    let ast = make::turbofish_generic_arg_list(args.clone()).clone_for_update();

    if let Some(mut mapping) = self.mappings() {
        let mut builder = SyntaxMappingBuilder::new(ast.syntax().clone());
        builder.map_children(
            args.into_iter().map(|arg| arg.syntax().clone()),
            ast.generic_args().map(|arg| arg.syntax().clone()),
        );
        builder.finish(&mut mapping);
    }

    ast
}

But typically, I would have written this constructor as follows

pub fn turbofish_generic_arg_list(
    &self,
    args: impl IntoIterator<Item = ast::GenericArg>,
) -> ast::GenericArgList {
    let args: Vec<ast::GenericArg> = args.into_iter().collect();
    let input: Vec<ast::SyntaxNode> = args.iter().map(|it| it.syntax().clone()).collect();
    let ast = make::turbofish_generic_arg_list(args).clone_for_update();
    
    if let Some(mut mapping) = self.mappings() {
        let mut builder = SyntaxMappingBuilder::new(ast.syntax().clone());
        builder.map_children(
            input.into_iter(),
            ast.generic_args().map(|arg| arg.syntax().clone()),
        );
        builder.finish(&mut mapping);
    }
    
    ast
}

The difference being how args is taken as a parameter. Is there a reason to use one approach over another? Are there any common types which implement IntoIterator but not Clone?

@Giga-Bowser Giga-Bowser force-pushed the migrate-wrap-unwrap-return branch from f62ef42 to 355c4d8 Compare December 7, 2024 12:50
@Veykril Veykril requested a review from DropDemBits December 9, 2024 11:13
@bors
Copy link
Contributor

bors commented Dec 11, 2024

☔ The latest upstream changes (presumably #18657) made this pull request unmergeable. Please resolve the merge conflicts.

@Giga-Bowser Giga-Bowser force-pushed the migrate-wrap-unwrap-return branch from 355c4d8 to 491dc94 Compare December 11, 2024 14:49
@bors
Copy link
Contributor

bors commented Dec 12, 2024

☔ The latest upstream changes (presumably #18652) made this pull request unmergeable. Please resolve the merge conflicts.

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.

4 participants