feat: convert unmerge_use to SyntaxFactory SyntaxEditor abstraction #18565
Conversation
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
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.
| use_.visibility(), | ||
| make::use_tree(path, tree.use_tree_list(), tree.rename(), tree.star_token().is_some()), | ||
| make.use_tree(path, tree.use_tree_list(), tree.rename(), tree.star_token().is_some()), | ||
| ) | ||
| .clone_for_update(); | ||
|
|
||
| tree.remove(); |
There was a problem hiding this comment.
This should use editor.delete(...)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We want to pass a SyntaxNode which implements Element. In this case, the one from tree.
| ) | ||
| .clone_for_update(); | ||
|
|
||
| tree.remove(); | ||
| 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()); |
There was a problem hiding this comment.
This should use editor.replace(...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think you're probably missing a clone_for_update. If you push your changes I can try to help debug.
|
☔ The latest upstream changes (presumably #18483) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributes to issue #18285
I think I fully converted this but tests are failing with the following output: