moveit2
moveit2 copied to clipboard
ros-planning
Interfaces Use Strong Types for Parameters
Need
As a developer using MoveIt; I want MoveIt to have expressive interfaces that can only be used correctly.
Acceptance
A PR is merged into moveit_core that introduces the concept of strong types for interfaces.
Notes
https://www.fluentcpp.com/2016/12/08/strong-types-for-strong-interfaces/
Somewhat off-topic, but: the linked page demonstrates a nice idea, but I'm wondering whether he isn't confusing type with role.
A width is not a type in my opinion. It's a regular double which is used in a certain context to represent a certain quantity with certain semantics. It's still a number, but it plays a different role as a double used to represent a height. Is that second double of a different class of numbers?
The fact we need to redeclare/reimplement all the regular mathematical operators and cannot easily use a Width as a regular number any more (which it basically is) seems to indicate something is not right with this approach. Enabling implicit conversion (as discussed in the comments) is one way to deal with this, but that seems to negate the main point of using the strong types (ie: it's semantically and syntactically clear what type is being used, and the compiler can check it for us).
Using user-defined literals to assign units to everything is something I see much more value in, and doesn't try to use typing to capture roles.
@tylerjw: what use of strong types do you envision in MoveIt? Perhaps an example could help potential contributors.
Edit:
The fact we need to redeclare/reimplement all the regular mathematical operators and cannot easily use a
Widthas a regular number any more (which it basically is) seems to indicate something is not right with this approach.
I realise this may be a limitation of the type system in C++ btw.
Perhaps the concept system would be better suited to capture these kind of things?
Perhaps the concept system would be better suited to capture these kind of things?
Concepts are amazing, but it will probably be years before we can use them as we'll need ROS to switch to C++20 before we can. I also think they solve a different problem.
I'll try to find an example of where I think this could be useful in MoveIt to help explain this idea. On a client project I was just on I used it for file descriptors and it made an interface much clearer than if it was just an int.
In the case of the file descriptor I was able to then give it a default value of -1 that is invalid instead of 0 (stdout) and one of my objects took a file descriptor as a constructor parameter which means that to call it the user has to explicitly construct this type so there is no explicit type conversation from int to this type that takes a file descriptor. Lastly I implement a constexpr conversation to int so inside my class that used it the logic wasn't changed. The point of the strong type in the case of my file descriptor was just to make initialization of it and interfaces that took one much more explicit.
Perhaps the example in the linked article is just wrong.
A file descriptor is not an int. It's a wholly different concept, which just happens to be represented by an int. But the int is really an index into a table/list of entities managed by the OS, and userland programs are given out those ints to be able to refer to the entities in that list, instead of having to lug around the entities themselves (or objects if you will).
In that sense, the int used as a file descriptor is a reference or pointer.
A width is not something which is conceptually similar.
A width stays a number, is reasoned about as a number, can be used in mathematical expressions as a number and that all makes sense.
No one tries to add two file descriptors together, as that would not make sense.
But it can make sense to want to do: width + height + width + height (for instance to calculate a circumference). If Width were an actual (strong) type, I'd have to either rely on implicit conversion, or overload the + to allow adding Width to Height (or, which is what the article does in the beginning: my_width.get(), which is both ugly and also intrusive).
That doesn't scale, and to me implies using types for this is not the right way to go about it.
Something is a role if an entity can, without changing its type, fulfil a different purpose, or be recognised as a different entity. In the above, if I want to use a Width as a Length, I can only do that if Width <- Length, or if I explicitly construct a Width{Length::get()} (pseudo). That leads to very large inheritance trees, which also typically result in conflicts (or require diamond -- or worse -- inheritance).
Note: I'm not saying there is no use for this idea, I just wanted to comment on it (which is why I began my comment with "somewhat off-topic").
I agree that width and height are maybe bad examples. For whatever reason if you Google strong types it seems to be the go-to example everyone uses. To reduce a ton of boilerplate you would need for strong types there is this library by the author of that blog: https://github.com/joboccara/NamedType
In the case of my file descriptor it made sense to me because it is a very different thing from an int, it just happens to be represented as an int in the language. As you said it makes no sense to multiply a file descriptor or really to do any math operations on it other than comparing it and checking the sign to see if it is valid.
I'll look for examples in MoveIt where this would improve the readability of the interfaces as that is the clear benefit I see to using strong types.
An example that might make sense (I'd need to try to make the change somewhere to understand if it would) would be velocity and position as strong types. There are cases where the two intact in math operations but it is not the full set of operations that can be done on two doubles. When they do interact they have a specific type that results.
For example it makes no sense to add a position to a velocity so maybe it would make sense to build types that would allow the compiler catch that.
Also, then you could make so you could multiply velocity by some time type (maybe chrono) and the result would be a position difference that could be added to a position.
For example it makes no sense to add a position to a velocity so maybe it would make sense to build types that would allow the compiler catch that.
Right, so that's what I was referring to with:
Using user-defined literals to assign units to everything is something I see much more value in, and doesn't try to use typing to capture roles.
Things like boost.units and nholthaus/units support what you describe (see the example in the Getting started guide, where they try to assign m^2 to a variable of type meter_t).
Edit: user-defined literals are of course also types. So again, I'm not against the idea, I just believe the example and the linked article are not the best ways to go about this.
I think I finally understand what you mean by roles. I think we see value in the idea in the same place. I'll go check out the tools you linked.
That units library looks awesome. I'm going to vendor it and just start using it if it isn't in rosdep already. That is basically was I was hoping for and and so much more.
Hi @tylerjw,
I am applying for the GSoC this year and need to include a pull request with my application.
I am happy to start working on a pull request for this issue. I suspect it will take some time to make updates for all interfaces but since I plan to work on python bindings for much of the core functionalities of MoveIt 2 as part of my GSoC project I think this would be a good issue for me to start working on.
Do you agree? If so could you confirm which units library you wish for me to use and which section of the codebase you would prefer for me to get started on?
FYI @henningkayser.
@peterdavidfagan did you already spent time on this? I agree that it's something that could have an effect on the GSoC project, but I'm not sure if we already have a good understanding on what API changes we actually need. I also think it's not really a "good first issue" anymore, so I removed the label. @tylerjw do you have any more thoughts on this?
Hi @henningkayser, I haven't spent much time on this issue yet other than briefly looking into the libraries mentioned above. I am happy to stop working on this in the meantime given the above comment.