grammarkdown icon indicating copy to clipboard operation
grammarkdown copied to clipboard

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

Open rbuckton opened this issue 4 years ago • 3 comments

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:

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

rbuckton avatar Jan 21 '21 05:01 rbuckton

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.

bakkot avatar Jan 21 '21 05:01 bakkot

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.

rbuckton avatar Jan 21 '21 06:01 rbuckton

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.

bakkot avatar Jan 21 '21 06:01 bakkot