🐛 zx: Prevent collisions between methods, signals and properties #722

bachp · 2024-04-17T21:23:43Z

Prevent collisons by adding an explicit suffix to properties and signals.

If a collision is detected a _ is added to the identifier until there is no more collision.

Closes:

Name collisions in generated Proxy code when propery getter/setter exists as a method #231
xmlgen: property setter can conflict with target interface provided setter method #169

zbus_xmlgen/src/lib.rs

zeenix

Thanks so much for contributing this PR and also to follow our dev practices from the start. However, I think a good solution here would require more work than changing all generated names.

zeenix

LGTM otherwise.

zeenix · 2024-04-18T21:17:03Z

zbus_xmlgen/src/lib.rs

+            let name = collision_hander
+                .avoid_collision(to_identifier(&to_snakecase(signal.name().as_str())));


I think the 2 steps should be combined into one. We don't really need a separate type for name collision, just one for creating identifiers, that ensures that the identifier isn't already used.

First I also tought this, but the to_identifier is also used in other places, eg. function parameters, and they don't collide with signals and properties. but

I could move the to_identifier function inside the avoid_collision but I thought this was it is more explict

I was just wondering if there is a case where we also have collisions in arguments?

First I also tought this

Ah, you're ahead of me. :)

I could move the to_identifier function inside the avoid_collision but I thought this was it is more explict

Or you could have the new API use to_identifier itself so that caller doesn't need to care. I'd call the type UniqueNameGenerator and method identifier_for_name, which takes the name as arg, also taking care of to_snakecase. You can suggest a better name, just don't like collision in the name (sounds negative 😆).

I was just wondering if there is a case where we also have collisions in arguments?

Doubtful but even if there was, it won't be global (i-e args of different methods can have the same name).

I tried severl variants of combining operations. In the end I found it's best to keep the operations separate so I can combine these in the different cases:

Functions and signals i need to_snakecase, to_identifier and make_unique

For Properties I need to_snakecase, to_identifier, and I need make_unique as a separate operation depending on read or write case.

For arguments I only need to_identifier, optionally we could apply make_unique per function here but I don't see this needed.

So my solution now is to just rename the CollisionHandler to Idenfifier and have a make_unique method.

The way I understand your proposal is to put the rules how to combine these into separate functions/methods and just call them, e.g. to_method,to_signal, to_property and to_argument

At the very least, it should have been clear, which names I proposed. :)

While I can do that I'm not convinced it is a good idea as it basically splits the rules into two locations.

How so? They'll do pretty much the same thing as you're doing right now, just in one place/type. They will all use to_snakecase and to_identifier, for example.

Let me know, if you'd want me to make the proposed changes in a separate commit on top.

I did a test with these changes and the NetworkManager dbus xml, and it doesn't solve all the conflicts. I need to dig a bit deeper. I think I will add a test case using the NetworkManager interface description to make the issue more reproducible.

@bachp this is the part that is still not resolved. You never answered my last question here. I know it's been many months since so I don't blame you for not remembering but let's try to conclude this only remaining issue,

zeenix · 2024-06-15T11:39:07Z

@bachp oh I'm so sorry. I never got a notification that you requested a review after updating the pr. I'll look into this very soon.

Thanks for your patience. 🙏

bachp · 2024-06-15T20:55:18Z

@zeenix As mentioned in #722 (comment) my change didn't completely solve the issue with the networkmanager dbus specification. Unfortunately I was busy with other things.

I want to add a test based on the real world example, not sure when I will get to it tough.

zeenix · 2024-07-11T21:44:05Z

@bachp i hope you've not forgotten about your existing PR. 😉

bachp · 2024-07-11T22:05:02Z

@zeenix No I did not but I ran into other issues while trying to interact with network manager, mainly #312 which is more of a blocker for me.I will come back to this once I found a solution for the more blocking issues.

zeenix

GH thinks this is a pending review from me so let me fix that. 😊 Also, it would be great if you could finish this soon. 🙏

