feat(rome_rowan): expose the kind of AstNode as a constant #2774

leops · 2022-06-24T10:23:47Z

Summary

This PR adds a new KIND_SET associated constant to the AstNode, representing the set of SyntaxKind values that can be cast into this node (the can_cast function would return true for these values). This allows code to statically list out all node types that match a certain node type, to optimize query matching in the analyzer for instance.

Example

This code uses the new feature to list the variants of JsSyntaxKind that can be cast into JsAnyRoot:

let root_kinds: Vec<_> = JsAnyRoot::KIND_SET.iter().collect();
assert_eq!(root_kinds.as_slice(), &[
    JsSyntaxKind::JS_MODULE,
    JsSyntaxKind::JS_SCRIPT,
    JsSyntaxKind::JS_EXPRESSION_SNIPPED,
]);

Test Plan

This is mostly a codegen change, and with most operations on SyntaxKindSet being const almost all these operations are checked at compile time (adding enough variants to JsSyntaxKind to make it overflow the 64 bytes of the bitfield would cause a compilation error for instance).
The changes to the rule registry are tested indirectly through the integration tests of the analyzer, ensuring that queries are still matched correctly

github-actions · 2022-06-24T10:28:23Z

Playground for commit 43c5c2a

github-actions · 2022-06-24T10:28:25Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	388	388	0
Failed	5558	5558	0
Panics	0	0	0
Coverage	6.53%	6.53%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12393	12393	0
Failed	3864	3864	0
Panics	0	0	0
Coverage	76.23%	76.23%	0.00%

MichaReiser · 2022-06-24T10:33:19Z

crates/rome_rowan/src/ast/mod.rs

+/// bitfield here being twice as large as it needs to cover all nodes as well
+/// as all token kinds
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct SyntaxKindSet<L: ?Sized + Language>([u128; 4], PhantomData<L>);


Would the solution be to increase the array length if it happens that we encounter a union that has more than 32 (4*128/32) variants?

The length of the array would need to be increased if any SyntaxKind ever got more than 512 (128*4) variants, currently the largest one is JsSyntaxKind with 481 variants

MichaReiser · 2022-06-24T10:34:50Z

crates/rome_rowan/src/green.rs

@@ -18,6 +18,20 @@ pub(crate) use self::node_cache::NodeCacheNodeEntryMut;
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct RawSyntaxKind(pub u16);

+impl RawSyntaxKind {
+    pub(crate) const fn as_bits(self) -> [u128; 4] {


Can you add some documentation to this part that explains what is happening here? I wouldn't know what to make of this function or how to change it in the future.

I've inlined the as_bits function in the 2 places it's used in SyntaxKindSet to make it easier to understand what it's doing in context. I also modified how the function is implemented to make easier to extend the size of the bitfield without having to introduce additional branches (I wasn't sure Rust's constant folding would be able to statically check it for error like this, but it turned out to work just as well)

cloudflare-pages · 2022-06-24T12:51:54Z

Deploying with Cloudflare Pages

Latest commit:	`abd524a`
Status:	✅ Deploy successful!
Preview URL:	https://e2b936dd.tools-8rn.pages.dev
Branch Preview URL:	https://feature-astnode-static-kind.tools-8rn.pages.dev

View logs

ematipico

Could you please add some example, somewhere, of how to use this new feature?

ematipico · 2022-06-24T13:15:26Z

crates/rome_analyze/src/registry.rs

    }
 }

