Fix: Report a declared port whose name cannot be used - #35
dv-picknik wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughThe XML parser now checks disallowed, non-reserved attributes against declared node ports and SubTree ports. It throws a line-specific ChangesXML port validation
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable issue remains identified; the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
A SubTree model can declare a port that nothing is able to bind.
`validatePortName` only rejects a leading digit, so `<input_port
name="_myPort"/>` parses, while `IsAllowedPortName` requires an alphabetic
first character and therefore refuses the same name. The C++ `InputPort()`
API refuses it too, with "Underscore is reserved".
The cost is silent. `createNodeFromXML` classifies each instance attribute
with `IsAllowedPortName`, so `_myPort="{outer}"` falls through to
`other_attributes`, the remapping is dropped, and the SubTree reads the
declared default instead of the parent's value. No exception and no log:
Parent sets outer='FROM_PARENT'. Declared default is DEFAULT_NOT_WIRED.
myPort inside SubTree -> FROM_PARENT
_myPort inside SubTree -> DEFAULT_NOT_WIRED
Throw when an attribute names a port the node declares but cannot bind,
naming the port and suggesting the rename.
Rejecting the declaration itself would be the smaller change and the wrong
one. A port declared and never remapped is inert, and Objectives in the wild
carry those: an editor wrote `_collapsed` into a `<TreeNodesModel>` port list
in moveit_pro_example_ws. Refusing it at parse time would stop that Objective
loading to fix a bug it does not have. This fires only where the remapping is
actually being lost, so the 148 `_collapsed` instance attributes in that same
workspace keep loading untouched.
Refs PickNikRobotics/moveit_pro#17640
6a4e20e to
a5ad6c0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
[written by AI] Closing. The premise does not hold up. A declared port BT.CPP cannot bind does lose its remapping in silence. What that argument rests on is 4.9.0 having taken something away, and it did not. The deb we ship today, const char first_char = str.data()[0];
if(!std::isalpha(first_char))
{
return false;
}and it has the same That leaves the question of who declares a port named So this would add a permanent throw to a vendored fork, to be carried across every upstream rebase, for a case our own product can no longer produce and that nobody has reported. Not worth it. The inconsistency itself is real and upstream. apt_build_farm#69 closes with this, and #23165 goes back to the published |
1 participant
[written by AI]
A SubTree can declare a port that nothing is able to bind, and nothing reports it.
BT.CPP has two port-name rules that disagree.
validatePortNameinxml_parsing.cppruns on<TreeNodesModel>port declarations and rejects only a leading digit.IsAllowedPortNameinbasic_types.cpprequires an alphabetic first character, andcreateNodeFromXMLuses it to classify every instance attribute. The C++InputPort()API uses it too, and says so plainly:So the declaration parses, and then
_myPort="{outer}"on the instance failsIsAllowedPortName, drops through toother_attributes, and the remapping is gone. The SubTree reads its declared default instead of the parent's value, with no exception and no log. Measured against this branch's parent:What this does
Throws when an instance attribute names a port the node declares but cannot bind, naming the port and the rename. Declared ports live in two places, so the check reads both:
manifest->portsfor a registered node,subtree_modelsfor a SubTree.Why not reject the declaration
That is the one-line change and it is the wrong one. A port declared and never remapped is inert, and Objectives in the wild carry those.
moveit_pro_example_wshas one: an editor wrote_collapsedinto a<TreeNodesModel>port list inkinova_gen3_base_config/objectives/get_imarker_pose_from_mesh_visualization.xml. Refusing it at parse time would stop that Objective loading, to fix a bug it does not have.The same workspace carries 148
_collapsedattributes on SubTree instances whose models do not declare it. This change fires only where a remapping is actually being lost, so all of those keep loading untouched.Testing
pixi run build && pixi run test, 529/529. Four new cases ingtest_name_validation.cpp:_myPortdeclared and remapped on a SubTree_foodeclared and remapped on a registered node, via thePortsListoverload_collapsedon an instance whose model does not declare it_collapseddeclared in a model and not remappedThe registered-node case exists because
CreatePortscreens names throughIsAllowedPortName, but thePortsListoverload ofregisterNodeTypetakes the map as given, so a hand-built list can carry a key no attribute will bind.Provenance
validatePortName's digit-only check is upstream,8f86eb99by Davide Faconti, present at tag4.9.0. Not something the fork introduced. Worth raising upstream separately. This carries the fix for our 10.2 line in the meantime.Refs PickNikRobotics/moveit_pro#17640