Implement the legalizer #438
Conversation
this contains all of the functions used in the legalizer as well as the recursive tree walk, but it does not contain the diagnostics, which will come in commits that follow.
There was a problem hiding this comment.
I've made it through 16 out of 26 commits. I know I've got some big commits still ahead of me (legalize message and friends; legalize fields), which I am still working through.
But I figured I should go ahead and publish the remarks I have so far since there has been no other activity on the PR since we talked a couple of days ago. I promise I've been working on it and not ignoring it! :)
| 19 | package 42; | ||
| | ^^^^^^^ | ||
| = help: to place a file in the empty package, remove the `package` declaration | ||
| = help: however, using the empty package is discouraged |
There was a problem hiding this comment.
This doesn't seem like a good diagnostic. Perhaps the AST node should allow an expression instead of just a path here, so that you diagnose 42 is an invalid type for the package name?
(Totally fine to be a TODO: I know changing the AST at this point might be enough churn to warrant a separate PR.)
There was a problem hiding this comment.
I explained elsewhere why this doesn't work properly. There's already a TODO in the .proto file.
| ... | | ||
| 22 | | import weak "foo.proto"; | ||
| | | ^^^^^^^^^^^^^^^^^^^^^^^^ this weak import... | ||
| 23 | | |
There was a problem hiding this comment.
Why does this diagnostic have an extra line of context after the snippet? The two above do not, so perhaps it's a bug?
If it's a quirk/bug in the reporting, totally fine to fix that in a separate PR. But, if it's an issue in the way the diagnostic is constructed in this commit, then let's try to fix in this branch.
There was a problem hiding this comment.
it's a quirk with the renderer, but it was easy enough to find and fix.
| | | ||
| 19 | / message M { | ||
| ... | | ||
| 23 | | |
There was a problem hiding this comment.
Same as above: why is there an extra line of context before the snippet?
This PR adds the legalizer: a collection of diagnostics that follow parsing to eliminate and diagnose invalid constructs that the parser accepted in order the make progress.
This PR also adds a new
syntaxpackage for e.g.syntax.Proto2and other editions knowledge. It adds functions for classifyingpredeclared.Names and some newtaxa.Nouns for naming concepts in diagnostics relevant to the legalizer. Parsing is also tweaked in a few places to generate better diagnostics.