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

Completely optional parser rules should be marked as errors #1400

Open
msujew opened this issue Mar 1, 2024 · 5 comments · May be fixed by #1459
Open

Completely optional parser rules should be marked as errors #1400

msujew opened this issue Mar 1, 2024 · 5 comments · May be fixed by #1459
Labels
grammar Grammar language related issue help wanted Extra attention is needed validation Validation related issue

Comments

@msujew
Copy link
Contributor

msujew commented Mar 1, 2024

Related to #1398

A parser rule that potentially consumes no input from the text is not valid, as the parser can potentially not attempt to create it:

Persons:
    ((elements+=ID) (elements+=ID)*)?;

This leads to unexpected behavior, and generally serves no purpose. We have a similar validation for terminals already, and should have one for parser rules as well.

@msujew msujew added help wanted Extra attention is needed validation Validation related issue grammar Grammar language related issue labels Mar 1, 2024
@bjthehun
Copy link
Contributor

bjthehun commented Mar 26, 2024

I'd like to work on this issue. Thoughts so far:

  • What should happen when the entry rule does not consume any input, like the one in the domainmodel example?
  • What about guard conditions in parser rules? These might get arbitrary complex, and I would find it easier to assert for now that those can be fulfilled sometimes.
  • For a rule B that depends on an optional rule A like B: a1=A a2=A; B itself should not need not be marked as completely optional, right? After all, if A would always consume input, then so would B.

@spoenemann
Copy link
Contributor

My two cents:

  • A rule marked as entry should be excluded from this validation: you usually want an empty file to be valid.
  • I'd already raise an error if a rule can potentially not consume any input, which means you don't have to care about guard conditions. If you encounter an alternative, check whether any of the alternatives is potentially empty.
  • When calling other rules, I'd assume that they do consume input, so that counts as being non-empty.
  • I would also count actions like {MyType} as being non-empty. Actually we could offer a code action as quick fix that adds an action to enforce the rule to be valid despite consuming no tokens.

@bjthehun
Copy link
Contributor

bjthehun commented Mar 28, 2024

* I would also count actions like `{MyType}` as being non-empty. Actually we could offer a code action as quick fix that adds an action to enforce the rule to be valid despite consuming no tokens.

So to clarify, this grammar would be valid, right?

grammar HelloWorld

interface Empty {};

entry Model:
    Emptyness;
// Action in a parser rule that consumes nothing
Emptyness: {Empty};

hidden terminal WS: /\s+/;

hidden terminal ML_COMMENT: /\/\*[\s\S]*?\*\//;
hidden terminal SL_COMMENT: /\/\/[^\n\r]*/;

Also, what about fragments? Should optionality be checked for them, too?

@bjthehun bjthehun linked a pull request Apr 18, 2024 that will close this issue
@spoenemann
Copy link
Contributor

So to clarify, this grammar would be valid, right?

It would be helpful if that works, but I'm not sure whether it's problematic for the parser.

Also, what about fragments? Should optionality be checked for them, too?

No, a fragment should be allowed to consume no tokens. When calling a fragment rule, treat its content as if it was part of the calling rule.

@bjthehun
Copy link
Contributor

OK. I've changed the validation to reflect these cases, and to fix false-positive errors with alternative groups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grammar Grammar language related issue help wanted Extra attention is needed validation Validation related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants