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

Don't eat proxy trait #225

Open
zeenix opened this issue Dec 6, 2021 · 12 comments
Open

Don't eat proxy trait #225

zeenix opened this issue Dec 6, 2021 · 12 comments
Labels
api break enhancement New feature or request

Comments

@zeenix
Copy link
Contributor

zeenix commented Dec 6, 2021

In GitLab by @grodin on Dec 6, 2021, 23:46

It wasn't clear that when you write

#[dbus_proxy]
trait SomeTrait {

}

that the struct that is generated for you has all the functions and methods of the trait, but doesn't actually implement it, and in fact effectively replaces it.

It was confusing the heck out of me that I couldn't write a function

fn foo<T: SomeTrait>(t: T) {}

without the compiler complaining that the trait wasn't in scope. It was only after I used cargo-expand to check what was actually generated that it became clear that the scope was "replaced" by the struct.

It would be great if the generated struct actually did implement the trait, and the trait remained, but not knowing much about macros, I've no idea if this is possible.

@zeenix
Copy link
Contributor Author

zeenix commented Dec 6, 2021

Thanks for reporting this. I think it'd be indeed better if we didn't replace the trait but rather implemented it. At least that's what we should try first and if (for any reason) it doesn't work, we can document it better.

We're pretty short on developer time these days so if you could give it a shot, that would help in making this happen sooner.

@zeenix
Copy link
Contributor Author

zeenix commented Jan 25, 2023

Adding "API break" tag cause keeping the source trait around could easily conflict with other symbols in the module (e.g the trait impl via dbus_interface macro).

@zeenix zeenix changed the title Document that the trait that is proxied is no longer accessible Don't eat dbus_proxy trait Jul 28, 2023
@zeenix
Copy link
Contributor Author

zeenix commented Jul 28, 2023

This would be a pre-requisite for #236.

@zeenix
Copy link
Contributor Author

zeenix commented Aug 31, 2023

This would be a pre-requisite for #236.

Actually, not necessarily. dbus_interface can generate the dbus_proxy for you. That way you've far less typing involved and this can be done w/o breaking API though introduction of new macro attribute(s).

@TTWNO
Copy link
Contributor

TTWNO commented Oct 3, 2024

This is actually exactly what we did with atspi a couple years ago and I removed all that code because it was such a hassle to maintain the macro... specifically for the original reason stated, using the trait as a generic.

@zeenix
Copy link
Contributor Author

zeenix commented Oct 3, 2024

@TTWNO I'm not sure I follow. Are you referring to what's stated in the description or my last comment?

@TTWNO
Copy link
Contributor

TTWNO commented Oct 11, 2024

The initial description. Having the trait passed through the macro would be very useful!

@zeenix
Copy link
Contributor Author

zeenix commented Oct 11, 2024

The initial description. Having the trait passed through the macro would be very useful!

Thanks. So interface generating the proxy doesn't help there I'm guessing?

I'm just wondering how you handle this right with interface-generated proxy. It certainly complicates that.

Perhaps there should be a new attribute to disable keeping the proxy and interface sets this by default. 🤔

@zeenix
Copy link
Contributor Author

zeenix commented Oct 13, 2024

There is a problem that I'm not sure how to solve here: The signal methods are not actually implemented by the proxy macro but rather API is generated for receiving these signals. IOW, the generated impl doesn't exactly implemented the trait user provides.

Until I (or someone else) can find a good solution to this, I'm not sure this can be done. So I'm removing this from 5.0 milestone.

@zeenix zeenix removed this from the zbus-5.0 milestone Oct 13, 2024
@TTWNO
Copy link
Contributor

TTWNO commented Oct 13, 2024

I'm just wondering how you handle this right with interface-generated proxy. It certainly complicates that.

This was some time ago, but I removed all the code that tried to use that feature. I ended up just adding a bunch of separate traits and implementing them on the proxies when necessary.

@bachp
Copy link

bachp commented Jan 7, 2025

There is a problem that I'm not sure how to solve here: The signal methods are not actually implemented by the proxy macro but rather API is generated for receiving these signals. IOW, the generated impl doesn't exactly implemented the trait user provides.

Until I (or someone else) can find a good solution to this, I'm not sure this can be done. So I'm removing this from 5.0 milestone.

Would a possible solution be to expose the trait not exactly as written, but enhanced by the macro? This way the trait would cover all methods of the generated proxy, while still allowing alternative implementations for example for mocking or testing.

@zeenix
Copy link
Contributor Author

zeenix commented Jan 7, 2025

Would a possible solution be to expose the trait not exactly as written, but enhanced by the macro?

Hmm.. sounds strange though and we might end up with the same issue as this was originally created for: Users expecting their input to be all there but then finding out that the signals are missing.

while still allowing alternative implementations for example for mocking or testing.

You don't really need to mock the proxy, given that we allow creation of in-process socket pairs now, which you can use to put a (mock/test) service on one side and a real proxy on the other.


Btw, since you're active here again, any chance you'll be finishing your old PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api break enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants