Cannot read properties of undefined (reading `'getReach'`) since v3.1.2 about bnf-parser HOT 16 CLOSED

One additional note is that it was likely introduced in #6 but that change wasn't properly compiled and packaged in the NPM package. See this diff from 3.1.1 to 3.1.2 from renovate; https://app.renovatebot.com/package-diff?name=bnf-parser&from=3.1.1&to=3.1.2

from bnf-parser.

Parsing "prop" crashed, but "prop::text" worked

from bnf-parser.

That looks right to me :)
And yes absolutely go ahead!

from bnf-parser.

Looking into it right now

from bnf-parser.

I can't believe I forgot to rerun build last time I commited a change, I only changed the readme.md between 3.1.1 to 3.1.2 - however I forgot to check I wasn't pushing outdated Typescript generated files... I'm sorry for the inconvenience.

I cannot repeat your issue, so I assume it was because I pushed the wrong prebuilt JS files to NPM.

Test case used

const {BNF} = require("../bin/index.js");
let res = BNF.parse(`
# Entry points
    ## Used when parsing the attribute
    attribute ::= ( ...association | ...identifier ) jsonPath? castOrModifiers?;
    ## Used when parsing a nested JSON path used inside of an attribute
    ## Difference with "attribute" is in the first part. Instead of accepting:
    ##  $association.attribute$ & attribute
    ## It accepts:
    ##  key, "quotedKey", and [0] (index access)
    partialJsonPath ::= ( ...indexAccess | ...key ) jsonPath? castOrModifiers? ;
    # Internals
    identifier ::= ( "A"->"Z" | "a"->"z" | digit | "_" )+ ;
    digit ::= "0"->"9" ;
    number ::= ...digit+ ;
    association ::= %"$" identifier ("." identifier)* %"$" ;
    jsonPath ::= ( ...indexAccess | ...keyAccess )+ ;
    indexAccess ::= %"[" number %"]" ;
    keyAccess ::= %"." key ;
    # path segments accept dashes without needing to be quoted
    key ::= nonEmptyString | ( "A"->"Z" | "a"->"z" | digit | "_" | "-" )+ ;
    nonEmptyString ::= ...(%"\\"" (anyExceptQuoteOrBackslash | escapedCharacter)+ %"\\"") ;
    escapedCharacter ::= %"\\\\" ( "\\"" | "\\\\" );
    any ::= !"" ;
    anyExceptQuoteOrBackslash ::= !("\\"" | "\\\\");
    castOrModifiers ::= (...cast | ...modifier)+;
    cast ::= %"::" identifier ;
    modifier ::= %":" identifier ;`);

console.log(res);

from bnf-parser.

I'm adding a second Github action now which will use the copy of bnf-parser from NPM to run the automated tests on top of the existing one which builds the current version of the package then runs using that.

from bnf-parser.

Hi! Unfortunately that did not fix our issue, with v3.1.3 we're getting the same result. See sequelize/sequelize#15795

from bnf-parser.

I'll see if I can send a PR with a failing minimal test

from bnf-parser.

There were some internal changes between v3.1.0 and v3.1.1 which could of potentially changed the structure of the outputted syntax tree, which means if you were assuming it was in a certain form and tried to access an invalid node's reach that would trigger the error.

But the only changes from v3.1.1 -> v3.1.3 are documentation apart from my hiccup of publishing the incorrect pre-built files to NPM for v3.1.2.

The fact that you're getting .getReach() is undefined either means you're treating a ParseError as a SyntaxNode or you're trying to access an invalid index of a SyntaxValue. I.e. your accessing a string char assuming it's a SyntaxNode, or just accessing an out of bound array element. All of which won't report properly in JS without manual type assertions.
🤔

from bnf-parser.

I'll try to provide a small replication of the issue but in the meantime this is the line that crashes: https://github.com/sequelize/sequelize/blob/e6690cdf27980d57be1bb9f138b04124775b153b/packages/core/src/utils/attribute-syntax.ts#L136

parse itself is what crashes, so the crash happens before we use any of the nodes it returns

from bnf-parser.

Okay, well in that case either there is a bug in my code, or there is a error in the provided BNF syntax which I'm not raising properly so that is silently erroring - either way it's an issue with my package.

If you could give me one or two sample strings that you're trying to parse as attribute - then I can also start trying to debug on my end without a full unit test for it.

from bnf-parser.

I am able to replicate the issue now, and am working on a solution

from bnf-parser.

@ephys it appears to just have been a found check on a zero->many repetition marker issue which wasn't handled correctly.
Can you confirm this is the expected output before I publish the patch incase there is something else I'm missing?

"prop" -> SyntaxNode {
  type: 'attribute',
  ref: ReferenceRange {
    start: Reference { line: 1, col: 1, index: 0 },
    end: Reference { line: 1, col: 5, index: 4 }
  },
  value: [
    SyntaxNode {
      type: 'identifier',
      ref: [ReferenceRange],
      value: 'prop',
      reach: [ReferenceRange]
    },
    SyntaxNode {
      type: 'jsonPath?',
      ref: [ReferenceRange],
      value: [],
      reach: null
    },
    SyntaxNode {
      type: 'castOrModifiers?',
      ref: [ReferenceRange],
      value: [],
      reach: null
    }
  ],
  reach: ReferenceRange {
    start: Reference { line: 1, col: 5, index: 4 },
    end: Reference { line: 1, col: 5, index: 4 }
  }
}
"prop::text" -> SyntaxNode {
  type: 'attribute',
  ref: ReferenceRange {
    start: Reference { line: 1, col: 1, index: 0 },
    end: Reference { line: 1, col: 11, index: 10 }
  },
  value: [
    SyntaxNode {
      type: 'identifier',
      ref: [ReferenceRange],
      value: 'prop',
      reach: [ReferenceRange]
    },
    SyntaxNode {
      type: 'jsonPath?',
      ref: [ReferenceRange],
      value: [],
      reach: [ReferenceRange]
    },
    SyntaxNode {
      type: 'castOrModifiers?',
      ref: [ReferenceRange],
      value: [Array],
      reach: null
    }
  ],
  reach: ReferenceRange {
    start: Reference { line: 1, col: 11, index: 10 },
    end: Reference { line: 1, col: 11, index: 10 }
  }
}

Also just double checking; sequelize/sequelize is under MIT, but I just wanted to double check you're fine with me putting your BNF syntax in my repo as a automated test case?

from bnf-parser.

Thanks for fixing this issue! Unfortunately it seems that you tried to publish this as 3.1.3 which already was published to GitHub Releases and NPM. Could you publish it as 3.1.4?

from bnf-parser.

I published 3.1.3 which had the fixes a while ago, I just forgot to mark it as a new release on Github. The only differences between 3.1.3 on NPM and 3.1.3 on Github is differences in the changelog.md.

from bnf-parser.

I ended up making a few documentation changes to justify a new patch, so v3.1.4 Github and NPM are now synced

from bnf-parser.