Serenity: LibJS: Lexer reports incorrect column for tokens

[FalseHonesty]

When lexing a piece of JS source for the console widget, I loop over the tokens to add styling information. However, it seems that the column numbers reported by these tokens are incorrect. I have to do some special math to make them usable, namely subtracting by 2, but I'm not sure why. Even after that subtraction, the output is sometimes still wrong, such as the case of let x = 10. Perhaps this is an issue in the console output code, but I'm not sure what it is doing wrong.

@linusg I believe you are the person to tag for LibJS :)

See this code from #2375.

[linusg]

@linusg I believe you are the person to tag for LibJS :)

don't have to, but I appreciate it :)

I'll have a look.

[linusg]

Can confirm, there's something weird going on:

#include <AK/StringBuilder.h>
#include <LibJS/Lexer.h>

int main()
{
    String source = "var foo = bar;\nfunction foo() {\n    return bar;\n}";
    JS::Lexer lexer(source);
    for (auto token = lexer.next(); token.type() != JS::TokenType::Eof; token = lexer.next()) {
        StringBuilder builder;
        for (size_t i = 0; i < max(token.line_column(), (size_t)1) - 1; ++i)
            builder.append(' ');
        dbg() << source.split('\n')[token.line_number() - 1];
        dbg() << builder.build() << "^";
        dbg() << "name = JS::TokenType::" << token.name();
        dbg() << "value = \"" << token.value() << "\"";
        dbg() << "trivia = \"" << token.trivia() << "\"";
        dbg() << "line_number = " << token.line_number();
        dbg() << "line_column = " << token.line_column();
        dbg() << "---------------------";
    }
}

js-tokenizer-test(25): var foo = bar;
js-tokenizer-test(25):  ^
js-tokenizer-test(25): name = JS::TokenType::Var
js-tokenizer-test(25): value = "var"
js-tokenizer-test(25): trivia = ""
js-tokenizer-test(25): line_number = 1
js-tokenizer-test(25): line_column = 2
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): var foo = bar;
js-tokenizer-test(25):      ^
js-tokenizer-test(25): name = JS::TokenType::Identifier
js-tokenizer-test(25): value = "foo"
js-tokenizer-test(25): trivia = " "
js-tokenizer-test(25): line_number = 1
js-tokenizer-test(25): line_column = 6
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): var foo = bar;
js-tokenizer-test(25):          ^
js-tokenizer-test(25): name = JS::TokenType::Equals
js-tokenizer-test(25): value = "="
js-tokenizer-test(25): trivia = " "
js-tokenizer-test(25): line_number = 1
js-tokenizer-test(25): line_column = 10
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): var foo = bar;
js-tokenizer-test(25):            ^
js-tokenizer-test(25): name = JS::TokenType::Identifier
js-tokenizer-test(25): value = "bar"
js-tokenizer-test(25): trivia = " "
js-tokenizer-test(25): line_number = 1
js-tokenizer-test(25): line_column = 12
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): var foo = bar;
js-tokenizer-test(25):               ^
js-tokenizer-test(25): name = JS::TokenType::Semicolon
js-tokenizer-test(25): value = ";"
js-tokenizer-test(25): trivia = ""
js-tokenizer-test(25): line_number = 1
js-tokenizer-test(25): line_column = 15
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): function foo() {
js-tokenizer-test(25): ^
js-tokenizer-test(25): name = JS::TokenType::Function
js-tokenizer-test(25): value = "function"
js-tokenizer-test(25): trivia = "
"
js-tokenizer-test(25): line_number = 2
js-tokenizer-test(25): line_column = 1
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): function foo() {
js-tokenizer-test(25):          ^
js-tokenizer-test(25): name = JS::TokenType::Identifier
js-tokenizer-test(25): value = "foo"
js-tokenizer-test(25): trivia = " "
js-tokenizer-test(25): line_number = 2
js-tokenizer-test(25): line_column = 10
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): function foo() {
js-tokenizer-test(25):             ^
js-tokenizer-test(25): name = JS::TokenType::ParenOpen
js-tokenizer-test(25): value = "("
js-tokenizer-test(25): trivia = ""
js-tokenizer-test(25): line_number = 2
js-tokenizer-test(25): line_column = 13
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): function foo() {
js-tokenizer-test(25):              ^
js-tokenizer-test(25): name = JS::TokenType::ParenClose
js-tokenizer-test(25): value = ")"
js-tokenizer-test(25): trivia = ""
js-tokenizer-test(25): line_number = 2
js-tokenizer-test(25): line_column = 14
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): function foo() {
js-tokenizer-test(25):                ^
js-tokenizer-test(25): name = JS::TokenType::CurlyOpen
js-tokenizer-test(25): value = "{"
js-tokenizer-test(25): trivia = " "
js-tokenizer-test(25): line_number = 2
js-tokenizer-test(25): line_column = 16
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25):     return bar;
js-tokenizer-test(25):     ^
js-tokenizer-test(25): name = JS::TokenType::Return
js-tokenizer-test(25): value = "return"
js-tokenizer-test(25): trivia = "
    "
