Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReferenceParser prevents repeatString optimization #155

Open
amake opened this issue Aug 28, 2023 · 1 comment
Open

ReferenceParser prevents repeatString optimization #155

amake opened this issue Aug 28, 2023 · 1 comment

Comments

@amake
Copy link

amake commented Aug 28, 2023

Consider this grammar:

class MyGrammar extends GrammarDefinition {
  @override
  Parser start() => ref0(myChars).starString().end();

  Parser<String> myChars() => anyOf('abc');
}

The linter will produce a warning to use one of the *String parsers, despite this already being the case:

expect(linter(MyGrammar().build()), isEmpty);

Output:

  Expected: empty
    Actual: [
              LinterIssue:LinterIssue(type: LinterType.warning, title: Character repeater, parser: Instance of 'FlattenParser<List<String>>', description: A flattened repeater (Instance of 'PossessiveRepeatingParser<String>'[0..*]) that delegates to a character parser (Instance of 'SingleCharacterParser'[any of " \t" expected]) can be much more efficiently implemented using `starString`, `plusString`, `timesString`, or `repeatString` that directly returns the underlying String instead of an intermediate List.)
            ]

I think that the cause is that the class of myChars is obscured by the wrapping ReferenceParser so that the type checks in repeatString fail and we end up on this line:

return self.repeat(min, max).flatten(message);

This seems like a difficult problem: the class of the referent is not known until build time, but repeatString needs to be called earlier.

Is the only solution here "don't use a reference"?

(Bigger question: Should references only be used to break cycles? I was under the impression that it was good practice to use them by default.)

@renggli
Copy link
Member

renggli commented Aug 28, 2023

Good observation and analysis. This is indeed a tricky problem that might cause subtle differences in a few other places too, such as when flattening of | / .or(Parser) and & / .seq(Parser) operators.

Another workaround could be to call optimize on the built parser:

expect(linter(optimize(MyGrammar().build())), isEmpty);

Regarding references: This is really up to you. The idea was that if you always use ref you can forget about cycles, but I have written grammars where ref0 was only used between productions and tokens were stored in (constant) variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants