feat: convert unmerge_use to SyntaxFactory SyntaxEditor abstraction #18565

davidkurilla · 2024-11-26T19:09:57Z

Contributes to issue #18285

I think I fully converted this but tests are failing with the following output:

failures:

---- handlers::unmerge_use::tests::unmerge_glob_import stdout ----
thread 'handlers::unmerge_use::tests::unmerge_glob_import' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- handlers::unmerge_use::tests::unmerge_indented_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_indented_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable

---- handlers::unmerge_use::tests::unmerge_nested_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_nested_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable

---- handlers::unmerge_use::tests::unmerge_renamed_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_renamed_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable

---- handlers::unmerge_use::tests::unmerge_use_item_on_self stdout ----
thread 'handlers::unmerge_use::tests::unmerge_use_item_on_self' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable

---- handlers::unmerge_use::tests::unmerge_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable

---- handlers::unmerge_use::tests::unmerge_use_item_with_visibility stdout ----
thread 'handlers::unmerge_use::tests::unmerge_use_item_with_visibility' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable

---- tests::generated::doctest_unmerge_use stdout ----
thread 'tests::generated::doctest_unmerge_use' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:548:9:
assertion failed: !self.data().mutable


failures:
    handlers::unmerge_use::tests::unmerge_glob_import
    handlers::unmerge_use::tests::unmerge_indented_use_item
    handlers::unmerge_use::tests::unmerge_nested_use_item
    handlers::unmerge_use::tests::unmerge_renamed_use_item
    handlers::unmerge_use::tests::unmerge_use_item
    handlers::unmerge_use::tests::unmerge_use_item_on_self
    handlers::unmerge_use::tests::unmerge_use_item_with_visibility
    tests::generated::doctest_unmerge_use

test result: FAILED. 2323 passed; 8 failed; 1 ignored; 0 measured; 0 filtered out; finished in 2.38s

darichey

let tree: ast::UseTree = ctx.find_node_at_offset::<ast::UseTree>()?.clone_for_update();

This clone_for_update is the culprit of the assertion failed: !self.data().mutable errors. clone_for_update makes the whole tree mutable, and then we later make a SyntaxEditor from a node from the tree (new_parent). When applying the edits, we assume the tree we were given was immutable and do clone_for_update again, hitting this assertion.

Not for this PR, but it would be nice to check this assumption earlier in SyntaxEditor::new to make the mistake more clear.

darichey · 2024-11-26T23:19:47Z

crates/ide-assists/src/handlers/unmerge_use.rs

        )
        .clone_for_update();

        tree.remove();


This should use editor.delete(...)

Okay so I will remove the .clone_for_update() method. I do still have one more question. The method editor.delete(...) requires an element argument. What should I put in for this argument?

We want to pass a SyntaxNode which implements Element. In this case, the one from tree.

darichey · 2024-11-26T23:20:14Z

crates/ide-assists/src/handlers/unmerge_use.rs

-        ted::insert(Position::after(use_.syntax()), new_use.syntax());
+        editor.insert(Position::after(use_.syntax()), new_use.syntax());

        builder.replace(old_parent_range, new_parent.to_string());


This should use editor.replace(...)

Similar to the question above. It appears the old_parent_range and new_parent.to_string() arguments are invalid for the editor.replace method as is.

Right, we'll want to pass the corresponding SyntaxNodes instead. We don't need the range of the old parent, but the old parent itself. Similarly, we don't need the stringified new parent, but the new parent itself.

edit: Actually, we probably don't even need this replace call anymore! The logic with the mutable nodes was a bit roundabout. You can experiment and see if that's true.

The original bug seems to be fixed but I am getting a new error. This one appears similar to the other 'immutable tree' error I was having in add_braces.

failures:

---- handlers::unmerge_use::tests::unmerge_glob_import stdout ----
thread 'handlers::unmerge_use::tests::unmerge_glob_import' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use std::fmt::*;
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

---- handlers::unmerge_use::tests::unmerge_indented_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_indented_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use std::fmt::Display as Disp;

---- handlers::unmerge_use::tests::unmerge_nested_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_nested_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use foo::bar::baz::qux;

---- handlers::unmerge_use::tests::unmerge_renamed_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_renamed_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use std::fmt::Display as Disp;

---- handlers::unmerge_use::tests::unmerge_use_item stdout ----
thread 'handlers::unmerge_use::tests::unmerge_use_item' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use std::fmt::Display;

---- handlers::unmerge_use::tests::unmerge_use_item_on_self stdout ----
thread 'handlers::unmerge_use::tests::unmerge_use_item_on_self' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use std::process;

