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

Add emu-grammar lint rules from ecmarkup to Grammarkdown's grammar checker #68

Open
rbuckton opened this issue Jan 21, 2021 · 3 comments

Comments

@rbuckton
Copy link
Owner

ecmarkup has a few lint rules that grammarkdown doesn't enforce on its own, but that make sense:

There are also a few cases that indicate some property types include | undefined that shouldn't, including:

  • Production.body (unconditionally set during parse)
  • Argument.name (unconditionally set during parse)

In other cases, grammarkdown already reports errors during parse that are also being reported by lint:

In general, I'd like to move a lot of these checks to grammarkdown since they aren't precisely "lint" checks (i.e., cases where you're enforcing stylistic consistency with otherwise acceptable source text), but rather are syntactic or semantic checks that should result in grammarkdown reporting errors regardless of consumer (be it ecmarkup, or the grammarkdown-vscode extension, etc.).

cc: @bakkot

@bakkot
Copy link
Contributor

bakkot commented Jan 21, 2021

Strictly speaking none of those are linting checks: they're error conditions, and will fail the build entirely if hit (by throwing). Contrast a genuine lint failure, which only fails the build if you pass --strict, and only at the end of the build. Those lines of code are only there because the TypeScript types imply those cases are possible (or at least they did at the time I wrote that code), and I wanted to be defensive against that possibility.

If the types can be made precise enough to rule out those cases, I'd be very happy to drop those checks from ecmarkup. If not I'll probably keep them as-is, even if grammarkdown makes them impossible; they'd then basically just be assertions, which don't hurt much, and they please the type checker. (I'd update the error messages to make that clear.) And as you say, either way it's definitely nicer for grammarkdown to do these sorts of checks itself.

@rbuckton
Copy link
Owner Author

In some cases, grammarkdown uses undefined as an indicator that something was parsed incorrectly, but lets a separate pass handle syntactic and semantic checks, this way I can leverage the AST in a language service and provide suggestions for completions and error recovery. In those cases I'll leave in the | undefined, but for cases where I always create a node I shouldn't have | undefined.

@bakkot
Copy link
Contributor

bakkot commented Jan 21, 2021

Makes sense. Presumably it generates diagnostics for those cases? (Or will, when this issue is closed.) If so, I'll probably convert these throws to instead just stop further checking of the emu-grammar in question, and rely on forwarding the diagnostics from grammarkdown to surface the issue to the user.

It'd be nice if the documentation could call out which fields which are typed as | undefined are guaranteed to in fact be defined when there are no diagnostics.

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

No branches or pull requests

2 participants