If a collision is detected and additional `_` is appended to the identifier until there is no more collision. Signed-off-by: Pascal Bach <pascal.bach@nextrem.ch>

bachp · 2024-08-17T17:06:15Z

@zeenix Not sure what is still open and why GH thinks there is a change required. From my point of view this is ready, I also rebased it on main.

It doesn't fix all naming collisions I encountered with NetworkManager, but I can address the remaining issues in a sparate PR.

zeenix · 2024-08-17T19:48:40Z

@zeenix Not sure what is still open

Oh? I understood from your previous two comments here that it's not ready and you've to fix something still.

why GH thinks there is a change required. From my point of view this is ready

Because there is still a conversation that's not marked as "resolved" yet.

It doesn't fix all naming collisions I encountered with NetworkManager, but I can address the remaining issues in a sparate PR.

For sure. I thought they were a blocker for you and you want to address them all here.

zeenix

Based on the latest discussion here, I wanted to set the status right.

zeenix reviewed Apr 18, 2024

View reviewed changes

zbus_xmlgen/src/lib.rs Outdated Show resolved Hide resolved

zeenix requested changes Apr 18, 2024

View reviewed changes

bachp force-pushed the xmlgen-avoid-collision branch from 7091871 to 1fcce02 Compare April 18, 2024 19:07

bachp requested a review from zeenix April 18, 2024 19:09

zeenix requested changes Apr 18, 2024

View reviewed changes

bachp force-pushed the xmlgen-avoid-collision branch from 1fcce02 to 2383c24 Compare April 20, 2024 06:55

bachp requested a review from zeenix April 20, 2024 07:02

zeenix requested changes Jul 30, 2024

View reviewed changes

bachp requested a review from zeenix August 17, 2024 17:02

🐛 zx: Prevent collisions between methods, signals and properties

acdb5be

If a collision is detected and additional `_` is appended to the identifier until there is no more collision. Signed-off-by: Pascal Bach <pascal.bach@nextrem.ch>

bachp force-pushed the xmlgen-avoid-collision branch from 2383c24 to acdb5be Compare August 17, 2024 17:04

zeenix requested changes Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 zx: Prevent collisions between methods, signals and properties #722

🐛 zx: Prevent collisions between methods, signals and properties #722

bachp commented Apr 17, 2024 •

edited

Loading

zeenix left a comment

zeenix left a comment

zeenix Apr 18, 2024 •

edited

Loading

bachp Apr 19, 2024

bachp Apr 19, 2024

zeenix Apr 19, 2024

bachp Apr 20, 2024

zeenix Apr 22, 2024

zeenix Apr 22, 2024

bachp Apr 26, 2024

zeenix Aug 17, 2024

zeenix Oct 1, 2024

zeenix commented Jun 15, 2024

bachp commented Jun 15, 2024

zeenix commented Jul 11, 2024

bachp commented Jul 11, 2024

zeenix left a comment

bachp commented Aug 17, 2024

zeenix commented Aug 17, 2024

zeenix left a comment

		let name = collision_hander
		.avoid_collision(to_identifier(&to_snakecase(signal.name().as_str())));

🐛 zx: Prevent collisions between methods, signals and properties #722

Are you sure you want to change the base?

🐛 zx: Prevent collisions between methods, signals and properties #722

Conversation

bachp commented Apr 17, 2024 • edited Loading

zeenix left a comment

Choose a reason for hiding this comment

zeenix left a comment

Choose a reason for hiding this comment

zeenix Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeenix commented Jun 15, 2024

bachp commented Jun 15, 2024

zeenix commented Jul 11, 2024

bachp commented Jul 11, 2024

zeenix left a comment

Choose a reason for hiding this comment

bachp commented Aug 17, 2024

zeenix commented Aug 17, 2024

zeenix left a comment

Choose a reason for hiding this comment

bachp commented Apr 17, 2024 •

edited

Loading

zeenix Apr 18, 2024 •

edited

Loading