grammarkdown
grammarkdown copied to clipboard
Add emu-grammar lint rules from ecmarkup to Grammarkdown's grammar checker
ecmarkup
has a few lint rules that grammarkdown doesn't enforce on its own, but that make sense:
-
[empty]
assertions should have no other content: https://github.com/tc39/ecmarkup/blob/52df46d6177a14f6d101a34aecbebf5834c77571/src/lint/utils.ts#L70 -
but not
operators cannot be empty: https://github.com/tc39/ecmarkup/blob/52df46d6177a14f6d101a34aecbebf5834c77571/src/lint/utils.ts#L132 -
one of
operators cannot be empty: https://github.com/tc39/ecmarkup/blob/52df46d6177a14f6d101a34aecbebf5834c77571/src/lint/utils.ts#L144 - Parsing constraints on a RHS should report an error if no symbols follow: https://github.com/tc39/ecmarkup/blob/52df46d6177a14f6d101a34aecbebf5834c77571/src/lint/utils.ts#L66
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:
- Arguments should have operators:
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
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.
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
.
Makes sense. Presumably it generates diagnostics for those cases? (Or will, when this issue is closed.) If so, I'll probably convert these throw
s 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.