-pub type MetadataIter<L> =
-    Map<IntoIter<RegistryRule<L>>, fn(RegistryRule<L>) -> (&'static str, &'static str)>;
+struct KindRules<L: Language> {


What's a KindRules? Should it be a RuleKind? Could you add some doc?

@leops I think you missed this comment. I believe this should be RulesKind. KindRules has a different meaning 😝

Interesting, I added some documentation to this struct but it's not showing in the code block attached to the comment in the conversation tab.
I think KindRules feels more natural to read left-to-right since it maps a single SyntaxKind to multiple Rule, whereas RulesKind feels more ambiguous: the -Kind suffix is generally used for enums, so it could be understood as "the kind of a collection of rules" when it's in fact a struct holding said collection of rules

It's more about english grammar here 😅 "kind rules" is like "kind person", which is the adjective to address a person that is "sweet, gentle, etc.", which is not the meaning we want to have. I'd suggest to find another wording

I guess then SyntaxKindRules would work, it makes it clearer that "kind" refers to the SyntaxKind trait

leops · 2022-06-24T14:33:48Z

Could you please add some example, somewhere, of how to use this new feature?

The new feature is being used in registry.rs to store rules in a flat vector, with each index of the vector being associated with the corresponding RawSyntaxKind value allowing for constant-time lookup of lint rules during traversal. That's a real-world use case though, I don't know if a more synthetic example would be easier to understand

ematipico · 2022-06-24T14:39:56Z

Could you please add some example, somewhere, of how to use this new feature?

The new feature is being used in registry.rs to store rules in a flat vector, with each index of the vector being associated with the corresponding RawSyntaxKind value allowing for constant-time lookup of lint rules during traversal. That's a real-world use case though, I don't know if a more synthetic example would be easier to understand

That's exactly why I think having a documented example would be beneficial for us. I looked at it and honestly I didn't understand some parts of it (for example the .to_raw().0 seemed quite hacky, and a documentation that explains it might help).

feat(rome_rowan): expose the kind of AstNode as a constant

ec775ee

leops temporarily deployed to aws June 24, 2022 10:23 Inactive

MichaReiser approved these changes Jun 24, 2022

View reviewed changes

Inline RawSyntaxKind::as_bits and make it extensible

0e43b80

leops temporarily deployed to aws June 24, 2022 12:45 Inactive

ematipico suggested changes Jun 24, 2022

View reviewed changes

Add additional documentation

7ff5359

leops temporarily deployed to aws June 24, 2022 14:32 Inactive

leops temporarily deployed to aws June 24, 2022 15:35 Inactive

Add additional documentation for the RuleRegistry::push method

8408bb3

leops force-pushed the feature/astnode-static-kind branch from c12f651 to 8408bb3 Compare June 24, 2022 15:41

leops temporarily deployed to aws June 24, 2022 15:41 Inactive

xunilrj approved these changes Jun 27, 2022

View reviewed changes

ematipico approved these changes Jun 27, 2022

View reviewed changes

rename KindRules to SyntaxKindRules

abd524a

leops temporarily deployed to aws June 27, 2022 12:55 Inactive

leops merged commit 21f39fd into main Jun 28, 2022

leops deleted the feature/astnode-static-kind branch June 28, 2022 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_rowan): expose the kind of AstNode as a constant #2774

feat(rome_rowan): expose the kind of AstNode as a constant #2774

leops commented Jun 24, 2022 •

edited

github-actions bot commented Jun 24, 2022 •

edited

github-actions bot commented Jun 24, 2022

MichaReiser Jun 24, 2022

leops Jun 24, 2022

MichaReiser Jun 24, 2022

leops Jun 24, 2022

cloudflare-pages bot commented Jun 24, 2022 •

edited

ematipico left a comment

ematipico Jun 24, 2022

ematipico Jun 27, 2022 •

edited

leops Jun 27, 2022

ematipico Jun 27, 2022

leops Jun 27, 2022

leops commented Jun 24, 2022

ematipico commented Jun 24, 2022

feat(rome_rowan): expose the kind of AstNode as a constant #2774

feat(rome_rowan): expose the kind of AstNode as a constant #2774

Conversation

leops commented Jun 24, 2022 • edited

Summary

Example

Test Plan

github-actions bot commented Jun 24, 2022 • edited

github-actions bot commented Jun 24, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-pages bot commented Jun 24, 2022 • edited

Deploying with Cloudflare Pages

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Jun 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leops commented Jun 24, 2022

ematipico commented Jun 24, 2022

leops commented Jun 24, 2022 •

edited

github-actions bot commented Jun 24, 2022 •

edited

cloudflare-pages bot commented Jun 24, 2022 •

edited

ematipico Jun 27, 2022 •

edited