Re: [PATCH 03/11] rust: macros: convert `#[vtable]` macro to use `syn`

[Gary Guo <gary@garyguo.net>]

On Mon, 5 Jan 2026 10:19:57 -0500
Tamir Duberstein <tamird@gmail.com> wrote:

> On Mon, Jan 5, 2026 at 9:27 AM Gary Guo <gary@garyguo.net> wrote:
> >
> > On Mon, 5 Jan 2026 06:02:35 -0500
> > Tamir Duberstein <tamird@gmail.com> wrote:
> >  
> > > On Sun, Jan 4, 2026 at 9:18 PM Gary Guo <gary@garyguo.net> wrote:  
> > > >
> > > > On Sun, 4 Jan 2026 18:44:43 -0500
> > > > Tamir Duberstein <tamird@gmail.com> wrote:
> > > >  
> > > > > On Thu, Dec 11, 2025 at 2:29 PM Gary Guo <gary@kernel.org> wrote:  
> > > > > >
> > > > > > From: Gary Guo <gary@garyguo.net>
> > > > > >
> > > > > > `#[vtable]` is converted to use syn. This is more robust than the
> > > > > > previous heuristic-based searching of defined methods and functions.
> > > > > >
> > > > > > When doing so, the trait and impl are split into two code paths as the
> > > > > > types are distinct when parsed by `syn`.
> > > > > >
> > > > > > Signed-off-by: Gary Guo <gary@garyguo.net>  
> > > > >
> > > > > Logic looks correct, but the duplication between handle_trait and
> > > > > handle_impl is unfortunate. I golfed on this a bit, see if you like
> > > > > it: https://github.com/tamird/linux/commit/8354c5a48769f5e1e52963d19ca57c31e5926b08.  
> > > >
> > > > I very much prefer the code to be separate. The trait and impl *should*
> > > > be different. It's just that they *look* similar.
> > > >
> > > > Defining
> > > >
> > > >         const HAS_FOO: bool = false;
> > > >
> > > > in trait is defining a new contract while providing *default* value,
> > > > while
> > > >
> > > >         const HAS_FOO: bool = true;
> > > >
> > > > is implementing such contract with a specific value. They look the
> > > > same but I think the way `syn` treats them differently is justified.  
> > >
> > > Yes, the similarity is perhaps superficial, but the duplication in the
> > > current patch goes a good bit further because all the surrounding
> > > ceremony is also duplicated.  
> >
> > What do you mean? The only code that is really duplicated is the gathering
> > of names from items (which have to be duplicate due to type difference
> > anyway, and they're still duplicate in your linked commit above.  
> 
> There's a bunch of boilerplate surrounding this code that is duplicated:
> - declaring `functions` and `consts
> - iterating over `functions`
> - computing the value of `gen_const_name`
> - checking if `gen_const_name` is already in `consts`
> 
> These are of course mostly straightforward, but it is duplication
> nonetheless, and it is all removed in my commit linked above.

I am not convinced.

These are very light-weight lines and it's not like the duplication
contains heavy logic inside. In my opinion the `if` (or `match`) that is
involved in handling them differently but inside a same function is more
cognitive load than just duplicating them. It's much easier to have a
top-level if to split into two code-paths.

Plus, the reason for `gen_const_name` check is different for the code
paths:
- for trait, it's for handling `#[cfg]` functions which are allowed to
  repeat
- for impl, it's for allowing user override plus the `#[cfg]`.

With `syn`, arguable the `#[cfg]` should be handled just by propagating
the `#[cfg]` from function to constant.

(Actually, we don't need to iterate over user-defined constants in the
trait code path at all even without the cfg change. I'll remove it in v2)

Best,
Gary

> 
> >  
> > >  
> > > >
> > > > I think the fact that existing code has a boolean and do different
> > > > things based on it is a good enough supporting reason to handle
> > > > different code path.
> > > >
> > > > For some new `vtable` features that I am working on would require quite
> > > > different impl between the two.  
> > >
> > > It seems unusual to justify current changes with future changes.  
> >
> > I gave both reasons that why I think it should be distinct code path today
> > and also why I prefer it to be it for the future. I did not justify the
> > current change with future changes.  
> 
> The today justification (the presence of a boolean) feels weak to me.
> It can be made an enum if that changes your position.