Lexer returning wrong line number when block strings are used

[cjoudrey]

I was debugging an issue in which a schema that was built from a schema dump (Schema.from_definition) had seemingly wrong values for ast_node.line.

I managed to pinpoint this to a bug in the Lexer when parsing block strings.

Repro script:

require "graphql"

p GraphQL::VERSION

puts

sdl = <<-GRAPHQL
"""
Query
"""

type Query {
  hello: String
}

"""
Node
"""

interface Node {
  id: ID
}
GRAPHQL

sdl.split("\n").each_with_index do |line, i|
  puts "#{(i+1).to_s.ljust(2, " ")} | #{line}"
end

puts

GraphQL::Language::Lexer.tokenize(sdl).each do |token|
  puts token.inspect
end

Output:

"1.9.0.pre1"

1  | """
2  | Query
3  | """
4  |
5  | type Query {
6  |   hello: String
7  | }
8  |
9  | """
10 | Node
11 | """
12 |
13 | interface Node {
14 |   id: ID
15 | }

(STRING "Query" [1:1])
(TYPE "type" [3:1])
(IDENTIFIER "Query" [3:6])
(LCURLY "{" [3:12])
(IDENTIFIER "hello" [4:3])
(COLON ":" [4:8])
(IDENTIFIER "String" [4:10])
(RCURLY "}" [5:1])
(STRING "Node" [7:1])
(INTERFACE "interface" [9:1])
(IDENTIFIER "Node" [9:11])
(LCURLY "{" [9:16])
(IDENTIFIER "id" [10:3])
(COLON ":" [10:5])
(IDENTIFIER "ID" [10:7])
(RCURLY "}" [11:1])

I'd expect the second token ((TYPE "type" [3:1])) to be (TYPE "type" [5:1]).

I dug into lexer.rl for a few minutes and I suspect the problem is that we aren't incrementing meta[:line] when emitting a :STRING token. Typically, meta[:line] gets incremented whenever the lexer sees a NEWLINE, but this won't account for new lines that are within the block string.

Once this is fixed, my second concern is, given the code below, what line should we be reporting:

require "graphql"

sdl = <<-GRAPHQL
"""
Query
"""

type Query {
  hello: String
}
GRAPHQL

schema = GraphQL::Schema.from_definition(sdl)
p schema.find("Query").ast_node.line

Before, when descriptions were comments, the line returned by ast_node.line would be the type token's line. Since we introduced block strings, the line reported would be the one where the description starts. While this might be correct, it is somewhat of a breaking change for tools that rely on these line numbers (like linters).

[rmosolgo]

Took a crack at it in #2056.

I wonder what we can do about that Query.ast_node.line issue, because it really seems weird.

On the one hand, you could change the Node class so that .line returns the line of type Query ... but then it's not the first line of the type definition.

Another option would be to extend the definition nodes to include references to the original tokens, for example .description_token and .name_token, so that we could distinguish between the two.

Maybe a better approach would be, rather than exposing tokens, just to expose .name_line, which is less implementation-dependent.

[cjoudrey]

On second thought, I think you're right.

If the type definition has a description, that is technically the first line where the type is defined.

I don't have any preference between adding .name_line or .name_token, I think both would work for the use-case I am working on.

[rmosolgo]

I think this was fixed, not sure why it was reopened.