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

That convention looks good to me and does keep the naming simple for the
vast majority.

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

I don't think I'm experienced enough to tell if that can confuse device
writers. Perhaps we can keep it for now as renaming is easy with the
support from the language server.

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

I thought that lint can help warn early when a device writer
accidentally named a method in the same way as an API. But as you have
pointed out, it doesn't really help in this case.

I'm still a bit worried about the potential ambiguity between API and
device-defined method names. The compiler keeps silent on that, and it
can eventually cause unexpected control flow at runtime. That said, I'm
not sure how likely it will hit us. We may keep it as is for now and go
extend that lint when we find out later that the ambiguity worths early
warnings.

>
> Paolo

--
Best Regards
Junjie Mao