Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_rowan): add a declare_union macro for one-off union AstNodes #2691

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Jun 9, 2022

Summary

This PR add a new declare_union for creating union AST nodes scoped to a local module, without having to add them to js.ungram and rome_js_syntax. Its usage looks like the following example:

declare_union! {
    /// Documentation
    #[allow(dead_code)]
    pub(crate) UnionName = NodeType | OtherNodeType
}

The resulting union type is an enum with a variant for each node type in the union and implementing:

  • the AstNode trait
  • the Debug and Display traits
  • the Clone, PartialEq, Eq and Hash traits with derive
  • the From trait for each variants
  • the Into trait for SyntaxNode and SyntaxElement

The macro is designed to fail to compile when the union would contain node types from different languages, as the AstNode trait must have a single Language associated type. I'm not really sure if and how this is a restriction we may lift in the future.

Test Plan

This macro is now used to implement the JsAnyBinaryLikeExpression and JsAnyBinaryLikeLeftExpression types in the formatting of binary-like expressions

@leops leops temporarily deployed to aws June 9, 2022 15:14 Inactive
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

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 350 350 0
Failed 5595 5596 ❌ ⏫ +1
Panics 1 0 ✅ ⏬ -1
Coverage 5.89% 5.89% 0.00%
⁉️ Panic To Failed (1):
typeGuardsAsAssertions.symbols

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 12391 12391 0
Failed 3866 3866 0
Panics 0 0 0
Coverage 76.22% 76.22% 0.00%

/// }
/// ```
#[macro_export]
macro_rules! declare_union {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we call declare_language_union or declare_node_union (this is maybe better, because it applies AstNode)?

The name, right now, is so generic that we could misuse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to declare_node_union, I also think it's the better option since the type being declared and all the types it contains must be AST nodes

type Language: Language;
}

macro_rules! impl_union_language {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I can't really understand the purpose of this macro. Could you explain please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This macro is an implementation detail of declare_union (it's not exported from the macros module) aimed at making it easier to implement the UnionLanguage trait. This trait is implemented for tuples of various arity, but because tuples are heterogeneous the implementation needs to declare one generic type for each element of the tuple. This in turn means it's impossible to implement the trait once for all tuples size, instead implementing the trait would look like this:

impl<T1> UnionLanguage for (T1,) { ... }
impl<T1, T2> UnionLanguage for (T1, T2) { ... }
impl<T1, T2, T3> UnionLanguage for (T1, T2, T3) { ... }

All these impl blocks share the same code with the only difference being the number of generic types, so it was simpler to write that code in a macro so it can easily be repeated for various numbers of generic arguments

@leops leops temporarily deployed to aws June 10, 2022 07:27 Inactive
@cloudflare-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: e0a3f41
Status: ✅  Deploy successful!
Preview URL: https://ec176d8e.tools-8rn.pages.dev
Branch Preview URL: https://feature-declare-union-macro.tools-8rn.pages.dev

View logs

@leops leops merged commit 0598552 into main Jun 10, 2022
@leops leops deleted the feature/declare-union-macro branch June 10, 2022 08:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants