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

Remove atspi-macros #96

Closed
TTWNO opened this issue Jun 19, 2023 · 3 comments · Fixed by #102
Closed

Remove atspi-macros #96

TTWNO opened this issue Jun 19, 2023 · 3 comments · Fixed by #102

Comments

@TTWNO
Copy link
Member

TTWNO commented Jun 19, 2023

The atspi-macros crate uses a bunch of very manual tricks to make a matching trait for each zbus proxy defined in atspi-proxies.
The maintained of such a crate will be terrible for the long-term stability of atspi, especially if the infrastructure in at-spi2-core actually gets reasonably-sized changes.

This will be somewhat involved, but I should be able to simply extract what the macro produces, and use that code directly instead of allowing it to be auto-generated.
I'm not sure how to enforce that the trait and structure match eachother, and manually implementing the trait for each proxy introduces a third place that the (more-or-less) same function definition will be defined.

If we can find a way to check at compile time that the trait has all methods that the proxy has, that would be sufficient for me.

Also, this functionality is only used inside the atspi-client crate, which defines some simplicity functions on top of these macro-generated traits, but it's not actually used in Odilia once the latest PR gets merged, so we don't necessarily have to re-create the traits at all if we decide that atspi-client isn't necessary.

Any ideas on this?

@luukvanderduim
Copy link
Collaborator

Instead of thinking, I decided to ask CoPilot. This being GitHub, it seems fitting. This is what it said:

One approach to ensure that the trait and structure match each other is to use a procedural macro to generate the trait and structure definitions. This way, you can define the structure and trait in a single macro invocation, and ensure that they match each other.

To check at compile-time that the trait has all methods that the proxy has, you can use the quote crate to generate a list of method names for the proxy, and then compare it to a list of method names for the trait. Here's an example of how to do this:

use quote::quote;

// This function generates a list of method names for a given type.
fn get_method_names<T>() -> Vec<String> {
    let mut method_names = Vec::new();
    let type_info = std::any::type_name::<T>();
    let trait_methods = quote! {
        trait Trait {
            #(#type_info::#method_names;)*
        }
    };
    // Parse the trait definition and extract the method names.
    // ...
    method_names
}

// Example usage:
let proxy_methods = get_method_names::<MyProxy>();
let trait_methods = get_method_names::<MyTrait>();
assert_eq!(proxy_methods, trait_methods);

In this example, the get_method_names function generates a list of method names for a given type by parsing the trait definition using the quote crate. You can then compare the method names for the proxy and trait to ensure that they match.

As for whether or not to recreate the traits, it depends on whether or not the atspi-client crate is necessary for your project. If it's not necessary, then you can simply remove the macro-generated traits and use the proxy objects directly. If it is necessary, then you can use the approach I described above to generate the traits and ensure that they match the proxy objects.

@luukvanderduim
Copy link
Collaborator

Luuk: "Cannot we do something similar without proc macros, for instance using static_assert or build.rs script?"
Pilot:
Yes, you can use static_assert or a build.rs script to generate compile-time checks for the trait and structure definitions. Here's an example of how to use static_assert:

use static_assertions::assert_impl_all;

// Define the trait and structure.
trait MyTrait {
    fn foo(&self);
    fn bar(&self);
}

struct MyStruct;

impl MyTrait for MyStruct {
    fn foo(&self) {}
    fn bar(&self) {}
}

// Check that the structure implements the trait.
assert_impl_all!(MyStruct: MyTrait);

In this example, the assert_impl_all macro from the static_assertions crate is used to check that the MyStruct type implements the MyTrait trait. If the check fails, the compiler will produce an error.

You can use a similar approach with a build.rs script to generate the trait and structure definitions and check that they match each other. The build.rs script can generate the trait and structure definitions using a procedural macro, and then use assert_impl_all to check that the structure implements the trait.

@luukvanderduim
Copy link
Collaborator

L: But is the current approach with atspi-macros that bad?

CP: The current approach with atspi-macros is not necessarily bad, but it does have some drawbacks.

One potential issue is that the macro-generated code can be difficult to maintain, especially if the infrastructure in at-spi2-core undergoes significant changes. This can make it difficult to keep the macro-generated code up-to-date and ensure that it remains compatible with the rest of the codebase.

Another issue is that manually implementing the trait for each proxy introduces a third place where the same function definition will be defined. This can make it difficult to ensure that the trait and proxy implementations remain in sync, especially if changes are made to the proxy implementations.

Using a procedural macro to generate the trait and structure definitions can help to address these issues by ensuring that the trait and proxy implementations remain in sync and reducing the amount of manual code that needs to be written. However, this approach can also introduce its own set of challenges, such as ensuring that the generated code is correct and maintaining the procedural macro itself.

Ultimately, the best approach will depend on the specific needs of the project and the trade-offs between maintainability, complexity, and performance.

@TTWNO TTWNO linked a pull request Jul 2, 2023 that will close this issue
@TTWNO TTWNO closed this as completed in #102 Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants