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

Tolerant parsing: continue parsing when a Glimmer syntax is invalid #1666

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BlueCutOfficial
Copy link

@BlueCutOfficial BlueCutOfficial commented Nov 20, 2024

This PR is far from ready, it simply aims at starting a discussion.

Context

When using ember-template-lint, any syntax error found throws a GlimmerSyntaxError and makes the result of the lint process invalid. On a project I've been working on with @mansona, we would have needed some sort of "tolerant mode", a way to be notified about errors without blocking the linter.

The current state of this PR is just a start, and aims at gathering feedbacks about the direction it should take:

  • It takes the PartialStatement as an example and doesn't throw when a Handlebars partial is found, and the resulting AST template simply doesn't contain the node.
  • The next step would be to build some kind of error node that would have a representation in the resulting AST template.
  • Using a new class field for the Parser seemed cleaner and less intrusive to me than trying to implement it as parse function's argument.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 20, 2024

feedbacks about the direction it should take:

  1. It takes the PartialStatement as an example and doesn't throw when a Handlebars partial is > found, and the resulting AST template simply doesn't contain the node.
  2. The next step would be to build some kind of error node that would have a representation in > the resulting AST template.
  3. Using a new class field for the Parser seemed cleaner and less intrusive to me than trying to implement it as parse function's argument.

I think we should only focus on # 2 here. As a parser, we don't really want to have varying APIs as we churn through version numbers (as we normally would in post-1.0 packages). This is slightly constrained by the VM still being pre 1.0 (and package-managers treating pre-1.0 semver differently 🫠 ), but also, fault tolerance is something all our parsing tools want:

  • Glint
  • Eslint
  • Prettier

As when we can correctly parse and recover from errors we can provide more helpful information to the user to aid them in fixing their syntax errors.

For example, here is what tree-sitter does (error nodes):

Screen.Recording.2024-11-20.at.10.38.45.AM.mov

(sorry for potato quality -- I need to see if quicktime has an option to not compress so hard (inefficiently, too!))

Even within an error node, it tries to recover immediately, which also keeps syntax highlighting nice, but this also has the benefit of allowing in-progress intellisense, such as this more realistic example:
image

@BlueCutOfficial
Copy link
Author

I think we should only focus on #2 here.

I started to work on the Error node thing. I hope I can post one more commit tomorrow, and you'll tell me if I am doing this right or wrong. Thanks for the first answer and recordings 👍

@BlueCutOfficial
Copy link
Author

BlueCutOfficial commented Nov 21, 2024

Here is the result template I have so far for the "continue parsing after an error" test:
Screenshot 2024-11-21 at 10 14 40

Please let me know your thought about the code and what you think is missing to get this PR ready. I would prefer to have a first approval about the approach ahead of implementing it for the other invalid syntaxes (and I am a bit in discovery mode here, I don't have personal opinions to defend).

@NullVoxPopuli
Copy link
Contributor

yea, for partials, that's that a very nice output!

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

Successfully merging this pull request may close these issues.

2 participants