Spec and graphql-js disagree on the meaning of a leading zero in numbers

[CornedBee]

A test case from graphql-js:

    expectSyntaxError('00', 'Invalid number, unexpected digit after 0: "0".', {
      line: 1,
      column: 2,
    });

But a literal reading of the grammar for IntValue simply says that 00 is two IntValue tokens, each with a value of 0.

This is obviously not useful; first there's no place in the grammar where two IntValue tokens may appear next to each other, and second, it would be really confusing.

But the spec should probably change its grammar to explicitly call this out.

[IvanGoncharov]

This is obviously not useful; first there's no place in the grammar where two IntValue tokens may appear next to each other,

Inside arrays as literal values:

{
  foo(arg: [0 0])
}

and second, it would be really confusing.

Yes, I brought this problem during the last working group meeting and used even more confusing example:

{
  foo(arg: [0x0F])
}

In this context [0x0F] is treated as [0 x0F] (number 0 and enum value x0F).
There are many similar examples in GraphQL grammars.
But at this stage of GraphQL lifecycle we can't introduce breaking changes:
https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md#guiding-principles

Once a query is written, it should always mean the same thing and return the same shaped result. Future changes should not change the meaning of existing schema or queries or in any other way cause an existing compliant GraphQL service to become non-compliant for prior versions of the spec.

So I want to include recommendation that any non-Punctuator tokens should have at least one ignored tokens between them. And the idea is to first test this in the context of graphql-js:
https://github.com/graphql/graphql-js/pull/1802/files#diff-fd078dc2466ebced817e00730708c64eR88

[CornedBee]

Inside arrays as literal values:

I realized that afterwards and thought I edited my post; I appear to have discarded that edit accidentally.

But at this stage of GraphQL lifecycle we can't introduce breaking changes:

Is it really a breaking change if what I think is effectively the reference implementation (graphql-js) doesn't follow the spec? The two primary .Net implementations, graphql-dotnet and HotChocolate reject '00' too. So does the Rust juniper parser, the Go graphql-go parser, and the PHP graphql-php parser. The two parsers that don't appear to reject '00' are the Java graphql-java and Ruby graphql-ruby parsers, which are built with parser/lexer generators rather than handcoded.

Basically, I think at this point this is more of an editorial defect in the spec in that it doesn't reflect the reality of the parsers than a bug in the parsers.

[IvanGoncharov]

@CornedBee I don't mean 00 specifically I mean the general issue of "touching" tokens.
00 is just a corner case where spec and graphql-js deviates. But graphql-js parser supports 0xF0, 1-1, 0a since the first release so we can't change lexer rules and forbid "touching" tokens.

Or do you propose to only forbid 00? I don't see how it's possible without changing general lexer rules.

[CornedBee]

Or do you propose to only forbid 00?

Just touching numeric tokens, so 01 as well, but not 0x0 or "one\"""two". (That would have been good too, but at least is somewhat comprehensible, and at least from my cursory reading there's no deviation between the spec and the actual implementations here.)

To express this, the same lookahead syntax as the LineTerminator production uses can be used, i.e. specify the first rule of the IntegerPart production as NegativeSign[opt] '0' [lookahead ≠ Digit], so that 00 and 01 do not match any production at all and become invalid source sequences.

[IvanGoncharov]

@CornedBee Since this is a change to grammar it should go through RFC process, more details here: https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md#rfc-contribution-champions
Do you want to be the champion for this proposal?