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

WIP: Article for how to deal with IDL field name collisions. #172

Open
wants to merge 5 commits into
base: gh-pages
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 79 additions & 2 deletions articles/110_interface_definition.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,88 @@ Both file names must use an upper camel case name and only consist of alphanumer
Field names must be lowercase alphanumeric characters with underscores for separating words.
They must start with an alphabetic character, they must not end with an underscore and never have two consecutive underscores.

### Naming of constants
#### Naming Conflicts

Constant names must be uppercase alphanumeric characters with underscores for separating words.
When defining field names it has been recommended to avoid language keywords and other reserved symbols.
However since we cannot be guaranteed to know all conflicts of potential future languages or implementations we must have a way to systematically deal with these conflicts without disrupting existing code bases and implementations.
To that end we will require that language bindings provide a reproducable name mangling mapping that will prevent conflicts with reserved keywords or other conflicts.

##### Background

This has come up specifically as we're adding support for Windows for ROS2.
`winnt.h` defines several macros that conflict with existing enumerations.
It is also expected to happen as support for new languages are added.
We cannot know all the potential future keywords and restrictions from a language which is selected to add support for in the future so we must have a generic solution which will allow future languages to be added without disrupting the existing usages.

##### Language Generator Requirements

In the case that a new language is added and there is a detected conflicting name for a field, the language specific generator is expected to implement a basic deconfliction mangling of the symbol to avoid the conflict.

Each generator will define the mangling procedure and provide a list of symbols for which the procedure will be applied.
This list should be all known keywords or otherwise conflicting symbols.

##### Example Deconfliction Policy

A example of a simple name mangling policy would be to append a trailing underscore for conflicting symbols.
So that would mean that `FOO` if declared as a conflicting symbol would be used in generated code as `FOO_`.

Clearly this would collide if someone defined `class` and `class_`, so slightly more unique string is recommended for clarity and uniqueness.
The mangling should be designed so that accidental collisions between names and mangled names cannot happen.
One easy way to do this would be to a language specific valid character not valid in the standard field name specification.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common approach - which doesn't rely on an extra character - is to apply the mangling in in both cases. So if a user defined FOO and FOO_ both are getting mangled to FOO_ and FOO__.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't applying the name mangling in both cases would mean that users would always have to access fields by mangled names? e.g. in C++, msg->class_, as needed, but also msg->data_, which is undesired (but at least it's consistent). I know that originally the mangling topic was brought up with a focus on enumerations (where perhaps this comment doesn't apply), but this document seems to be about fields in general.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just pointing out that the described key-value mapping in the current patch isn't sufficient. Not specific to fields or constants.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current state of ros2 idl restrictions prevents fields from being defined with a trailing underscore. If we aren't planning to relax that (I think it wasn't imposed in ros1), then perhaps a trailing underscore can always used for mangling, without any potential for accidental collisions?



##### Table of deconfliction

The language support package will provide a yaml file with a list of all the protected keywords and their mangled version in a dictionary as key and value respectively.

For example if the language had conflicts for `FOO`, `new`, and `delete` it's yaml file would like like this:

```
FOO: FOO_
new: new_
delete: delete_
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the comment above the table will not define the "only" keys which need to be mangled. "Derived" variations of the key (e.g. the mangle name) need to be considered and mangle too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the mangling strategy is producing symbols that are not allowed in the msg spec (like adding a trailing underscore as pointed out in https://github.com/ros2/design/pull/172/files#r194139732) we could get away without having to consider the mangled names as we would not have conflicts between use defined fields and mangles names by design


This yaml file will be considered the authority for deconflicting and should be exported in a way that will allow downstream code generators to automatically apply the deconfliction policy.

##### Stability
Each language generator will provide a standard name deconfliction policy so that users can use the symbols in a stable manner.

This table will remain stable so that codebases can rely on using the mangled symbols.
Appending to the table may be necessary if the first pass did not catch all the conflicting keywords.

The removal of any protected keywords should be avoided if at all possible as it will require all code using that field or constant to be updated.

##### Legacy Support

There are a number of known conflicts with the existing ROS1 codebase.
In these few cases we will undefine the matching C/C++ preprocessor macros which are conflicting in the definition and define the old value.
This will be done only inside the declaration header and the definition will be restored before returning to the user's code.
If you are using the legacy defines it's required to use the namespaced reference instead of using a shorthand version as those are know to conflict.

For an easy migration path.
The mangled version of the constants will be made available in ROS Melodic by manually defining them in parallel so that code can be ported and use the same constant values while the codebase is spanning ROS1 and ROS2 implementations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this work that has been started already or something we will do at some point during Melodic lifetime ?


It is recommended to move to the deconflicted version immediately.

Constants known to be at issue:

* `NO_ERROR`
* `DELETE`
* `ERROR`

#### Naming Constants

Constant names must be uppercase alphanumeric characters with underscores for separating words except the first character which should be a lowercase k.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this referring to the naming in the IDL or to the generated C++ code? If it is the former I would argue that we shouldn't put this constrain into the IDL itself. In some languages ERROR is just fine. Therefore I would leave such a convention to the language specific code generator.

They must start with an alphabetic character, they must not end with an underscore and never have two consecutive underscores.

To avoid conflicts generally all constants should be prefixed with a `k`.
This will avoid almost all conflicts in all languages.
This is designed to follow the Google Style Guide: [https://google.github.io/styleguide/cppguide.html#Constant_Names](https://google.github.io/styleguide/cppguide.html#Constant_Names)

This should be used for newly defined constants.
When migrating messages from ROS1 if there are existing constants in use the new version should be defined side by side with the old name to provide backwards compatibility for a full deprecation cycle.

## Syntax

The message and service definitions are text files.
Expand Down
2 changes: 1 addition & 1 deletion test_local_deploy.sh
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
#!/bin/sh

docker run --rm -v `pwd`:/srv/jekyll -i -t -p 127.0.0.1:4000:4000 jekyll/jekyll:pages jekyll serve --watch
docker run --rm -v `pwd`:/srv/jekyll -i -t -p 127.0.0.1:4000:4000 jekyll/jekyll:builder jekyll serve --watch