Rewind index to improve error recovery

[stasm]

The error recovery logic in the reference parser and the optimistic runtime parser is able to resume parsing at an offset which precedes the exact index of the error. Consider the following example:

foo = {
bar = Bar
     ^----- Expected token: "}"

The error occurs well into the bar… line. At the same time, I would expect bar to be a functional message and to parse correctly. Messages preceding any given message should not be allowed to break it.

This wasn't easy to achieve with the ParserStream architecture based on an iterator over the source string. I re-wrote it to use a simpler cursor-based approach. As it turned out, this simplified stream.js quite much and it also improved the performance :)

jsshell 63 (ms, compared to zeroseven):
  mean:   28.86 (-28%)
  stdev:  1.35
  sample: 30

node.js 9.11 (ms, compared to zeroseven):
  mean:   8.51 (-32%)
  stdev:  1.05
  sample: 30

[Pike]

As much as I understand the parser, this looks good to me. Simpler code is good, and also more consistency between stream API usage in parser and ftlstream.

I found one issue, where our error position is outside of our junk entry, and I don't think that's right. Good news, AFAICT, you have all the right information in getEntryOrJunk already.

And one stylistic nit.

The behavior tests are always a bit of black magic to me. They look better now than before (wth # key-05), but that's not a strong statement.

I'd love to cross-check the fixtures after the fix, so doing a request-changes instead of approve-with-comments.

[Pike]

I think we should rewind the error position, too.

[stasm]

If we rewind the error position, pretty printing the annotations (or highlighting them in IDEs) would always point to the beginning of the line. Which I'll admit is sometimes good, and sometimes bad :) It's the choice between:

A) Error position unchanged.

# This makes sense to me.
! E0003 on line 2:
  | foo = {
  | invalid placeable }
  …         ^----- Expected token: "}"

# Meh, this is kind of misleading.
! E0003 on line 2:
  | foo = {
  | bar = Bar
  …     ^----- Expected token: "}"

And:

B) Error position rewound.

# Meh, we could do better.
! E0003 on line 2:
  | foo = {
  | invalid placeable }
  … ^----- Expected token: "}"

# This makes sense to me.
! E0003 on line 2:
  | foo = {
  | bar = Bar
  … ^----- Expected token: "}"

I take it that you vote for B, as it's likely more common?

[Pike]

Just sugar, I'd love for next() and peek() to be symmetric in their implementation. I can see reasons for both ways, maybe perf can throw a dice?

[stasm]

I see what you mean. There's no difference in the perf benchmarks, but I like the idea of making the implementation symmetric. Also, I think I'll ditch the pre-increment operator in next(). It's easy to miss when reading the code.

[Pike]

r=me, not feeling strongly about the const nit, I'm not zibi ;-)

[Pike]

Nit, this can still be a const, right?

[stasm]

Ah, yes. I stopped using const in most of the code I write these days and only use them for actual constants. I typed let out of habit. I'll revert to const for consistency with the rest of the file.