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

ast: Use enum dispatch for tree traversal #10382

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
33 changes: 28 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions Cargo.toml
@@ -1,7 +1,6 @@
[workspace]
resolver = "2"
members = [
]
members = []

[workspace.package]
rust-version = "1.67"
Expand All @@ -24,6 +23,8 @@ fast-float = { git = "https://github.com/fish-shell/fast-float-rust", branch="fi
hexponent = { git = "https://github.com/fish-shell/hexponent", branch="fish" }
printf-compat = { git = "https://github.com/fish-shell/printf-compat.git", branch="fish" }

fish-macros = { path = "./macros" }

bitflags = "2.4.0"
errno = "0.2.8"
lazy_static = "1.4.0"
Expand All @@ -35,6 +36,7 @@ once_cell = "1.17.0"
rand = { version = "0.8.5", features = ["small_rng"] }
widestring = "1.0.2"
terminfo = { git = "https://github.com/meh/rust-terminfo", rev = "870327dd79beaecf50db09a15931d5ee1de2a24d" }
enum_dispatch = "0.3.12"

[dev-dependencies]
rand_pcg = "0.3.1"
Expand Down
13 changes: 13 additions & 0 deletions macros/Cargo.toml
@@ -0,0 +1,13 @@
[package]
name = "fish-macros"
version = "0.1.0"
rust-version.workspace = true
edition.workspace = true

[lib]
proc-macro = true

[dependencies]
proc-macro2 = "1.0.79"
quote = "1.0.35"
syn = { version = "2.0.53", features = ["full"] }
172 changes: 172 additions & 0 deletions macros/src/ast_node.rs
@@ -0,0 +1,172 @@
use proc_macro2::TokenStream;
use quote::{format_ident, quote, ToTokens};
use std::collections::HashMap;
use syn::{parse_quote, punctuated::Punctuated, Fields, Ident, ItemEnum, Token};

pub struct State<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

can you put the relevant PR description into the commit message, to help git blame

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'm sorry, I don't understand what you mean by this.

Copy link
Member

Choose a reason for hiding this comment

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

It's often a good idea to explain why some decisions where made,
the traditional place to put such explanation is the commit message.
For example it might be good explain why blanket_ref and NodeRef exist.

See also https://github.com/git/git/blob/79bdd48716a4c455bdc8ffd91d57a18d5cd55baa/Documentation/SubmittingPatches#L110-L225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I've pushed some additional commits since your initial review, should I do this as part of a second squash commit or what?

enums: &'a [ItemEnum],
tree: HashMap<&'a Ident, Vec<&'a Ident>>,
}

