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

Rename #[visited_node] attr to #[ast] #4282

Closed
overlookmotel opened this issue Jul 15, 2024 · 4 comments · Fixed by #4289
Closed

Rename #[visited_node] attr to #[ast] #4282

overlookmotel opened this issue Jul 15, 2024 · 4 comments · Fixed by #4289
Assignees
Labels
A-ast Area - AST

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Jul 15, 2024

At present we only have #[visited_node] attrs on types which are visited.

We will also need to add an attribute to all types which are part of AST but are not visited, for:

Currently:

#[visited_node]
enum Expression<'a> { /* ... */ }

I propose:

#[ast(visit)]
enum Expression<'a> { /* ... */ }

or:

#[ast]
#[visit]
enum Expression<'a> { /* ... */ }

Types which are part of AST, but which are not visited would omit the visit part e.g.:

#[ast]
pub enum FunctionType { /* ... */ }

Bikeshedding

We could call the attr something other than #[ast]. I considered #[ast_node] but:

  1. That name will be familiar to some from SWC, and I don't think we should use it because these attrs do something very different - they're no-op markers for codegen, rather than macros generating a huge amount of trait impls and other code.
  2. Some of these types (e.g. FunctionType) are not AST nodes per se.

The common factor between all these types is that they're part of the AST, so #[ast] seems descriptive.

But I'm not at all set on that name, if someone has a better suggestion.

cc @Boshen @rzvxa.

@overlookmotel overlookmotel added the A-ast Area - AST label Jul 15, 2024
@rzvxa
Copy link
Contributor

rzvxa commented Jul 15, 2024

From day one I was a fan of this more than any other way so that's where I'm.

#[ast(visit)]
enum Expression<'a> { /* ... */ }

If you remember at first I was more in favor of naming it ast_node but your arguments against it(both here and #3815 (comment)) made me change my decision. #[ast] seems like a good choice👍

@rzvxa
Copy link
Contributor

rzvxa commented Jul 15, 2024

I had an idea for generated derives which I've never shared publicly. We might want to add an attribute such as #[expand_derive(...)] or #[derived(...)] to easily mark stuff like Hash, PartialEq, and now CloneIn derives.

It is a marker and wouldn't expand the derived macro base on the builtins(or proc_macro_derive), but it is a good way to telegraph that these are being derived for the said type although they are generated and not happening in the compiler.

@rzvxa rzvxa self-assigned this Jul 16, 2024
@overlookmotel
Copy link
Contributor Author

@Boshen and I discussed on video call just now. He's given his 👍 to this.

overlookmotel pushed a commit that referenced this issue Jul 20, 2024
closes #4282
I went with `#[ast(visit)]` but we can change it to 2 separate markers as suggested by @overlookmotel in the issue.
@rzvxa
Copy link
Contributor

rzvxa commented Aug 4, 2024

closed via #4289

@rzvxa rzvxa closed this as completed Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants