Rowan upgrade to 0.16.0 breaks prev_sibling behaviour

[MalteJanz]

Discovered by MalteJanz/ludtwig#122

The failed test pipeline contains a bunch of issues related to the rowan upgrade that might need further investigation. But I think the easiest failing test is the following:

(can be found here)

    #[test]
    fn it_should_not_panic_on_prev_sibling_call() {
        let parse = parse("<div>a<hr/></div>");
        let root = SyntaxNode::new_root(parse.green_node);
        // println!("{}", debug_tree(&root));

        let prev = root.prev_sibling();
        // println!("prev sibling: {:?}", prev);
        assert!(prev.is_none());

        let child: HtmlTag = support::child(&root).unwrap();
        let prev = child.syntax().prev_sibling();
        // println!("{:?} prev sibling: {:?}", child, prev);
        assert!(prev.is_none());

        let child: HtmlTag = support::child(child.body().unwrap().syntax()).unwrap();
        let prev = child.syntax().prev_sibling();
        // println!("{:?} prev sibling: {:?}", child, prev);
        assert!(prev.is_some());
    }

Btw: I wrote this test while fixing my previous rowan contribution, see #143 . It basically came down to a off-by-one error which might regressed with the latest version, see initial issue: #139

Test output:

---- tests::it_should_not_panic_on_prev_sibling_call stdout ----
thread 'tests::it_should_not_panic_on_prev_sibling_call' panicked at crates/ludtwig-parser/src/lib.rs:152:9:
assertion failed: prev.is_some()

Which points to the last assertion and last line of the test above. Seems like the sibling somehow got lost 🤔

Test explaination

The parsing input:

<div>a<hr/></div>

produces the following tree:

ROOT@0..17
  HTML_TAG@0..17
    HTML_STARTING_TAG@0..5
      TK_LESS_THAN@0..1 "<"
      TK_WORD@1..4 "div"
      HTML_ATTRIBUTE_LIST@4..4
      TK_GREATER_THAN@4..5 ">"
    BODY@5..11
      HTML_TEXT@5..6
        TK_WORD@5..6 "a"
      HTML_TAG@6..11
        HTML_STARTING_TAG@6..11
          TK_LESS_THAN@6..7 "<"
          TK_WORD@7..9 "hr"
          HTML_ATTRIBUTE_LIST@9..9
          TK_SLASH_GREATER_THAN@9..11 "/>"
    HTML_ENDING_TAG@11..17
      TK_LESS_THAN_SLASH@11..13 "</"
      TK_WORD@13..16 "div"
      TK_GREATER_THAN@16..17 ">"

Note: remember the HTML_TEXT node with the "a" word token next to the HTML_TAG node ("hr" tag)

The first declaration of child ends up as the following tree (the outer div):

HTML_TAG@0..17
  HTML_STARTING_TAG@0..5
    TK_LESS_THAN@0..1 "<"
    TK_WORD@1..4 "div"
    HTML_ATTRIBUTE_LIST@4..4
    TK_GREATER_THAN@4..5 ">"
  BODY@5..11
    HTML_TEXT@5..6
      TK_WORD@5..6 "a"
    HTML_TAG@6..11
      HTML_STARTING_TAG@6..11
        TK_LESS_THAN@6..7 "<"
        TK_WORD@7..9 "hr"
        HTML_ATTRIBUTE_LIST@9..9
        TK_SLASH_GREATER_THAN@9..11 "/>"
  HTML_ENDING_TAG@11..17
    TK_LESS_THAN_SLASH@11..13 "</"
    TK_WORD@13..16 "div"
    TK_GREATER_THAN@16..17 ">"

And doesn't have a previous sibling as expected.

Next up it get's the body of that div tag (isn't stored in a variable), which also looks like expected:

BODY@5..11
  HTML_TEXT@5..6
    TK_WORD@5..6 "a"
  HTML_TAG@6..11
    HTML_STARTING_TAG@6..11
      TK_LESS_THAN@6..7 "<"
      TK_WORD@7..9 "hr"
      HTML_ATTRIBUTE_LIST@9..9
      TK_SLASH_GREATER_THAN@9..11 "/>"

And inside that it get's the first tag which ends up in the second declared child variable:

HTML_TAG@6..11
  HTML_STARTING_TAG@6..11
    TK_LESS_THAN@6..7 "<"
    TK_WORD@7..9 "hr"
    HTML_ATTRIBUTE_LIST@9..9
    TK_SLASH_GREATER_THAN@9..11 "/>"

As expected that's the <hr/> tag. And now comes the previously working call to prev_sibling on that exact node, which surprisingly ends up with none instead of the HTML_TEXT node.

The same test and parser implementation was succeeding in rowan 0.15.16

[MalteJanz]

@milianw Might be related to your PR #171 as it slightly changed the prev_sibling implementation 🤔
https://github.com/rust-analyzer/rowan/pull/171/files#diff-d652bc172cf95622c9c34f43bd6ed9a6c27f042945f710cb9025cae7b12dd579L393

[milianw]

Hey @MalteJanz, sorry for that if it's my patch. Quite the pity that rowan doesn't have these test cases in its own repository :( I ran the tests we have in slint, and supposedly it worked for rust-analyzer too (or @Veykril?)

Have you tried reverting my patch to verify that it is to blame? If that is the case, maybe it should be reverted for now?

How can I reproduce the failure, can you write down the required steps for me to run? Then I might be able to find some time the next days to investigate.

[MalteJanz]

Hi, thanks for the quick response. I didn't investigate further yet if it is really related to your change, it was just a guess based on the commits of the new version. I also find it unfortunate that the rowan repository itself doesn't have a proper test suite 🙈

For reproducing it, feel free to:

• clone https://github.com/MalteJanz/ludtwig
• run cargo test -p ludtwig-parser (in that crate only the above mentionend test case fails)
• feel free to play around with the test in crates/ludtwig-parser/src/lib.rs
• you can also run all the tests (or more specific the tests of the other crate), there are some "linting rules" that also changed their behaviour because of the rowan upgrade, but these might be more complex as they are parsing more complex templates and checking the whole syntax tree

[milianw]

Thanks for spotting this @MalteJanz, seems like the issue lurked in #119 and my new PR should handle that