js-tokenizer-test(25): line_number = 3
js-tokenizer-test(25): line_column = 5
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25):     return bar;
js-tokenizer-test(25):            ^
js-tokenizer-test(25): name = JS::TokenType::Identifier
js-tokenizer-test(25): value = "bar"
js-tokenizer-test(25): trivia = " "
js-tokenizer-test(25): line_number = 3
js-tokenizer-test(25): line_column = 12
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25):     return bar;
js-tokenizer-test(25):               ^
js-tokenizer-test(25): name = JS::TokenType::Semicolon
js-tokenizer-test(25): value = ";"
js-tokenizer-test(25): trivia = ""
js-tokenizer-test(25): line_number = 3
js-tokenizer-test(25): line_column = 15
js-tokenizer-test(25): ---------------------
js-tokenizer-test(25): }
js-tokenizer-test(25): ^
js-tokenizer-test(25): name = JS::TokenType::CurlyClose
js-tokenizer-test(25): value = "}"
js-tokenizer-test(25): trivia = "
"
js-tokenizer-test(25): line_number = 4
js-tokenizer-test(25): line_column = 0
js-tokenizer-test(25): ---------------------

[alimpfard]

Could this be related to
https://github.com/SerenityOS/serenity/blob/07af2e6b2cf2dd0f6bd94e8918f5a1894d83633b/Userland/js.cpp#L522

I don't quite remember why that happened (or even if that FIXME is relevant anymore), but the outcomes seem similar

[FalseHonesty]

Could this be related to

https://github.com/SerenityOS/serenity/blob/07af2e6b2cf2dd0f6bd94e8918f5a1894d83633b/Userland/js.cpp#L522

I don't quite remember why that happened (or even if that FIXME is relevant anymore), but the outcomes seem similar

I absolutely think it is related. The math to subtract 2 from the token's position was working for everything except for the last token, which wouldn't always be 2 more than it should in certain cases. By adding a fake space at the end, that would probably resolve the remaining problems. The subtraction by 2 would still be necessary for some reason though.

[alimpfard]

Why does this start at 1?
https://github.com/SerenityOS/serenity/blob/07af2e6b2cf2dd0f6bd94e8918f5a1894d83633b/Libraries/LibJS/Lexer.h#L62

[alimpfard]

The math to subtract 2 from the token's position was working for everything except for the last token

I see. that makes sense

[alimpfard]

:thinking:

"where we are in the line" plus "where the end of the token is" minus "where the start of the token is"

...how has that never blown up?

[predmond]

The problem is that the constructor calls consume which advance m_line_column to 2 and m_position to 1.

If you set m_line_column to 0 before the consume() in the constructor it seems to work correctly.

(notice in the test program only the tokens in the first line are off)

