Allow deprecating fields of interfaces

[InvincibleRMC]

Description

Proposes adding support for annotatoins with the form @deprecated ( text='foo' ).

For now only supports .idl since this shouldn't happen often. This method allows us to add support in the future if we desire.

Fixes ros2/ros2#1679

Note: I can do python also if this methodology is approved.

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

[InvincibleRMC]

Pulls: #945, ros2/rosidl_typesupport_fastrtps#145
Gist: https://gist.githubusercontent.com/InvincibleRMC/8324eb73e25ffbbb22757567c7780d8d/raw/e2297a0c6e2aca964e6406ba63beb6b02477dc3d/ros2.repos
BUILD args: --continue-on-error --packages-above-and-dependencies rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection rosidl_typesupport_fastrtps_cpp
TEST args: --packages-above rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection rosidl_typesupport_fastrtps_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18545

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[InvincibleRMC]

Pulls: #945, ros2/rosidl_typesupport_fastrtps#145
Gist: https://gist.githubusercontent.com/InvincibleRMC/722adcc3d2011fa39540f64f251d65df/raw/e2297a0c6e2aca964e6406ba63beb6b02477dc3d/ros2.repos
BUILD args: --continue-on-error --packages-above-and-dependencies rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_fastrtps_cpp
TEST args: --packages-above rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_fastrtps_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18549

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[InvincibleRMC]

Pulls: #945, ros2/rosidl_typesupport_fastrtps#145
Gist: https://gist.githubusercontent.com/InvincibleRMC/121d0ef53beee6d66aefdfb2eec72c97/raw/e2297a0c6e2aca964e6406ba63beb6b02477dc3d/ros2.repos
BUILD args: --continue-on-error --packages-above-and-dependencies rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp
TEST args: --packages-above rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18566

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

Rebuild

• Linux

• Linux-rhel

[InvincibleRMC]

Pulls: #945, ros2/rosidl_typesupport_fastrtps#145
Gist: https://gist.githubusercontent.com/InvincibleRMC/3734099820e7dcc6682c9b2b7c2b83e8/raw/e2297a0c6e2aca964e6406ba63beb6b02477dc3d/ros2.repos
BUILD args: --continue-on-error --packages-above-and-dependencies rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp
TEST args: --packages-above rosidl_generator_c rosidl_generator_cpp rosidl_parser rosidl_generator_tests rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_fastrtps_c rosidl_typesupport_fastrtps_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18570

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[jmachowinski]

@InvincibleRMC Tanks for the PR ! Looks good to me at first glance, could you post a generated header from this ?
You seem to disable the deprecation warnings in some places, whats the reasoning behind that ?

[Pengkun-ZHU]

Greetings mate @InvincibleRMC, I too were interested in this request and did a PR.
I just quickly looked into yours, admittedly it is architecturally cleaner, the IDL annotation pipeline usage instead of comment-to-annotation approach seems more native.

I'll simply withdraw mine, should you need any help, I'd be more than willing to assist:)

[jmachowinski]

@Pengkun-ZHU can you review the patch ?

[InvincibleRMC]

@InvincibleRMC Tanks for the PR ! Looks good to me at first glance, could you post a generated header from this ? You seem to disable the deprecation warnings in some places, whats the reasoning behind that ?

Here is the detail/deprecated__struct.hpp for example. We deprecated the set__x() method and the x field itself. We have to add deprecation suppression around any other internal functions. For example here in the equality method we still need to use the old deprecated field to check if messages are equal. So any internal private use has suppression but, any public apis that directly affect setting or use of the variable do emit deprecation warnings.

If we had private fields and only public getters and setters we wouldn't need any internal deprecation suppression.

// generated from rosidl_generator_cpp/resource/idl__struct.hpp.em
// with input from rosidl_generator_tests:msg/Deprecated.idl
// generated code does not contain a copyright notice

// IWYU pragma: private, include "rosidl_generator_tests/msg/deprecated.hpp"


#ifndef ROSIDL_GENERATOR_TESTS__MSG__DETAIL__DEPRECATED__STRUCT_HPP_
#define ROSIDL_GENERATOR_TESTS__MSG__DETAIL__DEPRECATED__STRUCT_HPP_

#include <algorithm>
#include <array>
#include <cstdint>
#include <memory>
#include <string>
#include <vector>

#include "rosidl_runtime_c/deprecation.h"
#include "rosidl_runtime_cpp/bounded_vector.hpp"
#include "rosidl_runtime_cpp/message_initialization.hpp"


#ifndef _WIN32
# define DEPRECATED__rosidl_generator_tests__msg__Deprecated __attribute__((deprecated))
#else
# define DEPRECATED__rosidl_generator_tests__msg__Deprecated __declspec(deprecated)
#endif

namespace rosidl_generator_tests
{

namespace msg
{

// message struct
template<class ContainerAllocator>
struct Deprecated_
{
  using Type = Deprecated_<ContainerAllocator>;

  DISABLE_DEPRECATED_PUSH

  explicit Deprecated_(rosidl_runtime_cpp::MessageInitialization _init = rosidl_runtime_cpp::MessageInitialization::ALL)
  {
    if (rosidl_runtime_cpp::MessageInitialization::ALL == _init ||
      rosidl_runtime_cpp::MessageInitialization::ZERO == _init)
    {
      this->x = 0l;
      this->new_x = 0l;
    }
  }

  explicit Deprecated_(const ContainerAllocator & _alloc, rosidl_runtime_cpp::MessageInitialization _init = rosidl_runtime_cpp::MessageInitialization::ALL)
  {
    (void)_alloc;
    if (rosidl_runtime_cpp::MessageInitialization::ALL == _init ||
      rosidl_runtime_cpp::MessageInitialization::ZERO == _init)
    {
      this->x = 0l;
      this->new_x = 0l;
    }
  }
  DISABLE_DEPRECATED_POP


