New parser fixes tests failing against #281

[Razican]

I have ported #261 to the new parser (#281) (this should close #261). And I'm working on fixing the tests that don't pass. I'm getting this error:

ParsingError: Unexpected Token 'empty' at line 1, col 13

When parsing:

        var empty = [ ];
        var one = ["a"];
        var many = ["a", "b", "c"];
        var duplicates = ["a", "b", "c", "a", "b"];

So, I'll try to fix that variable creation tomorrow if possible. Feel free to add suggestions.

[jasonwilliams]

Let me know if there’s anyway I can help, not picked up anything yet.
May have some time later or tomorrow

[Razican]

I fixed this particular error. It turns out that it was reading new lines and not skipping them, so it didn't understand that this was a variable declaration. But I found a new one:

ParsingError: Expected one of ';' or 'line terminator', got '=' in routine 'variable declaration' at line 2, col 19

[jasonwilliams]

Interesting, I’m already getting the feeling this parser is easier to reason with than before. Hope I haven’t spoke too soon.

Update this thread if you sort it, if not I’ll take a look

[Razican]

Cool, this seems to fix this one too. I got a new one with this code:

function comp(a) {
    return a == "a";
}
var many = ["a", "b", "c"];

It gives:

ParsingError: Expected token '[', got '{' in routine 'function declaration' at line 2, col 26

You can try it by running cargo test builtins::array::tests::find. I will try to fix it during the day.

I'm also getting a panic when running another test, so I'm guessing there is something else wrong here:

thread 'builtins::array::tests::find_index' panicked at 'failed to convert to object: Undefined', boa/src/exec/mod.rs:128:35

I think we are messing things too much with so much "peek" or "next_if" and so on. Maybe this could be simplified? It seems to give a lot of of-by-one issues, among others. Maybe one options is to implement or to at least mimic the Iterator traits from the standard library.

For many of our needs, we could use the Iterator trait and the Peekable structure, or other implementations in the std::iter module.

[jasonwilliams]

For many of our needs, we could use the Iterator trait and the Peekable structure, or other implementations in the std::iter module.

I do want to do this, and was attempting to do this previously but ran into some issues, so i put it on the back burner for now (i think because some methods want to go backwards etc).
I put it outside of the scope of the New Parser because migrating to this and changing to an iterator at the same time caused confusion.

You can see that i made a start by abstracting over get_token(pos) and instead using methods like get_next_token, peek, next_if etc.

This should make it easy to migrate to an iterator in future

[jasonwilliams]

I think we are messing things too much with so much "peek" or "next_if" and so on. Maybe this could be simplified? It seems to give a lot of of-by-one issues, among others.

Yeah we need to be on the same mental model for this.
So in the old parser getting next was incrementing in the array and returning a value.
In this, getting next pops-off whatever is first in the array, so the other methods need to match up.

[HalidOdat]

Cool, this seems to fix this one too. I got a new one with this code:

function comp(a) {
    return a == "a";
}
var many = ["a", "b", "c"];

It gives:

ParsingError: Expected token '[', got '{' in routine 'function declaration' at line 2, col 26

There is a bug when parsing function declarations in function: read_function_declaration. Before parsing function body.

self.expect(
    TokenKind::Punctuator(Punctuator::OpenBracket),
        "expected '{'",
)?;

It should be:

self.expect(
    TokenKind::Punctuator(Punctuator::OpenBlock),
        "expected '{'",
)?;

[Razican]

I added the fix by @HalidOdat and now that seems to work. Still, it seems that it gets stuck in some kind of infinite loop. So I will put this as ready for review, but we need to check that.

[jasonwilliams]

Looks good to me.
Which bit of JS you running to get the inifinte loop?

Edit: im guessing the

function comp(a) {
  return a == "a";
}
var many = ["a", "b", "c"];

plus some tests.

[HalidOdat]

Its from the Array.prototype.every test.
More specifically its this code:

var empty = [];

var array = [11, 23, 45];

function callback(element) {
    return element > 10;
}

function callback2(element) {
    return element < 10;
}

var appendArray = [1, 2, 3, 4];

function appendingCallback(elem, index, arr) {
    arr.push('new');
    return elem !== "new";
}

I'm not sure exactly what part of this code causes the infinite loop.

[jasonwilliams]

Would be nice if there's some way of logging each state it ends up in without affecting production builds at all.
Like a verbose logger, similar to assert_debug() which i believe doesn't get compiled in release builds.

[HalidOdat]

This code also goes in an infinite loop.

var empty = [ ];
var one = ["a"];
var many = ["a", "b", "c"];
var duplicates = ["a", "b", "c", "a", "b"];

I am confused were the origin of the bug is. 😕

[jasonwilliams]

For var many = ["a", "b", "c"];
it seems to be skipping over the [ for the array and trying to assign a directly to many

Call Stack

read_assignment_expression 
read_initializer 
read_variable_declaration 
read_variable_declaration_list 
read_variable_statement 
read_statement 
read_statement_list_item 
read_statements 
read_statement_list 
parse_all 
parser_expr 
forward_val 
main 

Im stepping through using lldb

Edit:
Its because we changed how get_next_token works so all the peeks() are now wrong.
Peek will skip over a token, so this is another off-by-1 error.

It might be quicker to just change the implementation of peek to get the current token rather than the next one.
Changing peek by removing the 1 fixes this

[jasonwilliams]

If we merged this back into the main parser branch i can fix peek and that should fix quite a few errors

[Razican]

If we merged this back into the main parser branch i can fix peek and that should fix quite a few errors

Sure, let's merge this and keep working on the other branch :)

[HalidOdat]

The bug that causes the infinite looping is in the function variable_declaration_continuation.
The bug was that it did not move forward by one when a new line-terminator, semicolon, or comma was found and it checked the next token not the current one.

fn variable_declaration_continuation(&mut self) -> Result<bool, ParseError> {
    let mut newline_found = false;

    loop {
        match self.get_current_token() { // <----
            Ok(tok) => match tok.kind {
                TokenKind::LineTerminator => newline_found = true,
                TokenKind::Punctuator(Punctuator::Semicolon) => {
                    return Ok(false);
                }
                TokenKind::Punctuator(Punctuator::Comma) => {
                    return Ok(true);
                }
                _ if newline_found => return Ok(false),
                _ => break,
            },
            Err(_) => return Ok(false),
        }
        self.pos += 1; // <-----
    }

    Err(ParseError::Expected(
        vec![
            TokenKind::Punctuator(Punctuator::Semicolon),
            TokenKind::LineTerminator,
        ],
        self.get_current_token()?,
        "variable declaration",
    ))
}

Now it does not cause an infinite loop.

test result: FAILED. 63 passed; 63 failed; 1 ignored; 0 measured; 0 filtered out

WERE HALF WAY THERE!!! 😀