BlockStringCharacter production doesn't quite work

[glasser]

The definition of BlockStringCharacter is:

BlockStringCharacter ::
  - SourceCharacter but not `"""` or `\"""`
  - `\"""`

But if I understand correctly, SourceCharacter is always a single character. So the concept of but not-ing a multi-character sequence out of SourceCharacter doesn't really make sense.

I think this should be something more like

BlockStringCharacter ::
  - SourceCharacter but not `"` or `\`
  - `"` [lookahead != `"`]
  - `""` [lookahead != `"`]
  - `\` [lookahead != `"`]
  - `\"` [lookahead != `"`]
  - `\""` [lookahead != `"`]
  - `\"""`

I'm also not super clear after reading the spec if, eg,

"""

\""""

"""

is legit. After all you could interpret the spec as saying "three double quotes cannot be in the block string unless there's a backslash immediately before them" which is not the case here. (My version would make that allowed and parsed as \""" followed by ". graphql-js certainly treats it this way.)

Thoughts?

[benjie]

A grammar production may specify that certain characters or tokens are not permitted to follow it by using the pattern {[lookahead != NotAllowed]}. Lookahead restrictions are often used to remove ambiguity from the grammar. -- https://spec.graphql.org/draft/#sec-Grammar-Notation.Lookahead-Restrictions

Looks like it's okay to put more than one character in the lookahead; so we could simplify this to:

BlockStringCharacter ::
  - SourceCharacter but not `"` or `\`
  - `"` [lookahead != `""`]
  - `\` [lookahead != `"""`]
  - `\"""`

I agree that the existing implementation is ambiguous; I believe that this edit respects the status quo, e.g. surprisingly allowing \"""" (normally I'd expect the \" to be ignored, and the """ to end the block quote, but as you point out that's not the current behaviour of the reference implementation, and doesn't seem to be implied by the existing spec text.