docs: say what actually guards the dynamic_message publish/take casts - #665
Open
espressolee wants to merge 1 commit into
Open
docs: say what actually guards the dynamic_message publish/take casts#665espressolee wants to merge 1 commit into
espressolee wants to merge 1 commit into
Conversation
The SAFETY comment on DynamicPublisher::publish is a verbatim copy of the one on Publisher::publish, typo included. On Publisher<T> its first sentence is true: the generic parameter ties the message to <T as Message>::RmwMsg. On DynamicPublisher, whose whole point is that the type is a runtime value, it claims a guarantee the type system cannot give. The actual guard is the message_type comparison two lines above, and that comparison is by name. Neither comment mentions the obligation that the name comparison rests on: two different type support libraries are involved. DynamicMessageMetadata loads rosidl_typesupport_introspection_c and that is what lays out message.storage; rcl_publish serialises those bytes through rosidl_typesupport_c. Both constructors already say so in prose a few lines up -- "This loads the introspection type support library ... However, we also need the regular type support library" -- so the fact is known, just absent from where the unsafe block is justified. Same for DynamicSubscription: metadata.create() lays the storage out by introspection, rcl_take writes it through the C type support, and the comment discusses pointer validity only. Both are sound as written: each constructor resolves the metadata and the C type support from the same package name in the same function, so they come from one install prefix. This records why, so a reader auditing these blocks is not told the compiler is checking something it never sees. Comments only; no code changes.
Author
|
Gentle review ping: this remains a comments-only safety-contract clarification at
The issue it documents (#664) is still open; this PR only states the existing ownership/lifetime guard around the casts and does not claim to fix #664. Happy to reword or drop either comment hunk. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comments only. No code changes — every changed line starts with
//, and stripping commentsfrom both files before and after gives byte-identical text.
What it fixes
DynamicPublisher::publishcarries this:// SAFETY: The message type is guaranteed to match the publisher type by the type system.It is a verbatim copy of the comment on
Publisher::publish, misspelling of "explictly"included. On
Publisher<T>the sentence is true — the generic parameter ties the message to<T as Message>::RmwMsg. OnDynamicPublisher, whose whole point is that the type is a runtimevalue, the compiler cannot see anything of the sort. What actually guards the cast is the
message_typecomparison two lines above, and that comparison is by name(
MessageTypeName { package_name, type_name }).The obligation neither comment mentions
Two type support libraries are involved:
DynamicMessageMetadatarosidl_typesupport_introspection_cmessage.storagerosidl_typesupport_crcl_publish/rcl_takeserialise withSo the name comparison rests on those two describing the same layout. Both constructors already
say the two libraries exist, in prose a few lines up — "This loads the introspection type
support library … However, we also need the regular type support library" — so this is known,
just missing from the place where the
unsafeblock is justified.DynamicSubscription'stake()is the mirror:metadata.create()lays out the storage byintrospection,
rcl_takewrites it through the C type support, and the comment discussespointer validity only.
Both are sound as written
Each constructor resolves the metadata and the C type support from the same package name in the
same function, through one
ament.find_package, so they come from one install prefix and agreeby construction. This PR records that, rather than changing it. The point is that a reader
auditing these blocks is currently told the compiler is checking something it never sees.
Happy to reword or drop either hunk.
Generative AI disclosure
This PR was opened on 2026-08-08, before this repository adopted the OSRF AI policy in #672 on
2026-09-01. Adding the disclosure now so the record is complete:
AGENTS.mdalso asks for the trailer in the commit message body. I have left the commit as itis rather than rewriting it, since amending would invalidate the existing CI results and any
references to the current SHA. Happy to amend and force-push if you would prefer the trailer on
the commit itself.