feat(rome_js_parser): check for missing children in parser tests #2176
Conversation
Parser conformance results on ubuntu-latestjs/262
jsx/babel
ts/babel
ts/microsoft
|
xtask/codegen/src/generate_nodes.rs
Outdated
| let missing_children = if is_list { | ||
| quote! { | ||
| .chain( | ||
| self.#method_name().missing_children() |
There was a problem hiding this comment.
I'm having a hard time getting my head around. Does this correctly handle separated lists?
There was a problem hiding this comment.
I'm having somewhat conflicting feelings about this change.
It does achieve the goal but it results in a lot of additional code and start wondering if it instead would be useful to expose the metadata for each node in a different form. How many fields does a JS_MODULE have, which fields are mandatory or optional? Is it a separated list?
Something like this could also be useful when thinking about serializing/presenting nodes #2120
An alternative approach to this which I once planned to do but never came along is to add a contains_syntax_errors flags to SyntaxNode's and that flag gets set to true inside of the JsSyntaxFactory if any child node has that flag or if any required child is missing.
This wouldn't just be useful here but also in other places where we want to assert that a tree is valid. Ideally, adding a diagnostic would also toggle that flag but that will require some changes to the way we emit diagnostics today.
crates/rslint_parser/src/tests.rs
Outdated
| let buffer = buffer.into_inner(); | ||
| let mut buffer = String::from_utf8(buffer).unwrap(); | ||
| for syntax_error in syntax_errors { | ||
| writeln!(buffer, "{}", syntax_error).expect("writing to a String cannot fail"); | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be neat for debuggability if this would emit Diagnostic's too instead of writing to a string. But I can see why this is difficult.
A cheap way of implementing the same is to print the AST as a string and then assert that it doesn't contain missing (required).
Deploying with
|
| Latest commit: |
814f083
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://45a523de.tools-8rn.pages.dev |
leops
force-pushed
the
feature/report-missing-children
branch
from
35ffdf2
to
c53b078
Compare
|
@leops is it OK if we close this PR for now? |
|
Oh I completely forgot about this one. In the most recent version of this PR it just checked whether there was any "missing (required)" in the debug representation of the tree, so I think it worked as expected without adding too much new code but if I remember correctly I didn't move it out of draft at the time because TS support was incomplete at the time and the test was failing then. Now that's it's stable (and in preparation for the future other languages we want to add) maybe I could rebase it on the latest main ? |
|
Oh, didn't notice you updated it. Yeah, that looks useful. Let's rebase it and see if all tests pass. |
leops
force-pushed
the
feature/report-missing-children
branch
from
c53b078
to
814f083
Compare
leops
changed the title
Summary
This adds an additional check in the tests of
rome_js_parserto verify the parsed result doesn't contain syntax errors by printing out the debug representation of the AST and checking if the resulting text contains themissing (required)string