impl<'a> State<'a> {
pub fn new(enums: &'a [ItemEnum]) -> Self {
Self {
enums,
tree: enums
.iter()
.map(|e| (&e.ident, e.variants.iter().map(|v| &v.ident).collect()))
.collect(),
}
}

pub fn generate_enum_definitions(&self, stream: &mut TokenStream) {
for flavor in FLAVORS {
for definition in self.enums {
let new_def = flavor.transform(definition, |x| self.tree.contains_key(x));
new_def.to_tokens(stream);
}
}
}

pub fn generate_transitive_from_impls(&self, stream: &mut TokenStream) {
let root = &self.enums[0].ident;

for category in &self.tree[root] {
for node in &self.tree[category] {
if let Some(leaves) = self.tree.get(node) {
// From<KeywordEnum> for NodeEnum
self.emit_from_impl([node, category, root], true, stream);

for leaf in leaves {
// From<KeywordTime> for LeafEnum
self.emit_from_impl([leaf, node, category], false, stream);
// From<KeywordTime> for NodeEnum
self.emit_from_impl([leaf, category, root], false, stream);
}
} else {
// From<Argument> for NodeEnum>
self.emit_from_impl([node, category, root], false, stream);
}
}
}
}

fn emit_from_impl(
&self,
[inner, middle, outer]: [&Ident; 3],
inner_is_already_enum: bool,
stream: &mut TokenStream,
) {
for flavor in FLAVORS {
let inner_ty = if inner_is_already_enum {
flavor.suffix_and_lifetime(inner)
} else {
flavor.ampersand(inner)
};
let middle_ty = flavor.suffix(middle);
let outer_ty = flavor.suffix_and_lifetime(outer);

let lifetime = (flavor != Flavor::Val).then_some(quote!(<'n>));

quote! {
The0x539 marked this conversation as resolved.
Show resolved Hide resolved
impl #lifetime From<#inner_ty> for #outer_ty {
fn from(value: #inner_ty) -> Self {
Self::from(#middle_ty::from(value))
The0x539 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
.to_tokens(stream)
}
}

/// Hackish: An accept() method that takes NodeRef<'a> rather than &'a NodeRef<'_>.
/// Visitors that actually use the lifetime, such as Traversal, need this behavior,
/// because if you have an &'a NodeRef<'b>, then 'b is your actual borrow from the AST.
pub fn generate_inherent_accept_method(&self, stream: &mut TokenStream) {
for definition in self.enums {
let ty = Flavor::Ref.suffix(&definition.ident);
let variants = definition.variants.iter().map(|v| &v.ident);
quote! {
impl<'n> #ty<'n> {
pub fn accept(self, visitor: &mut impl NodeVisitor<'n>, reverse: bool) {
match self {
#(Self::#variants(x) => x.accept(visitor, reverse)),*
}
}
}
}
.to_tokens(stream)
}
}
}

#[derive(Copy, Clone, PartialEq)]
enum Flavor {
The0x539 marked this conversation as resolved.
Show resolved Hide resolved
Val,
Ref,
Mut,
Copy link
Member

Choose a reason for hiding this comment

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

isn't this usually called RefMut? Maybe use Value instead of Val then.

Copy link
Contributor Author

@The0x539 The0x539 Mar 23, 2024

Choose a reason for hiding this comment

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

The Flavor enum's days are numbered anyway: we only use NodeEnum for the AST root, and we only use NodeRefMut for parsing. In both cases, it's private to the ast module and I've already described a refactor that would eliminate the enum.

}

const FLAVORS: [Flavor; 3] = [Flavor::Val, Flavor::Ref, Flavor::Mut];

impl Flavor {
fn transform(self, def: &ItemEnum, is_enum: impl Fn(&Ident) -> bool) -> ItemEnum {
let mut def = def.clone();

def.ident = self.suffix(&def.ident);
if self != Self::Val {
def.generics = parse_quote!(<'n>);
}

for variant in &mut def.variants {
let name = &variant.ident;
let field_type = if is_enum(name) {
self.suffix_and_lifetime(name)
} else {
self.ampersand(name)
};
variant.fields = Fields::Unnamed(parse_quote! { (#field_type) });
}

// Hackish: We want to explicitly write the mut-specific #[enum_dispatch] attributes,
// but they can't be applied to the Ref flavor. The Ref flavor also gets Copy+Clone.
if self == Self::Ref {
fn mentions_mut(attr: &syn::Attribute) -> bool {
let parser = Punctuated::<Ident, Token![,]>::parse_separated_nonempty;
let Ok(args) = attr.parse_args_with(parser) else {
return false;
};
args.iter().any(|arg| arg.to_string().contains("Mut"))
}

def.attrs.retain(|attr| !mentions_mut(attr));
def.attrs.push(parse_quote!(#[derive(Copy, Clone)]))
}

def
}

fn suffix(self, name: &Ident) -> Ident {
match self {
Self::Val => format_ident!("{name}Enum"),
Self::Ref => format_ident!("{name}Ref"),
Self::Mut => format_ident!("{name}RefMut"),
}
}

fn suffix_and_lifetime(self, name: &Ident) -> syn::Type {
let name = self.suffix(name);
match self {
Self::Val => parse_quote!(#name),
Self::Ref | Self::Mut => parse_quote!(#name<'n>),
}
}

fn ampersand(self, name: &Ident) -> syn::Type {
match self {
Self::Val => parse_quote!(#name),
Self::Ref => parse_quote!(&'n #name),
Self::Mut => parse_quote!(&'n mut #name),
}
}
}
108 changes: 108 additions & 0 deletions macros/src/blanket_ref_impl.rs
@@ -0,0 +1,108 @@
use proc_macro2::TokenStream;
use quote::{quote, ToTokens};
use syn::spanned::Spanned;
use syn::{parse_quote, parse_quote_spanned};
use syn::{Block, Ident, ImplItem, ImplItemFn, ItemTrait, Pat, Signature, TraitItem, Visibility};

pub fn emit_trait_impl(trait_def: &ItemTrait, mutable: bool, stream: &mut TokenStream) {
let name = &trait_def.ident;
let impl_items = make_impl_body(&trait_def.items, mutable);
let implementor = if mutable { quote!(&mut T) } else { quote!(&T) };
quote! {
impl<T: #name> #name for #implementor {
#(#impl_items)*
}
}
.to_tokens(stream);
}

/// Input:
/// ```
/// type Foo;
/// fn bar(&self, baz: Qux) -> Quux;
/// ```
///
/// Output:
/// ```
/// type Foo = T::Foo;
/// fn bar(&self, baz: Qux) -> Quux { T::bar(self, baz) }
/// ```
fn make_impl_body(trait_items: &[TraitItem], mutable: bool) -> Vec<ImplItem> {
let mut impl_items = vec![];

for item in trait_items {
let trait_fn = match item {
TraitItem::Fn(f) => f,
TraitItem::Type(associated_type) => {
let name = &associated_type.ident;
impl_items.push(parse_quote! {
type #name = T::#name;
});
continue;
}
_ => continue,
};

let block = make_fn_body(&trait_fn.sig, mutable);

// Copy the signature from the trait's definition.
// Discard attributes and the default body.
let impl_fn = ImplItemFn {
attrs: vec![],
vis: Visibility::Inherited,
defaultness: None,
sig: trait_fn.sig.clone(),
block,
};
impl_items.push(impl_fn.into());
}

impl_items
}

/// Input:
/// ```
/// fn bar(&self, baz: Qux) -> Quux
/// ```
///
/// Output:
/// ```
/// { T::bar(self, baz) }
Copy link
Member

Choose a reason for hiding this comment

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

the input/output comments are helpful, maybe add them elsewhere too?
Maybe a high level one for blanket_ref, etc.
Perhaps use "realistic" names that sound like real AST nodes, so it's easier to categorize

AFAICT, T is the concrete node here so we implement all traits for &IfStatement and &mut IfStatement.
The mutable variant is questionable because we never need that outside the populator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, the two traits we actually need the #[blanket_mut] attribute for are quite simple.
However, to implement that, we still need e.g., impl<T: Node> Node for &mut T, which isn't so trivial.
As long as we need to keep the mutable flavor around, we might as well have the attribute for 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.

Doc comment added to top level.

/// ```
fn make_fn_body(sig: &Signature, mutable: bool) -> Block {
let mut args = vec![];
for param in &sig.inputs {
let arg = match param {
syn::FnArg::Receiver(r) if r.mutability.is_some() && !mutable => {
// Hackish: This trait method needs &mut self,
// but we're currently generating the &T flavor of blanket impl,
// so we're unable to call the original method.
//
// Just emit a dummy body, because this method isn't expected
// to be called for this implementor.
//
// TODO: Split up the traits that need this (Leaf, Token, Keyword)
// so that this case can become a hard error.
return parse_quote!({ unimplemented!() });
}

syn::FnArg::Receiver(r) => Ident::from(r.self_token),
syn::FnArg::Typed(pat) => {
let Pat::Ident(pat) = &*pat.pat else {
// Reject any pattern other than just an identifier
return parse_quote_spanned! {pat.span()=>
{
compile_error!("expected a simple identifier");
loop {}
}
};
};
pat.ident.clone()
}
};
args.push(arg)
}

let name = &sig.ident;
parse_quote!({ T::#name(#(#args),*) })
}