Repository: mozilla/nunjucks (js)

Issue:
Token/Node lineno and colno do not correspond to source positions
I use nunjucks to power an internal tool at my work which allows users to configure nunjucks templates as part of a process automation tool. This has worked really well, and I've built a fair amount of tooling on top of nunjucks that uses the parser API to do some cool introspection to find e.g. references to symbols in the template which we know are invalid based on the context.

One frustration I've run into as I've used more internal APIs like the parser is that the Nodes' `lineno` and `colno` values don't seem to consistently match up with the source position in the input template. This has limited my ability to highlight error positions in the template, or at least required me to use some hacky workarounds to try to pinpoint a Node's source position.

After digging into it a bit, I think I've tracked the issue down to a single line in the lexer where the `index` field is updated directly in a way that does not keep `lineno` and `colno` in sync:

https://github.com/mozilla/nunjucks/blob/b82815815db5bb6249c270d518a77510fe575903/nunjucks/src/lexer.js#L378

This causes the line and column numbers to not be advanced to take into account tokens extracted with `_extractString` (primarily variable and block start/end tokens).

Pull request description:
lexer more accurately tracks token line and column information
## Summary

Proposed change:

This PR updates the lexer to more accurately keep track of tokens' source positions. The `_extractString` function was directly manipulating the `index` field without keeping `lineno` and `colno` in sync, whereas the rest of the lexer appears to use `forward` and `forwardN` which keeps line and column positions in sync.

Closes #1188 .


## Checklist

I've completed the checklist below to ensure I didn't forget anything. This makes reviewing this PR as easy as possible for the maintainers. And it gets this change released as soon as possible.

* [x] Proposed change helps towards [*purpose of this project*](https://github.com/mozilla/nunjucks/blob/master/CONTRIBUTING.md#purpose).
* [ ] [*Documentation*](https://github.com/mozilla/nunjucks/tree/master/docs/) is added / updated to describe proposed change.
* [x] [*Tests*](https://github.com/mozilla/nunjucks/tree/master/tests) are added / updated to cover proposed change.
* [ ] [*Changelog*](https://github.com/mozilla/nunjucks/blob/master/CHANGELOG.md) has an entry for proposed change (if user-facing fix or feature).

I'd be happy to update the documentation and/or changelog as necessary with this change. I'm not sure what the policy for internal API changes like this are.

Patch:
diff --git a/nunjucks/src/lexer.js b/nunjucks/src/lexer.js
index 2e33adb..ef99227 100644
--- a/nunjucks/src/lexer.js
+++ b/nunjucks/src/lexer.js
@@ -375,7 +375,7 @@ class Tokenizer {
 
   _extractString(str) {
     if (this._matches(str)) {
-      this.index += str.length;
+      this.forwardN(str.length);
       return str;
     }
     return null;


Test patch:
diff --git a/tests/compiler.js b/tests/compiler.js
index a282ecc..8bdebe6 100644
--- a/tests/compiler.js
+++ b/tests/compiler.js
@@ -816,7 +816,7 @@
         tmpl.render({}, function(err, res) {
           expect(res).to.be(undefined);
           expect(err.toString()).to.be([
-            'Template render error: (parse-error.njk) [Line 1, Column 24]',
+            'Template render error: (parse-error.njk) [Line 1, Column 26]',
             '  unexpected token: ,',
           ].join('\n'));
           done();
diff --git a/tests/lexer.js b/tests/lexer.js
index 0c406e5..515c46e 100644
--- a/tests/lexer.js
+++ b/tests/lexer.js
@@ -32,6 +32,17 @@
       if (lib.isArray(type)) {
         expect(tok.type).to.be(type[0]);
         expect(tok.value).to.be(type[1]);
+      } else if (lib.isObject(type)) {
+        expect(tok.type).to.be(type.type);
+        if (type.value != null) {
+          expect(tok.value).to.be(type.value);
+        }
+        if (type.lineno != null) {
+          expect(tok.lineno).to.be(type.lineno);
+        }
+        if (type.colno != null) {
+          expect(tok.colno).to.be(type.colno);
+        }
       } else {
         expect(tok.type).to.be(type);
       }
@@ -436,5 +447,289 @@
         lexer.TOKEN_SYMBOL,
         lexer.TOKEN_VARIABLE_END);
     });
+
+    it('should keep track of token positions', function() {
+      hasTokens(lexer.lex('{{ 3 != 4 == 5 <= 6 >= 7 < 8 > 9 }}'),
+        {
+          type: lexer.TOKEN_VARIABLE_START,
+          lineno: 0,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_INT,
+          value: '3',
+          lineno: 0,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_OPERATOR,
+          value: '!=',
+          lineno: 0,
+          colno: 5,
+        },
+        {
+          type: lexer.TOKEN_INT,
+          value: '4',
+          lineno: 0,
+          colno: 8,
+        },
+        {
+          type: lexer.TOKEN_OPERATOR,
+          value: '==',
+          lineno: 0,
+          colno: 10,
+        },
+        {
+          type: lexer.TOKEN_INT,
+          value: '5',
+          lineno: 0,
+          colno: 13,
+        },
+        {
+          type: lexer.TOKEN_OPERATOR,
+          value: '<=',
+          lineno: 0,
+          colno: 15,
+        },
+        {
+          type: lexer.TOKEN_INT,
+          value: '6',
+          lineno: 0,
+          colno: 18,
+        },
+        {
+          type: lexer.TOKEN_OPERATOR,
+          lineno: 0,
+          colno: 20,
+          value: '>=',
+        },
+        {
+          type: lexer.TOKEN_INT,
+          lineno: 0,
+          colno: 23,
+          value: '7',
+        },
+        {
+          type: lexer.TOKEN_OPERATOR,
+          value: '<',
+          lineno: 0,
+          colno: 25,
+        },
+        {
+          type: lexer.TOKEN_INT,
+          value: '8',
+          lineno: 0,
+          colno: 27,
+        },
+        {
+          type: lexer.TOKEN_OPERATOR,
+          value: '>',
+          lineno: 0,
+          colno: 29,
+        },
+        {
+          type: lexer.TOKEN_INT,
+          value: '9',
+          lineno: 0,
+          colno: 31,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_END,
+          lineno: 0,
+          colno: 33,
+        });
+
+      hasTokens(lexer.lex('{% if something %}{{ value }}{% else %}{{ otherValue }}{% endif %}'),
+        {
+          type: lexer.TOKEN_BLOCK_START,
+          lineno: 0,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'if',
+          lineno: 0,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'something',
+          lineno: 0,
+          colno: 6,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_END,
+          lineno: 0,
+          colno: 16,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_START,
+          lineno: 0,
+          colno: 18,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'value',
+          lineno: 0,
+          colno: 21,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_END,
+          lineno: 0,
+          colno: 27,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_START,
+          lineno: 0,
+          colno: 29,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'else',
+          lineno: 0,
+          colno: 32,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_END,
+          lineno: 0,
+          colno: 37,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_START,
+          lineno: 0,
+          colno: 39,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'otherValue',
+          lineno: 0,
+          colno: 42,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_END,
+          lineno: 0,
+          colno: 53,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_START,
+          lineno: 0,
+          colno: 55,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'endif',
+          lineno: 0,
+          colno: 58,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_END,
+          lineno: 0,
+          colno: 64,
+        });
+
+      hasTokens(lexer.lex('{% if something %}\n{{ value }}\n{% else %}\n{{ otherValue }}\n{% endif %}'),
+        {
+          type: lexer.TOKEN_BLOCK_START,
+          lineno: 0,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'if',
+          lineno: 0,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'something',
+          lineno: 0,
+          colno: 6,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_END,
+          lineno: 0,
+          colno: 16,
+        },
+        {
+          type: lexer.TOKEN_DATA,
+          value: '\n',
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_START,
+          lineno: 1,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'value',
+          lineno: 1,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_END,
+          lineno: 1,
+          colno: 9,
+        },
+        {
+          type: lexer.TOKEN_DATA,
+          value: '\n',
+        },
+        {
+          type: lexer.TOKEN_BLOCK_START,
+          lineno: 2,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'else',
+          lineno: 2,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_END,
+          lineno: 2,
+          colno: 8,
+        },
+        {
+          type: lexer.TOKEN_DATA,
+          value: '\n',
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_START,
+          lineno: 3,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'otherValue',
+          lineno: 3,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_VARIABLE_END,
+          lineno: 3,
+          colno: 14,
+        },
+        {
+          type: lexer.TOKEN_DATA,
+          value: '\n',
+        },
+        {
+          type: lexer.TOKEN_BLOCK_START,
+          lineno: 4,
+          colno: 0,
+        },
+        {
+          type: lexer.TOKEN_SYMBOL,
+          value: 'endif',
+          lineno: 4,
+          colno: 3,
+        },
+        {
+          type: lexer.TOKEN_BLOCK_END,
+          lineno: 4,
+          colno: 9,
+        });
+    });
   });
 }());