  // field types and members
  using _x_type =
    int32_t;
  [[deprecated("Use new_x")]]
  _x_type x;
  using _new_x_type =
    int32_t;
  _new_x_type new_x;

  // setters for named parameter idiom
  [[deprecated("Use new_x")]]
  Type & set__x(
    const int32_t & _arg)
  {
    DISABLE_DEPRECATED_PUSH
    this->x = _arg;
    DISABLE_DEPRECATED_POP
    return *this;
  }
  Type & set__new_x(
    const int32_t & _arg)
  {
    this->new_x = _arg;
    return *this;
  }

  // constant declarations

  // pointer types
  using RawPtr =
    rosidl_generator_tests::msg::Deprecated_<ContainerAllocator> *;
  using ConstRawPtr =
    const rosidl_generator_tests::msg::Deprecated_<ContainerAllocator> *;
  using SharedPtr =
    std::shared_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator>>;
  using ConstSharedPtr =
    std::shared_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator> const>;

  template<typename Deleter = std::default_delete<
      rosidl_generator_tests::msg::Deprecated_<ContainerAllocator>>>
  using UniquePtrWithDeleter =
    std::unique_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator>, Deleter>;

  using UniquePtr = UniquePtrWithDeleter<>;

  template<typename Deleter = std::default_delete<
      rosidl_generator_tests::msg::Deprecated_<ContainerAllocator>>>
  using ConstUniquePtrWithDeleter =
    std::unique_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator> const, Deleter>;
  using ConstUniquePtr = ConstUniquePtrWithDeleter<>;

  using WeakPtr =
    std::weak_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator>>;
  using ConstWeakPtr =
    std::weak_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator> const>;

  // pointer types similar to ROS 1, use SharedPtr / ConstSharedPtr instead
  // NOTE: Can't use 'using' here because GNU C++ can't parse attributes properly
  typedef DEPRECATED__rosidl_generator_tests__msg__Deprecated
    std::shared_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator>>
    Ptr;
  typedef DEPRECATED__rosidl_generator_tests__msg__Deprecated
    std::shared_ptr<rosidl_generator_tests::msg::Deprecated_<ContainerAllocator> const>
    ConstPtr;

  // comparison operators
  bool operator==(const Deprecated_ & other) const
  {
    DISABLE_DEPRECATED_PUSH
    if (this->x != other.x) {
      return false;
    }
    DISABLE_DEPRECATED_POP
    if (this->new_x != other.new_x) {
      return false;
    }
    return true;
  }
  bool operator!=(const Deprecated_ & other) const
  {
    return !this->operator==(other);
  }
};  // struct Deprecated_

// alias to use template instance with default allocator
using Deprecated =
  rosidl_generator_tests::msg::Deprecated_<std::allocator<void>>;

// constant definitions

}  // namespace msg

}  // namespace rosidl_generator_tests

#endif  // ROSIDL_GENERATOR_TESTS__MSG__DETAIL__DEPRECATED__STRUCT_HPP_

[InvincibleRMC]

If approved could be used on ros2/common_interfaces#254 for instance.

[Pengkun-ZHU]

Hi @InvincibleRMC, @jmachowinski, all the code looks sound to me.
One small add-on suggestion: I noticed the CLI tools (ros2 topic echo, etc.) could also benefit from showing deprecation warnings when displaying messages. Since the Python generator emits DeprecationWarning, we could enable warning display in ros2cli to make deprecated fields visible to users.

For the Python generator support, I actually have a working implementation that adds deprecation warnings to Python message properties. I'd be happy to collaborate on that side if you'd like a hand.

[InvincibleRMC]

I don't think we should warn in cli tools. Deprecation warnings should affect maintainers of packages not necessarily users of packages.

[jmachowinski]

Hm, I am confused, you seem to focus on deprecation of single members of messages. I thought this was about deprecation of messages altogether.
E.g. This use case : ros2/common_interfaces#283

[InvincibleRMC]

I set out to solve this ros2/ros2#1679

But for overall deprecation of messages you could just mark all fields deprecated and point towards the new message if one exists. Which should work for all messages except Empty.

[Pengkun-ZHU]

I assume IDL parser already parses struct-level annotations?
We could simply let the generator check message.structure.has_annotation('deprecated') and apply the deprecation to the class/type alias. This is actually much simpler than per-field:

• C++ generator: Apply [[deprecated("text")]] to the struct template class, or use the existing deprecated_macro_name that's already applied to Ptr/ConstPtr - but conditionally based on the annotation
• C generator: Apply ROSIDL_DEPRECATED("text") to the typedef struct.
• Python generator: Apply @_deprecated('text') to the message class itself

[jmachowinski]

I think you can't deprecate a message in all corner cases correctly with only marking its members as deprecated.
I guess you would not see a deprecation warning then in functions using the message and in the wired case, that nobody uses a member of the struct.

Anyway this was what I had in mind :

namespace test_msgs
{
namespace msg
{
// message struct
template<class ContainerAllocator>
struct [[deprecates("Reason")]] Empty_
{
.
.
.
};
}  // namespace msg
}  // namespace test_msgs

[InvincibleRMC]

I don't disagree that in all case this a not necessarily a clean replacement for struct level deprecation. My goal was to solve ros2/ros2#1679 which is about field level deprecation. The hope is that this would help us evolve message definitions. Rather then need to replace them with new ones every time. I would rather keep these prs focused on addressing this issue. And we can discuss struct level in the future.