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

zbus: Use typed associated constants for ProxyDefault #482

Closed
wants to merge 2 commits into from
Closed

zbus: Use typed associated constants for ProxyDefault #482

wants to merge 2 commits into from

Conversation

bilelmoussaoui
Copy link
Contributor

@bilelmoussaoui bilelmoussaoui commented Sep 28, 2023

This is a breaking API change, so it makes sense for the upcoming zbus release

Will be used in the next commit in ProxyDefault and so in the dbus_proxy macro
As zbus_names::InterfaceName/zbus_names::BusName/zvariant::ObjectPath are much clearer
then a &'static str
@bilelmoussaoui
Copy link
Contributor Author

Actually, my changes are not that great. As clippy complains about a "const" item should never be interior mutable and we can't switch to using a static instead of a const as we can't have associated statics so far in rust. The only remaining option is to make T::from_static_str for ObjectPath/InterfaceName/BusName const

@zeenix
Copy link
Contributor

zeenix commented Sep 28, 2023

The only remaining option is to make T::from_static_str for ObjectPath/InterfaceName/BusName const

That sounds like a good change anyway?

@bilelmoussaoui
Copy link
Contributor Author

It is yes but involves no longer going through TryFrom as that can't be const for obvious reasons

@zeenix
Copy link
Contributor

zeenix commented Sep 28, 2023

It is yes but involves no longer going through TryFrom as that can't be const for obvious reasons

That's fine. We just need to ensure the string is checked for validity.

@zeenix
Copy link
Contributor

zeenix commented Oct 20, 2023

@bilelmoussaoui what's the update on this?

@TTWNO
Copy link
Contributor

TTWNO commented Nov 9, 2023

Since serde::de::Error:* are not const, you would need to stop using those errors for conversion into zvariant::Error. All serde errors are put into the zvariant::Message variant anyways, so perhaps it isn't that big of a deal to write custom error messages.

EDIT: Ooooo. .to_string() and format! are both non-const as well. Could be a non-trivial overhall, unless simply returning a bool would be considered acceptable instead of giving a proper error message.

@zeenix
Copy link
Contributor

zeenix commented Nov 9, 2023

Since serde::de::Error:* are not const, you would need to stop using those errors for conversion into zvariant::Error. All serde errors are put into the zvariant::Message variant anyways, so perhaps it isn't that big of a deal to write custom error messages.

I'm a bit confused. AFAIK there are no errors involved here. 🤔

@TTWNO
Copy link
Contributor

TTWNO commented Nov 9, 2023

In order to make from_static_str const we must make the checker function const. That checket function currently calls non-const functions from the serde:::de:Error trait. But, even if you try to remove that, and wtite the error messages yourself (in this case into zvariant::Error::Message(...), you van't do that either because the Message variant accepts a string, which is not possible to create a non-empty one of in const.

This is specifically related to ObjectPath, but I suspect it applies elsewhere.

@zeenix
Copy link
Contributor

zeenix commented Nov 9, 2023

Thanks for explaining. My memory of this PR's details was a bit foggy.

That checket function currently calls non-const functions from the serde:::de:Error trait. But, even if you try to remove that

I think it shouldn't be using serde::de::Error at all. We should have our own variants used there in any case (regardless of this PR).

, and wtite the error messages yourself (in this case into zvariant::Error::Message(...), you van't do that either because the Message variant accepts a string, which is not possible to create a non-empty one of in const.

We need dedicated variants then and they can avoid String.

@TTWNO
Copy link
Contributor

TTWNO commented Nov 9, 2023

We need dedicated variants then and they can avoid String.

I understand. I'll give this one a go once I'm done with #496 ... and I promise I'll read the contributors guide first this time ;)

@zeenix
Copy link
Contributor

zeenix commented Nov 9, 2023

I understand. I'll give this one a go once I'm done with #496 ... and I promise I'll read the contributors guide first this time ;)

Awesome! I wish most people had your positive attitude towards my perfectionism. :)

@TTWNO
Copy link
Contributor

TTWNO commented Nov 11, 2023

New discovery: since zvariant::Error cannot be dropped in const contexts, we'll have to create a second error enum that can be dropped in const contexts, and then manually convert it into zvariant::Error (since From::from is not const either).

Are you okay with that?

@MaxVerevkin
Copy link
Contributor

and then manually convert it into zvariant::Error

Maybe zvariant::Error can have a variant which contains this new enum?

@zeenix
Copy link
Contributor

zeenix commented Nov 11, 2023

Are you okay with that?

I guess that's the only option so yeah.

Maybe zvariant::Error can have a variant which contains this new enum?

Sure. We can do something like zvariant::Error::MaxDepthExceeded variant. However, can you please explain a bit why that new struct will be ok to drop in const contexts but not zvariant::Error itself? 🤔

@TTWNO
Copy link
Contributor

TTWNO commented Nov 11, 2023

However, can you please explain a bit why that new struct will be ok to drop in const contexts but not zvariant::Error itself?

String, and Arc<_> can both not be dropped in const contexts.

@zeenix
Copy link
Contributor

zeenix commented Nov 23, 2023

@bilelmoussaoui I'm assuming you won't be finishing this anytime soon. @TTWNO if you want to do this, you'll need to create a new PR anyway so I'm closing this.

@zeenix zeenix closed this Nov 23, 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 this pull request may close these issues.

4 participants