---- handlers::unmerge_use::tests::unmerge_use_item_with_visibility stdout ----
thread 'handlers::unmerge_use::tests::unmerge_use_item_with_visibility' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: pub use std::fmt::Display;

---- tests::generated::doctest_unmerge_use stdout ----
thread 'tests::generated::doctest_unmerge_use' panicked at /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rowan-0.15.15/src/cursor.rs:824:9:
immutable tree: use std::fmt::Display;

failures:
handlers::unmerge_use::tests::unmerge_glob_import
handlers::unmerge_use::tests::unmerge_indented_use_item
handlers::unmerge_use::tests::unmerge_nested_use_item
handlers::unmerge_use::tests::unmerge_renamed_use_item
handlers::unmerge_use::tests::unmerge_use_item
handlers::unmerge_use::tests::unmerge_use_item_on_self
handlers::unmerge_use::tests::unmerge_use_item_with_visibility
tests::generated::doctest_unmerge_use

test result: FAILED. 2323 passed; 8 failed; 1 ignored; 0 measured; 0 filtered out; finished in 1.84s

I think you're probably missing a clone_for_update. If you push your changes I can try to help debug.

bors · 2024-12-05T18:59:38Z

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

darichey

Adding those clone_for_updates will resolve the errors. Run the tests to see the two remaining problems:

We need to also remove the comma to turn use std::fmt::{Debug, }; into use std::fmt::{Debug};. This was handled for us here when calling tree.remove(). @tareknaser ran into the same problem in #18484 where you can see he introduced a new SyntaxEditor-based Removable trait and migrated that code. You could do the same thing here or just inline that code for now (either way, one of you will have to rebase over the other and fix up the code later).
We need to fix the whitespace: use std::fmt::{Debug};use std::fmt::Display;. This was handled for us here when calling ted::insert. We probably want a similar helper for SyntaxEditor to insert a fixup whitespace. Don't worry about this for now, I will look into it.

darichey · 2025-01-10T16:11:52Z

crates/syntax/src/ast/syntax_factory/constructors.rs

    }
+
+    pub fn use_(&self, visibility: Option<ast::Visibility>, use_tree: ast::UseTree) -> ast::Use {
+        make::use_(visibility, use_tree)


Suggested change

make::use_(visibility, use_tree)

make::use_(visibility, use_tree).clone_for_update()

I have implemented this change, but I am unable to run cargo test due to some build error on my side. I think it has to do with the new version of Rust, but I am unable to compile a number of crates like serde and linking with rust-lld with crates like libc and crossbeam-utils

darichey · 2025-01-10T16:11:59Z

crates/syntax/src/ast/syntax_factory/constructors.rs

+    }
+
+    pub fn use_tree(&self, path: ast::Path, use_tree_list: Option<ast::UseTreeList>, alias: Option<ast::Rename>, add_star: bool) -> ast::UseTree {
+        make::use_tree(path, use_tree_list, alias, add_star)


Suggested change

make::use_tree(path, use_tree_list, alias, add_star)

make::use_tree(path, use_tree_list, alias, add_star).clone_for_update()

I have implemented this change, but I am unable to run cargo test due to some build error on my side. I think it has to do with the new version of Rust, but I am unable to compile a number of crates like serde and linking with rust-lld with crates like libc and crossbeam-utils

Veykril · 2025-05-05T07:36:06Z

I believe this has been done in #19469

feat: convert unmerge_use to SyntaxFactory SyntaxEditor abstraction

cade0c9

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2024

darichey suggested changes Nov 26, 2024

View reviewed changes

davidkurilla requested a review from darichey November 28, 2024 15:49

fix: remove clone_for_update statement

c6df725

darichey suggested changes Jan 10, 2025

View reviewed changes

fix: add clone_for_update to SyntaxFactory methods

c8267e7

davidkurilla requested a review from darichey January 14, 2025 04:23

snprajwal mentioned this pull request Mar 27, 2025

refactor: migrate unmerge_use -> unmerge_imports to syntax editor #19465

Closed

Veykril closed this May 5, 2025

	make::use_(visibility, use_tree)
	make::use_(visibility, use_tree).clone_for_update()

	make::use_tree(path, use_tree_list, alias, add_star)
	make::use_tree(path, use_tree_list, alias, add_star).clone_for_update()

feat: convert unmerge_use to SyntaxFactory SyntaxEditor abstraction #18565

feat: convert unmerge_use to SyntaxFactory SyntaxEditor abstraction #18565

Uh oh!

Conversation

davidkurilla commented Nov 26, 2024

Uh oh!

darichey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darichey Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Dec 5, 2024

Uh oh!

darichey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veykril commented May 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

darichey Dec 4, 2024 •

edited

Loading