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

[Paolo Bonzini]

On Tue, Feb 11, 2025 at 12:34 PM Junjie Mao <junjie.mao@hotmail.com> wrote:
> 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.

I now agree that (1) is more important than (2). Adding a function like

    fn realize(&self, bus: *mut BusState) {
        // TODO: return an Error
        assert!(bql_locked());
        unsafe {
            bindings::qdev_realize(self.as_mut_ptr(),
                bus, addr_of_mut!(bindings::error_fatal));
        }
    }

to DeviceMethods is enough to cause an error:

error[E0034]: multiple applicable items in scope
   --> hw/char/pl011/src/device.rs:714:9
    |
714 |     dev.realize();
    |         ^^^^^^^ multiple `realize` found

> 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.

There is one more, which is a variant of (c): use Deref to delegate to
the superclass, and traits for interfaces only. Then the default would
always be the closest to the class you're defining, and you could
override it with SysBusDevice::realize(&dev).

It requires more glue code, but creating it could be delegated to
#[derive(Object)].

I think we can use (a) as proposed by Zhao and yourself, and document
this convention

(1) whenever a name is unique in the hierarchy, do not add the prefix

(2) whenever a name is not unique in the hierarchy, always add the
prefix to the classes that are lower in the hierarchy; for the top
class, decide on a case by case basis.

For example, you'd have

DeviceMethods::cold_reset()
PciDeviceMethods::pci_device_reset()
PciBridgeMethods::pci_bridge_reset()

PciDeviceMethods adds the prefix because the three methods have
different functionality. Subclasses of PciBridgeMethods may need to
call either pci_device_reset() or pci_bridge_reset().

And also, because there is a similar name in DeviceMethods,
PciDeviceMethods::reset() would be confusing.

(As an aside, pci_device_reset() probably should be implemented at the
Resettable level, e.g. RESET_TYPE_BUS, but that's a different story).

Or perhaps pci_bridge_reset() becomes PciBridgeCap::reset(), which is
not a trait. That's okay too, and it's not a problem for the naming of
pci_device_reset().

but:

DeviceMethods::realize()
SysbusDeviceMethods::sysbus_realize()
PciDeviceMethods::pci_realize()

Here, DeviceMethods does not add the prefix because generally the
lower classes only add casts and compile-time typing but not
functionality. The basic realize() functionality is the same for all
devices.

What about confusion with the realize function on the struct? It's
unlikely to be an issue because it has a different signature than
DeviceMethods::realize(), which takes a BusState too. But if I'm wrong
and there _is_ confusion, renaming DeviceMethods::realize() is easy.

> Just found the lint: 
> https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method

Almost: "It lints if a struct has two methods with the same name: one
from a trait, another not from a trait." - it doesn't check two
traits. Also I think in this case it doesn't fire because the trait is
implemented for &PL011State, not PL011State.

Paolo