Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Tue, Feb 11, 2025 at 7:47 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
>> I would suggest to keep the "sysbus" prefix in the method name, or in
>> general, keep the class prefix in the method names in XXXClassMethods
>> traits. Otherwise APIs from different parent classes may also
>> conflict. As an example, in the following class hierarchy:
>>
>>   Object -> DeviceState -> PCIDevice -> PCIBridge -> ...
>>
>> For PCIDevice instances there is an API pci_device_reset(), and for
>> PCIBridge ones pci_bridge_reset(). Without class prefixes both will be
>> wrapped (as blanket impl in two traits) as reset() and a dev.reset()
>> call will lead to a "multiple applicable items in scope" build error.
>
> Yes, reset is definitely a problem.
>
> I will wrap qdev_realize() in DeviceMethods to check if you get
> "multiple applicable items" from that as well.
>
> I can also go back and add "sysbus_" in front of init_mmio, init_irq,
> etc.; but on the other hand we have Timer::{modify, delete} and
> DeviceMethods::init_{clock,qdev}_{in,out}, should they be changed as
> well? I'm not sure there is a single solution that always works.
>

The DeviceMethods::init_* wrappers may need a similar prefix for the
same reason, though it seems unlikely to suffer from such name
conflicts. Meanwhile, adding prefixes to timer::* seems redundant.

Essentially we want a naming convention that (1) avoids any potential
name conflicts, and (2) applies consistently to (ideally) all APIs. For
goal (1), we need something at API call sites that can resolve the
potential ambiguity. So instead of dev.realize(), we may write:

  a) dev.sysbus_realize()

  b) SysBusDeviceMethods::realize(&dev); dev.realize() is still ok if
     there is no ambiguity

  c) dev.as_ref::<SysBusDevice>().realize()

  (any more options?)

None looks perfect, unfortunately. Option (a) introduces inconsistent
naming conventions as mentioned earlier; (b) cannot prevent confusions
when a device has both a "reset()" method and "dev.reset()" calls; (c)
deviates from how wrappers are auto-delegated to child classes today and
is the longest to write.

Option (b) + a lint that warns same method names looks a good tradeoff
to me. I tried to find some clippy lints for that purpose, but have not
found any yet. A similar issue exists [1], but has been kept open for >6
years and is not exactly what we want.

[1] https://github.com/rust-lang/rust-clippy/issues/3117

--
Best Regards
Junjie Mao

>> I agree that uses of proc macros should be carefully evaluated to see if
>> they really help or hurt. In this specific case, I'm not sure if using
>> attributes solves the root cause, though.
>
> Yes, it doesn't help if you have multiple similarly-named methods
> across the "superclasses".
>
> Paolo