Skip to content

Disallow empty placeholders in the syntax #345

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

Closed
stasm opened this issue Feb 14, 2023 · 3 comments · Fixed by #348
Closed

Disallow empty placeholders in the syntax #345

stasm opened this issue Feb 14, 2023 · 3 comments · Fixed by #348
Labels
blocker-candidate The submitter thinks this might be a block for the next release resolve-candidate This issue appears to have been answered or resolved, and may be closed soon. syntax Issues related with syntax or ABNF

Comments

@stasm
Copy link
Collaborator

stasm commented Feb 14, 2023

I'm refactoring our BNF in #344 to explicitly define whitespace rules. In order to do so, I had to change our grammar from LL(1) to LL(1) with backtracking.

In the process I noticed an opportunity to make the grammar LL(2). IIUC, this provides more guarantees about the complexity of parsing than LL(1) with backtracking. LL(2) is still O(n), while backtracking can be arbitrarily long and can result in exponential worst-case complexity.

-Placeholder ::= '{' s? (Expression | Markup | MarkupEnd)? s? '}'
+Placeholder ::= '{' s? (Expression | Markup | MarkupEnd) s? '}'

Do we need empty placeholders?

I recall previous discussions with @eemeli about potentially allowing comment-only or metadata-only placeholders in the future. Any thoughts on this?

Originally posted by @stasm in #344 (comment)

@stasm
Copy link
Collaborator Author

stasm commented Feb 14, 2023

My current thinking is that it'd be fine to forbid empty placeholders in the syntax. In the data model I'd probably allow for them, but that's really an entirely separate question.

Originally posted by @eemeli in #344 (comment)

@stasm
Copy link
Collaborator Author

stasm commented Feb 14, 2023

Ah, an interesting point about being strict in the syntax and lenient in the data model. I like the approach.

Originally posted by @stasm in #344 (comment)

@aphillips aphillips added syntax Issues related with syntax or ABNF blocker-candidate The submitter thinks this might be a block for the next release labels Feb 14, 2023
@aphillips
Copy link
Member

Note that in MF1 an empty placeholder is an error.

@stasm stasm changed the title Disallow empty placeholders Disallow empty placeholders in the syntax Feb 14, 2023
@stasm stasm added the resolve-candidate This issue appears to have been answered or resolved, and may be closed soon. label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker-candidate The submitter thinks this might be a block for the next release resolve-candidate This issue appears to have been answered or resolved, and may be closed soon. syntax Issues related with syntax or ABNF
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants