Don't replace entire module and file nodes when inserting imports

[Veykril]

This change minifies the resulting diff of import insertions by inserting or replacing the produced use tree directly through an action return value instead replacing the entire container node. This action has to be applied by the caller now. This unfortunately pulls the AssistBuilder into scope of insert_use back again but I tried to at least keep it away from the insert_use fn itself.

I'm open to more/better ideas regarding this :)

Fixes #6196

[matklad]

Would returning SyntaxRewriter work here? See AssistBuilder::rewrite. As usual, this is "how to compose tree edits?" open probelm.

[Veykril]

SyntaxRewriter doesn't have any insertion logic yet from what I've seen which was why I didn't consider it I think. On a second thought it would probably make a lot more sense to have this use a SyntaxRewriter instead though.

[Veykril]

I don't think a SyntaxRewriter can be used here as it can't model insertions(at least I don't see how), as it is meant for rewriting existing nodes only, so just replacements and deletions.

[matklad]

I don't think a SyntaxRewriter can be used here as it can't model insertions(at least I don't see how)

It can't model them now, but it makes sense to extend it to allow this. Implementation wise, rewrite_children function can look into some auxilary map to decide if it needs to appaend element to new_children

That is, we can add

struct SyntaxRewriter {
  ...
  insertions: FxHashMap<InsetPost, Vec<SyntaxElement>>,
}

enum InsertPos { FirstChildOf(SyntaxNode), After(SyntaxElement) }

and that might do the trick?

[Veykril]

Oh alright, I wasn't sure whether I should touch that part or not, but in that case I'll take another look at SyntaxRewriter

[matklad]

Nothing in rust-analyzer is completely complete :)

…

On Tue, 20 Oct 2020 at 20:23, Lukas Wirth ***@***.***> wrote: Oh alright, I wasn't sure whether I should touch that part or not, but in that case I'll take another look at SyntaxRewriter — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M6EFCVGVOT3USBCCG3SLXIQ3ANCNFSM4SWK4LAA> .

[Veykril]

I just realized the problem with using SyntaxRewriter for this after all, as the AssistBuilder::rewrite function makes use of algo::diff this will again introduce the same problem with the diff becoming the entire input node on insertion.

Unless I also do changes to TreeDiff to track insertions as well, I guess I can look into that.

Edit: Blocked on #6310/#6365

[Veykril]

Alright, updated this PR to my recent changes and tested the server locally. It now does not move the cursor in imports anymore 🎉

I had to rewrite extract_struct_from_enum_variant.rs quite a bit to basically use SyntaxRewriters everywhere to prevent overlapping edits to happen.

[matklad]

Perfect, thanks! Rewrite of extract_struct_from_enum_variant is very welcome! (btw, since the code is in your ICache, note that extending it to work with record variants would be nice: #4468 (comment) ;) )

bors r+

[bors]

Build succeeded:

• Rust (macos-latest)
• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript (ubuntu-latest)
• TypeScript (windows-latest)

[SomeoneToIgnore]

Do you think it's possible to somehow insert the first import without rewriting the whole file?
In terms of the unit test of the algo module, I want to have:

#[test]
fn first_use_added() {
    check_diff(
        r#"fn main() {
    stdi<|>
}"#,
        r#"use foo::bar;

fn main() {
    stdi<|>
}"#,
        expect![[r#"
            insertions:

            Line 0: Node(SOURCE_FILE@0..25)
            -> use foo::bar;
            -> "\n\n"

            replacements:



            deletions:


        "#]],
    );
}

and instead I get the

insertions:

Line 0: Node(FN@0..25)
-> "\n\n"
-> fn main() {
    stdi<|>
}

replacements:

Line 0: Token(FN_KW@0..2 "fn") -> use
Line 1: Token(IDENT@3..7 "main") -> foo::bar
Line 1: Node(PARAM_LIST@7..9) -> ;

deletions:

Line 1: " "
Line 1: {
    stdi<|>
}

diff.

[Veykril]

I'm not sure, maybe with some special handling. The problem is what an insertion really means for diffing here, currently it just checks if the node in the original tree still exists in the new tree, then it marks everything between as insertions, otherwise it marks stuff as replacements. Prior to my changes this just replaced the whole parent node, so when fiddling with imports you'd just get a SOURCE_FILE replacement everytime.

Edit: Hm actually this should already mark that as in insertion since the fn main node is still the same isnt it? thats weird

[SomeoneToIgnore]

It does mark it so, but the condition last_lhs is None for the first comparison (old fn block vs new use block) and that's why no actual insertion happens.
I've tried to brute force things by getting a lhs.parent() (which is a source file in this case) instead of last_lhs, but it did not work, the insertion places the new use statement afterwards the old fn node despite that.

I'm not well familiar with syntax trees and the proper ways to make things work here, but interested to find some solution to this so appreciate any pointers or fixes 🙂

[Veykril]

Yes I just noticed that again as well, insertions are anchored after elements, so if the first element is whats being inserted we have a problem here. Maybe we just gotta keep track of elements that are to be inserted directly into their parent as the first child in a separate Field in TreeDiff, as in an extra map similar to the insertions one and then insert into the TextBuilder accordingly. Doesn't sound like the best solution but diffing is quite hard as I noticed 😅 And we need to try real hard to not throw away the cursor positions too much.

I could give it a try sometime later today unless you wanna experiment.

[SomeoneToIgnore]

You're definitely better than me in this, so go ahead if you want to.
Thanks for looking into this.