[alimpfard]

"where we are in the line" minus "where the end of the token is" plus "where the start of the token is"

as-is with this backwards thing?
Either old me was an idiot, or current me is. at least one of those is very true.

[linusg]

(notice in the test program only the tokens in the first line are off)

Not true, last line has line_column = 0 for the CurlyClose token, that should be 1.

[predmond]

If you set m_line_column to 0 before the consume() in the constructor it seems to work correctly.

hmm.. if you do this then everything looks right but the last token:

}
^
name = JS::TokenType::CurlyClose
value = "}"
trivia = "
"
line_number = 4
line_column = 0
---------------------

why is line_column 0?

Something about this code confuses me.. after consuming the token the Tokens line_column is m_line_column + (token length). But isn't m_line_column past the token at that point? Shouldn't it be m_line_column - (token lenth) to index the start of the token?

[FalseHonesty]

(notice in the test program only the tokens in the first line are off)

Not true, last line has line_column = 0 for the CurlyClose token, that should be 1.

Why should line column be 1? Isn't it the first character of the line?

[predmond]

Something about this code confuses me.. after consuming the token the Tokens line_column is m_line_column + (token length). But isn't m_line_column past the token at that point? Shouldn't it be m_line_column - (token lenth) to index the start of the token?

I can't read code. Nevermind. It's doing that already.

[linusg]

Why should line column be 1? Isn't it the first character of the line?

Yes, line and column number are 1-based.

[FalseHonesty]

Why should line column be 1? Isn't it the first character of the line?

Yes, line and column number are 1-based.

Oh. I definitely didn't realize that, gotcha.

[predmond]

(notice in the test program only the tokens in the first line are off)

Not true, last line has line_column = 0 for the CurlyClose token, that should be 1.

I believe this is because when reading the last token it gets the wrong length,

if (m_position >= m_source.length()) {
        m_position = m_source.length() + 1;
        m_current_char = EOF;
        return;
    }

when m_position is m_source.length() it adds 1 for some reason so the length (m_position - value_start) becomes 2 which makes line_column 0.

[sunverwerth]

@predmond That might be it. I think I remember writing that code late at night in a brain fog.

[predmond]

diff --git a/Libraries/LibJS/Lexer.cpp b/Libraries/LibJS/Lexer.cpp
index dc094f4f5..f67d8ff9b 100644
--- a/Libraries/LibJS/Lexer.cpp
+++ b/Libraries/LibJS/Lexer.cpp
@@ -149,8 +149,12 @@ Lexer::Lexer(StringView source)

 void Lexer::consume()
 {
-    if (m_position >= m_source.length()) {
-        m_position = m_source.length() + 1;
+    if (m_position > m_source.length())
+        return;
+
+    if (m_position == m_source.length()) {
+        m_position++;
+        m_line_column++;
         m_current_char = EOF;
         return;
     }
diff --git a/Libraries/LibJS/Lexer.h b/Libraries/LibJS/Lexer.h
index e60c2dd65..71f22b053 100644
--- a/Libraries/LibJS/Lexer.h
+++ b/Libraries/LibJS/Lexer.h
@@ -55,11 +55,11 @@ private:
     bool match(char, char, char, char) const;

     StringView m_source;
-    size_t m_position = 0;
+    size_t m_position { 0 };
     Token m_current_token;
-    int m_current_char = 0;
-    size_t m_line_number = 1;
-    size_t m_line_column = 1;
+    int m_current_char { 0 };
+    size_t m_line_number { 1 };
+    size_t m_line_column { 0 };

     struct TemplateState {
         bool in_expr;

Ultimately there are two issues.
1) m_line_column is initialized to 1 which leads to incorrect token line column numbers for tokens on the first line
2) after reading EOF m_line_column isn't incremented one last time

After playing whack-a-mole for a while I think the above patch finally fixes both problems.

[FalseHonesty]

Fixed by #2401, thanks @predmond :)