Skip to content

Reject incompatible parameter range constraints - #3243

Open
aiqubits wants to merge 1 commit into
ros2:rollingfrom
aiqubits:fix/issue-2184-parameter-range-types
Open

Reject incompatible parameter range constraints#3243
aiqubits wants to merge 1 commit into
ros2:rollingfrom
aiqubits:fix/issue-2184-parameter-range-types

Conversation

@aiqubits

Copy link
Copy Markdown

Fixes #2184.

Range constraints were previously ignored when the parameter type did not
match one of the existing numeric range validation branches. For example, a
boolean parameter could be declared with an integer range constraint.

This change:

  • rejects integer range constraints for types other than integers and integer arrays;
  • rejects floating-point range constraints for types other than doubles and double arrays;
  • validates statically typed parameters that are declared without an initial value;
  • validates dynamically typed parameters whenever they acquire or change their concrete type;
  • preserves the existing scalar and per-element array range validation behavior.

Testing

  • Added coverage for incompatible initialized parameter declarations.
  • Added coverage for statically typed declarations without initial values.
  • Added coverage for incompatible integer/floating-point range combinations.
  • Added coverage for dynamically typed parameters changing to an incompatible type.
  • Full test_node suite passed: 48/48 tests, using the equivalent patch with the
    rclcpp version matching the available archived Rolling dependencies.
  • git diff --check passed.

Signed-off-by: aiqubits <aiqubits@hotmail.com>

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed exact head 727478d93e34395c71a0c1125772eee0fdc60821 and GitHub's current merge 08a8e0b1501ea7d4c16259ae3bd0564c0eddda80.

The new compatibility check is applied in all three relevant paths: initialized declarations, dynamic retyping, and statically typed declarations without an initial value. An independent exact-source matrix covered every ROS parameter type against integer-only, floating-point-only, and mutually exclusive dual-range descriptors for each path. The base accepted 69 incompatible combinations; the head and merge matched the intended matrix in all 90/90 cases. The existing scalar and per-element array range branches for integer, integer-array, double, and double-array values remain present and ordered after the type check.

git diff --check is clean, the sole commit passes DCO, and the head and merge trees are identical. GitHub currently exposes DCO and Mergify checks but no repository build/test job for this PR, so my approval is based on the exact-source matrix and focused code-path review rather than an upstream compiled test run. I found no blocking issue.

Disclosure: I used OpenAI Codex to inspect the exact revisions, construct and run the compatibility matrix, and draft this review. I verified the source, revisions, outputs, and conclusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disallow setting range constraints for types that should not have them

2 participants