Stream: t-compiler/rust-analyzer

Topic: Tree diff rewrite tests


Lukas Wirth (Oct 22 2020 at 09:12, on Zulip):

Quick question regarding how I should model the unit tests. I was thinking of something like the following. @matklad

#[cfg(test)]
mod tests {
    use expect_test::{expect, Expect};
    use text_edit::TextEdit;

    use crate::AstNode;

    #[test]
    fn test_diff() {
        check_diff(
            r"
use expect_test::{expect, Expect};

use crate::AstNode;
",
            "
use expect_test::{expect, Expect};
use text_edit::TextEdit;

use crate::AstNode;
",
            expect![[r#"
                TreeDiff {
                    replacements: {
                        Token(
                            WHITESPACE@35..37 "\n\n",
                        ): Token(
                            WHITESPACE@35..36 "\n",
                        ),
                        Token(
                            CRATE_KW@41..46 "crate",
                        ): Node(
                            NAME_REF@40..49
                              IDENT@40..49 "text_edit"
                            ,
                        ),
                        Token(
                            IDENT@48..55 "AstNode",
                        ): Token(
                            IDENT@51..59 "TextEdit",
                        ),
                        Token(
                            WHITESPACE@56..57 "\n",
                        ): Token(
                            WHITESPACE@60..62 "\n\n",
                        ),
                    },
                    deletions: [],
                    insertions: {
                        Token(
                            WHITESPACE@56..57 "\n",
                        ): [
                            Node(
                                USE@62..81
                                  USE_KW@62..65 "use"
                                  WHITESPACE@65..66 " "
                                  USE_TREE@66..80
                                    PATH@66..80
                                      PATH@66..71
                                        PATH_SEGMENT@66..71
                                          CRATE_KW@66..71 "crate"
                                      COLON2@71..73 "::"
                                      PATH_SEGMENT@73..80
                                        NAME_REF@73..80
                                          IDENT@73..80 "AstNode"
                                  SEMICOLON@80..81 ";"
                                ,
                            ),
                            Token(
                                WHITESPACE@81..82 "\n",
                            ),
                        ],
                    },
                }"#]],
        )
    }

    fn check_diff(from: &str, to: &str, expected_diff: Expect) {
        let from_node = crate::SourceFile::parse(from).tree().syntax().clone();
        let to_node = crate::SourceFile::parse(to).tree().syntax().clone();
        let diff = super::diff(&from_node, &to_node);
        expected_diff.assert_eq(&format!("{:#?}", diff));

        let mut from = from.to_owned();
        let mut text_edit = TextEdit::builder();
        diff.into_text_edit(&mut text_edit);
        text_edit.finish().apply(&mut from);
        assert_eq!(&*from, to, "diff did not turn `from` to `to`");
    }
}
Lukas Wirth (Oct 22 2020 at 09:14, on Zulip):

Though i guess this gets rather unwieldy quite fast? not sure

matklad (Oct 22 2020 at 09:31, on Zulip):

I'd use something more visual

matklad (Oct 22 2020 at 09:32, on Zulip):

Specifically, when rendering insertions and deletions, I'd use Display of nodes, rather than verbose debug

matklad (Oct 22 2020 at 09:33, on Zulip):

I imgine something like this would work

replacements:

STRUCT_DEF@10..20 ->
"enum Foo { }"

...
matklad (Oct 22 2020 at 09:33, on Zulip):

Ie, we use {:?} for anchor and {} for replacement

Lukas Wirth (Oct 22 2020 at 10:10, on Zulip):

So something like this?

#[cfg(test)]
mod tests {
    use expect_test::{expect, Expect};
    use itertools::Itertools;
    use text_edit::TextEdit;

    use crate::AstNode;

    #[test]
    fn insert_use() {
        check_diff(
            r"
use expect_test::{expect, Expect};

use crate::AstNode;
",
            "
use expect_test::{expect, Expect};
use text_edit::TextEdit;

use crate::AstNode;
",
            expect![[r#"
                insetions:

                Token(WHITESPACE@56..57 "\n") ->
                "use crate::AstNode;"
                "
                "

                replacements:

                Token(WHITESPACE@35..37 "\n\n") ->
                "
                "
                Token(CRATE_KW@41..46 "crate") ->
                "text_edit"
                Token(IDENT@48..55 "AstNode") ->
                "TextEdit"
                Token(WHITESPACE@56..57 "\n") ->
                "

                "

                deletions:


            "#]],
        )
    }

    #[test]
    fn remove_use() {
        check_diff(
            r"
use expect_test::{expect, Expect};
use text_edit::TextEdit;

use crate::AstNode;
",
            "
use expect_test::{expect, Expect};

use crate::AstNode;
",
            expect![[r#"
                insetions:



                replacements:

                Node(NAME_REF@40..49) ->
                "crate"
                Token(IDENT@51..59 "TextEdit") ->
                "AstNode"
                Token(WHITESPACE@60..62 "\n\n") ->
                "
                "
                Token(WHITESPACE@35..36 "\n") ->
                "

                "

                deletions:

                "use crate::AstNode;"
                "
                "
            "#]],
        )
    }

    #[test]
    fn merge_use() {
        check_diff(
            r"
use std::{
    fmt,
    hash::BuildHasherDefault,
    ops::{self, RangeInclusive},
};
",
            "
use std::fmt;
use std::hash::BuildHasherDefault;
use std::ops::{self, RangeInclusive};
",
            expect![[r#"
                insetions:

                Node(PATH_SEGMENT@5..8) ->
                "::"
                "fmt"
                Token(WHITESPACE@86..99 "            \n") ->
                "use std::hash::BuildHasherDefault;"
                "
                "
                "use std::ops::{self, RangeInclusive};"
                "
                "

                replacements:

                Token(WHITESPACE@86..99 "            \n") ->
                "
                "
                Token(IDENT@5..8 "std") ->
                "std"

                deletions:

                "::"
                "{
                    fmt,
                    hash::BuildHasherDefault,
                    ops::{self, RangeInclusive},
                }"
            "#]],
        )
    }

    #[test]
    fn dd() {
        check_diff(
            r#"
fn main() {
    if<|> let Ok(x) = Err(92) {
        foo(x);
    }
}
            "#,
            r#"
fn main() {
    let x = match Err(92) {
        Ok(it) => it,
        _ => return,
    };
    foo(x);
}
            "#,
            expect![[r#"
                insetions:

                Node(IF_EXPR@17..22) ->
                " "
                "x"
                " "
                "="
                " "
                "match Err(92) {
                        Ok(it) => it,
                        _ => return,
                    }"
                ";"

                replacements:

                Token(WHITESPACE@42..43 " ") ->
                "
                "
                Node(BLOCK_EXPR@43..66) ->
                "}"
                Token(LET_KW@23..26 "let") ->
                "foo(x)"
                Token(WHITESPACE@26..27 " ") ->
                ";"
                Node(IF_EXPR@17..22) ->
                "let"
                Token(WHITESPACE@22..23 " ") ->
                "
                    "

                deletions:

                "Ok(x)"
                " "
                "="
                " "
                "Err(92)"
                "
                "
                "}"
            "#]],
        )
    }

    fn check_diff(from: &str, to: &str, expected_diff: Expect) {
        let from_node = crate::SourceFile::parse(from).tree().syntax().clone();
        let to_node = crate::SourceFile::parse(to).tree().syntax().clone();
        let diff = super::diff(&from_node, &to_node);

        let insertions = diff.insertions.iter().format_with("\n", |(k, v), f| {
            f(&format!(
                "{:?} ->\n{}",
                k,
                v.iter().format_with("\n", |v, f| f(&format!("\"{}\"", v)))
            ))
        });

        let replacements = diff
            .replacements
            .iter()
            .format_with("\n", |(k, v), f| f(&format!("{:?} ->\n\"{}\"", k, v)));

        let deletions = diff.deletions.iter().format_with("\n", |v, f| f(&format!("\"{}\"", v)));

        let actual = format!(
            "insetions:\n\n{}\n\nreplacements:\n\n{}\n\ndeletions:\n\n{}\n",
            insertions, replacements, deletions
        );
        expected_diff.assert_eq(&actual);

        let mut from = from.to_owned();
        let mut text_edit = TextEdit::builder();
        diff.into_text_edit(&mut text_edit);
        text_edit.finish().apply(&mut from);
        assert_eq!(&*from, to, "diff did not turn `from` to `to`");
    }
}
matklad (Oct 22 2020 at 10:27, on Zulip):

Yeah, that's more readable already

matklad (Oct 22 2020 at 10:28, on Zulip):

We probably can get rid of some internal quotes though

matklad (Oct 22 2020 at 10:28, on Zulip):

(not sure why I put them in my example)

matklad (Oct 22 2020 at 10:28, on Zulip):

Generally, optimize the thing to be as human-readable as possible, and expect would make sure that writing it out is not the pain

matklad (Oct 22 2020 at 10:29, on Zulip):

Well, it would help to print the line number where the op is happening as well. I didn't expect most of the anchors to be whitespace :D

Lukas Wirth (Oct 22 2020 at 10:30, on Zulip):

Ye a lot of whitespace is being replaced and inserted :big_smile:

matklad (Oct 22 2020 at 10:31, on Zulip):

For whitespace, I think we can special case this and use {:?} of the text of the node

matklad (Oct 22 2020 at 10:31, on Zulip):

so that you get "\n" rather than " "

Lukas Wirth (Oct 22 2020 at 10:31, on Zulip):

Ye that seems like a good idea

Lukas Wirth (Oct 22 2020 at 10:31, on Zulip):

regarding line numbers, how would i get those?

matklad (Oct 22 2020 at 10:32, on Zulip):

I am afraind just manually: text[..range.start()].lines().count()

matklad (Oct 22 2020 at 10:32, on Zulip):

add -/+1 to taste

Lukas Wirth (Oct 22 2020 at 11:13, on Zulip):

Alright, looks good to me I'd say. Question is what makes good tests for this this :sweat_smile:

    #[test]
    fn insert_use() {
        check_diff(
            r"
use expect_test::{expect, Expect};

use crate::AstNode;
",
            "
use expect_test::{expect, Expect};
use text_edit::TextEdit;

use crate::AstNode;
",
            expect![[r#"
                insertions:

                Line 4: Token(WHITESPACE@56..57 "\n")
                -> use crate::AstNode;
                -> "\n"

                replacements:

                Line 4: Token(IDENT@48..55 "AstNode") -> TextEdit
                Line 4: Token(WHITESPACE@56..57 "\n") -> "\n\n"
                Line 2: Token(WHITESPACE@35..37 "\n\n") -> "\n"
                Line 4: Token(CRATE_KW@41..46 "crate") -> text_edit

                deletions:


            "#]],
        )
    }

    #[test]
    fn remove_use() {
        check_diff(
            r"
use expect_test::{expect, Expect};
use text_edit::TextEdit;

use crate::AstNode;
",
            "
use expect_test::{expect, Expect};

use crate::AstNode;
",
            expect![[r#"
                insertions:



                replacements:

                Line 3: Token(IDENT@51..59 "TextEdit") -> AstNode
                Line 3: Node(NAME_REF@40..49) -> crate
                Line 2: Token(WHITESPACE@35..36 "\n") -> "\n\n"
                Line 3: Token(WHITESPACE@60..62 "\n\n") -> "\n"

                deletions:

                Line 4: use crate::AstNode;
                Line 5: "\n"
            "#]],
        )
    }

    #[test]
    fn merge_use() {
        check_diff(
            r"
use std::{
    fmt,
    hash::BuildHasherDefault,
    ops::{self, RangeInclusive},
};
",
            "
use std::fmt;
use std::hash::BuildHasherDefault;
use std::ops::{self, RangeInclusive};
",
            expect![[r#"
                insertions:

                Line 2: Node(PATH_SEGMENT@5..8)
                -> ::
                -> fmt
                Line 6: Token(WHITESPACE@86..87 "\n")
                -> use std::hash::BuildHasherDefault;
                -> "\n"
                -> use std::ops::{self, RangeInclusive};
                -> "\n"

                replacements:

                Line 2: Token(IDENT@5..8 "std") -> std

                deletions:

                Line 2: ::
                Line 2: {
                    fmt,
                    hash::BuildHasherDefault,
                    ops::{self, RangeInclusive},
                }
            "#]],
        )
    }

    #[test]
    fn early_return_assist() {
        check_diff(
            r#"
fn main() {
    if let Ok(x) = Err(92) {
        foo(x);
    }
}
            "#,
            r#"
fn main() {
    let x = match Err(92) {
        Ok(it) => it,
        _ => return,
    };
    foo(x);
}
            "#,
            expect![[r#"
                insertions:

                Line 3: Node(BLOCK_EXPR@40..63)
                -> " "
                -> match Err(92) {
                        Ok(it) => it,
                        _ => return,
                    }
                -> ;
                Line 5: Token(R_CURLY@64..65 "}")
                -> "\n"
                -> }

                replacements:

                Line 5: Token(R_CURLY@64..65 "}") -> foo(x);
                Line 3: Token(LET_KW@20..23 "let") -> x
                Line 3: Token(IF_KW@17..19 "if") -> let
                Line 3: Node(BLOCK_EXPR@40..63) -> =
                Line 5: Token(WHITESPACE@63..64 "\n") -> "\n    "

                deletions:

                Line 3: " "
                Line 3: Ok(x)
                Line 3: " "
                Line 3: =
                Line 3: " "
                Line 3: Err(92)
            "#]],
        )
    }
matklad (Oct 22 2020 at 11:14, on Zulip):

Question is what makes good tests for this this

Placing mark::hit! into every interesting if and writing minimal suite that hits them all/

matklad (Oct 22 2020 at 11:15, on Zulip):

looks nice!

Lukas Wirth (Oct 22 2020 at 12:02, on Zulip):

Alright, pushed the test stuff :thumbs_up:

Lukas Wirth (Oct 22 2020 at 12:04, on Zulip):

Interesting, import resolution for the mark macros fails on CI, as well as in RA but not when I build myself

Lukas Wirth (Oct 22 2020 at 12:05, on Zulip):

Oh I see the problem

Last update: Jul 24 2021 at 21:00UTC