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

Use the Crystal parser to verify corpus tests #59

Merged
merged 6 commits into from
Dec 23, 2024

Conversation

keidax
Copy link
Collaborator

@keidax keidax commented Dec 22, 2024

This adds two things:

  • An implementation of the visitor pattern that traverses a Crystal::ASTNode tree and prints out an AST in the same format as tree-sitter.
  • A test script using the new visitor to ensure the Crystal compiler generates the same parse tree as tree-sitter for specific corpus tests.

tree-sitter.json can include multiple grammars. The current tag system for tests lets you use :language(LANG) to indicate a test should be parsed with a different grammar than the first grammar listed in tree-sitter.json. So, :language(crystal) is implicitly added to every test that doesn't specify a language tag.

I'm overloading this tag as an explicit opt-in mechanism for the new script here. Corpus tests tagged explicitly with :language(crystal) will be run by tree-sitter test exactly the same way as before. The tag is also the signal for crystal_parse_corpus.cr to pick up that corpus test.

I found a few tests that it doesn't make sense to verify, because Crystal just handles nodes in a way that's too different from tree-sitter. For the first iteration, I've enabled parser verification on about 30 tests. Planning to iterate with future PRs to expand this as much as possible. My end goal is making sure subtle precedence issues like crystal-lang/crystal#15303 don't slip under the radar again!

Base automatically changed from keidax/node-reorganization to main December 23, 2024 00:15
@devnote-dev
Copy link
Collaborator

I think we'd need to review which tests should be included for this, as well as which tests we rely on the Crystal compiler on rather than the grammar.

The issue is found in various parts of the language, for example, weird call assignments. Crystal allows quite a few of them when it shouldn't (work is in progress to have these disallowed), but until then it would be wrong to support it in the grammar just because it's presently valid.

@keidax keidax force-pushed the crystal-parser-verification branch 3 times, most recently from 0eaf7c1 to 43cdfcc Compare December 23, 2024 01:17
@keidax
Copy link
Collaborator Author

keidax commented Dec 23, 2024

I think we'd need to review which tests should be included for this, as well as which tests we rely on the Crystal compiler on rather than the grammar.

Just to be clear, this is an additional set of tests. It's not replacing using the grammar for any tests, tree-sitter test is still the baseline.

The issue is found in various parts of the language, for example, weird call assignments. Crystal allows quite a few of them when it shouldn't (work is in progress to have these disallowed), but until then it would be wrong to support it in the grammar just because it's presently valid.

I agree the grammar shouldn't support behavior that's clearly a bug.

However, in the absence of a formal language specification, I've been relying on the compiler as the source of truth. By using the compiler to verify as many grammar examples as we reasonably can, we'll know right away if a change to the compiler needs to be reflected in the grammar.

I'm not aware of the call assignments issue, is that crystal-lang/crystal#14527 / crystal-lang/crystal#14815?

@keidax keidax force-pushed the crystal-parser-verification branch from 43cdfcc to f0c20cf Compare December 23, 2024 01:34
@devnote-dev
Copy link
Collaborator

It was the latter PR but I didn't know it had been merged recently, so that's one less thing.

There's also issues like class Foo:Bar being valid, but I haven't verified which bugs the tree-sitter unknowingly permits so it's probably better to manage those as a separate issue.

test/crystal_parse_corpus.cr Outdated Show resolved Hide resolved
test/crystal_parse_corpus.cr Outdated Show resolved Hide resolved
@keidax keidax requested a review from devnote-dev December 23, 2024 15:07
@keidax keidax merged commit f125867 into main Dec 23, 2024
3 checks passed
@keidax keidax deleted the crystal-parser-verification branch December 23, 2024 17:31
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.

3 participants