Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Append trailing text as atoms if we encounter ERROR nodes #267

Open
Wilfred opened this issue Apr 24, 2022 · 3 comments
Open

Append trailing text as atoms if we encounter ERROR nodes #267

Wilfred opened this issue Apr 24, 2022 · 3 comments

Comments

@Wilfred
Copy link
Owner

Wilfred commented Apr 24, 2022

test:
 foo: 'a'
test:
 foo: 'b'
  fail: true

Spotted in #266. The YAML parse tree does not include fail: true.

@lorepozo
Copy link

I stumbled into this issue because of a bug in the Makefile tree sitter which led to "No syntactic changes".

As a diff tool, difftastic should be made more resilient to errors in a particular language's parser, so that a bad parser won't break the tool. Unfortunately, because of this lack of resiliency and likelihood of bugs in a language's parser, I've decided to stop using difftastic. I need to trust my diff tool.

Here's a unit test of a simplified repro (written so it passes, depending on how this error handling is implemented the behavior should be different):

foo: 'a'
  fail: true
/// src/parse/tree_sitter_parser.rs
#[test]
fn test_parse_error_node() {
    let arena = Arena::new();
    let config = from_language(guess::Language::Yaml);
    let res = parse(&arena, "foo: 'a'\n  fail: true", &config);

    if let [Syntax::List { children, .. }] = res[..] {
        let atoms_content: Vec<&str> = children
            .iter()
            .map(|c| match c {
                Syntax::Atom { content, .. } => content.as_str(),
                _ => panic!("Expected Atom"),
            })
            .collect();
        let expected: Vec<&str> = vec!["foo", ":", "'a'"];
        assert_eq!(atoms_content, expected)
    } else {
        panic!("Expect one List node")
    }
}

Also after some debugging, I found that the error node here is at the root which gets skipped because of this behavior in parse:

// The tree always has a single root, whereas we want nodes for
// each top level syntax item.
cursor.goto_first_child();

Here's the curser.node() before and after that cursor traversal:

# before
{Node ERROR (0, 0) - (0, 8)}
# after
{Node block_mapping_pair (0, 0) - (0, 8)}

@Wilfred
Copy link
Owner Author

Wilfred commented Sep 19, 2022

FWIW the Makefile issue is primarily #350, although I agree better error handling would help the Makefile case too.

@Wilfred
Copy link
Owner Author

Wilfred commented Sep 19, 2022

It might also make sense to prefer text parsing if we encounter too many ERROR nodes. This would help for helm files (which are apparently Go syntax templates).

Wilfred added a commit that referenced this issue Sep 19, 2022
Wilfred pushed a commit that referenced this issue Mar 16, 2023
* Align build/test/node with tree-sitter-javascript

* Fix file count extension.
Wilfred pushed a commit that referenced this issue Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants