Lexer.lookahead() seems to violate its comment about not modifying the state of the lexer

[haikusw]

Apologies in advance if I'm misreading this and for not providing a PR that includes a fix and a test verifying the issue (no Typescript setup or experience).

I'm working on enhancing the Swift graphql project which mirrors this js implementation with more complete support for some of the graphql definition language parsing it is currently missing.

In reading the source for the lexer.js lookahead function I see the comment says:

/**
   * Looks ahead and returns the next non-ignored token, but does not change
   * the state of Lexer.
   */

However it appears that blockstring support modified the implementation of readToken called by lookahead such that it can now change the state of the lexer in violation of the claim in the comment above and the usual expectation for a lookahead style function of a lexer. Not sure if that actually matters in this codebase (yet ;) ), but…

This modification of the lexer is possible because lookahead calls readToken passing in the lexer and readToken calls readBlockString which also passes along the lexer - on line 192.

The passed-in lexer is modified in readBlockString on line 569 and again down further on lines 569, 570 and 578.

Thought I'd mention it so that someone working on the project can write a quick test and decide if this is the bug lurking in the shadows it appears to potentially be.

Cheers.

P.S. In my implementation of lookahead I simply save off line and lineStart at the beginning and then restore before returning from lookahead. Somewhat fragile in that any future changes to readToken or readBlockString that modify additional fields of the lexer may not know to update the save/restore code in lookahead, so a collection of unit tests to catch this if it happens is something I plan to add. Considered duplicating the lexer but that has performance considerations. Anyway, seems likely something similar will work in this js version.

[IvanGoncharov]

@haikusw Yes, you absolutely right. Support of block strings is a relatively new addition compared to the rest of GraphQL so it was implemented somewhat ad-hoc.
We definitely need to return to previous behavior and since we need to rewrite Lexer to support comments as part of the AST we can address this issue at the same time.

[haikusw]

Thanks for the confirmation. Not sure your schedule for those bigger changes, but something like the save/restore strategy I did for the Swift version of `lookahead()` here: https://github.com/GraphQLSwift/GraphQL/blob/023af5acd113dba5502d2357558bc91cc71c8d71/Sources/GraphQL/Language/Lexer.swift#L99 (Lines 101-102 and then 116-117) might be a decent short-term fix if those bigger things are out on the horizon some. peace.

…

On Aug 28, 2020, at 6:41 AM, Ivan Goncharov ***@***.***> wrote: @haikusw <https://github.com/haikusw> Yes, you absolutely right. Support of block strings is a relatively new addition compared to the rest of GraphQL so it was implemented somewhat ad-hoc. We definitely need to return to previous behavior and since we need to rewrite Lexer to support comments as part of the AST we can address this issue at the same time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABWIP236CMQAFLDU2XTIOLSC6XXZANCNFSM4QNFFBXQ>.

[yaacovCR]

Cross-posting from GraphQLSwift/GraphQL#158

Basically, when the comment says the lexer start is not changed, I think it means with respect to the current token only, but not with respect to other internal state.

In fact, I think there is an optimization that means additional lexer state can and must change.

Specifically, when we look ahead, we save the next token within the linked list, even though we don't modify the current token pointer, such that when we advance() after calling lookahead() we don't have to reprocess the next token, we just read from token.next. (advance() works by calling lookahead() and then specifically advancing the current token pointer.)

Considering that we never re-lex a token, the line and start position of the line within the body must be permanently advanced by lookahead() otherwise the lexer will think that is on the old line. From what I can tell, parsing will continue as normal, because we lex from the end position of the last token, and that won't be incorrect, but every token from that point on will have the wrong line number and presumably the wrong column.

[yaacovCR]

Closed by #4294