Skip to content

Improve missing parameter override error - #1324

Open
Old-Ding wants to merge 3 commits into
ros2:rollingfrom
Old-Ding:codex/parameter-override-error
Open

Improve missing parameter override error#1324
Old-Ding wants to merge 3 commits into
ros2:rollingfrom
Old-Ding:codex/parameter-override-error

Conversation

@Old-Ding

Copy link
Copy Markdown

Description

A parameter override such as -p bla currently exposes the internal lexer diagnostic Expected lexeme type (22), which does not tell the user that the assignment syntax is missing.

Translate the separator parse failure at the parameter-rule parser layer into the expected name:=value form. Keep the existing outer argument context, return codes, and all other invalid-rule diagnostics unchanged. Add a regression test through the public rcl_parse_arguments() entry point.

Addresses the diagnostic portion of #1001.

Is this user-facing behavior change?

Yes. Invalid parameter overrides that omit :=value now report the expected rule format instead of a lexer token number.

Did you use Generative AI?

Yes. OpenAI Codex assisted with root-cause analysis and preparing the focused code and regression test. I reviewed the diff and verification results.

Additional Information

Local verification:

  • source-level control-flow assertion verified that only the missing-separator branch replaces the lexer error and retains RCL_RET_INVALID_PARAM_RULE
  • UTF-8/CRLF validation for both changed files
  • git diff --check

The native test_arguments gtest requires the ROS 2 build dependencies and CMake/colcon tooling, which are not installed on this Windows host; repository CI covers the regression test.

Report the expected name:=value form when a parameter override lacks assignment syntax instead of exposing lexer token internals.

Signed-off-by: Old-Ding <35417409+Old-Ding@users.noreply.github.com>
@mergify

mergify Bot commented Jul 11, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Comment thread rcl/src/rcl/arguments.c

ret = rcl_lexer_lookahead2_expect(&lex_lookahead, RCL_LEXEME_SEPARATOR, NULL, NULL);
if (RCL_RET_WRONG_LEXEME == ret) {
rcl_reset_error();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this would (acutally does) delete the error messages provided by underlying implementation.
i think what we should do here is capture the error, reset the buffer, and then set a new formatted message that includes the previous error as context.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in dd4b6e7. The parser now captures the lexer error before resetting it, then includes that diagnostic as context in the parameter override format message. The regression test checks for both the user-facing guidance and the underlying lexer error.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Follow-up: the initial revision only covered the separator path. The updated change also routes errors reported by _rcl_parse_param_name() through the same context-preserving wrapper.

I verified the updated commit (ff95ccd) in a Rolling container: test_arguments passed, and all 52 CTest entries passed with 0 failures. The test binary was confirmed to load the locally built /tmp/colcon_ws/build/rcl/librcl.so.

Old-Ding added 2 commits July 14, 2026 11:39
Keep the underlying lexer diagnostic when adding the user-facing parameter override format guidance. Extend the regression test to require both messages.

Signed-off-by: Old-Ding <35417409+Old-Ding@users.noreply.github.com>
Signed-off-by: Old-Ding <35417409+Old-Ding@users.noreply.github.com>
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.

2 participants