rclpy.time.Time and rclpy.duration.Duration raise error on comparing to None

[suurjaak]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • binaries
• Version or commit hash:
  • ros-foxy-rclpy 1.0.8-1focal.20220209.144341
• Client library:
  • rclpy

Steps to reproduce issue

import rclpy.time

t = rclpy.time.Time()
print("t in [None, t]: %r" % (t in [None, t]))

import rclpy.duration

d = rclpy.duration.Duration()
print("d in [None, d]: %r" % (d in [None, d]))

Expected behavior

t in [None, t]: True

d in [None, d]: True

Actual behavior

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/time.py", line 105, in __eq__
    raise TypeError("Can't compare time with object of type: ", type(other))
TypeError: ("Can't compare time with object of type: ", <class 'NoneType'>)

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/duration.py", line 44, in __eq__
    raise TypeError("Can't compare duration with object of type: ", type(other))
TypeError: ("Can't compare duration with object of type: ", <class 'NoneType'>)

Additional information

Not specific to Foxy: present in all versions of ROS2.

[fujitatomoya]

this problem can be reproducible with https://github.com/ros2/ros2/commit/077c53b9a27100bcb4e3ec94549d5a9e6690fea5

[suurjaak]

As a fix, I would simply remove this raising (https://github.com/ros2/rclpy/blob/master/rclpy/rclpy/time.py#L106) and instead return NotImplemented if the given argument is not an instance of Time. Ditto for Duration.

There is a comment there that says it's for avoiding "hard-to-find bugs when comparing with integers", but this does not seem merited. The temporal classes are not expected to support any auto-casting anyway, and getting an error from a simple comparison is certainly surprising. How I stumbled upon this was a unittest failing because assertNotEqual(sometimevalue, None) crashed..

I can make a pull request for this, if such a change is acceptable.

[suurjaak]

@fujitatomoya ping?

[fujitatomoya]

I confirmed that this can be reproduced, but not sure this is the issue. I think current code makes sense, any thoughts?

[suurjaak]

How does the current code make sense? It is a perfectly valid operation to compare any Python object for equality with any other Python object.

Current code logic breaks standard expectations.

[fujitatomoya]

I am not original author on this code, but It seems to return TypeError explicitly and intentionally. So i just would like to hear from other opinion.

[suurjaak]

@sloretz Any input on this?

[jacobperron]

I agree that we might want to change the logic to return False (instead of raising an error). The current logic does not seem very Pythonic.

[suurjaak]

@jacobperron @fujitatomoya How about that pull request?

[suurjaak]

@jacobperron @fujitatomoya @sloretz How about that pull request: https://github.com/ros2/rclpy/pull/1007 ?