Fix: Report a declared port whose name cannot be used

src/xml_parsing.cpp

@@ -900,6 +900,29 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element,
     }
     else if(!IsReservedAttribute(port_name))
     {
+      // A name that IsAllowedPortName refuses can still be a declared port.
+      // validatePortName only rejects a leading digit, so a SubTree model may
+      // declare `<input_port name="_myPort"/>`, which no C++ InputPort() can
+      // ever bind. Falling straight through to other_attributes drops
+      // `_myPort="{value}"` without a word and leaves the node reading its
+      // default. Report it rather than ignoring the remapping in silence.
+      bool declared_port = manifest != nullptr && manifest->ports.count(port_name) > 0;
+      if(!declared_port && node_type == NodeType::SUBTREE)
+      {
+        auto model_it = subtree_models.find(type_ID);
+        declared_port = model_it != subtree_models.end() &&
+                        model_it->second.ports.count(port_name) > 0;
+      }
+      if(declared_port)
+      {
+        const std::string reason = " and is declared, but the name cannot be used"
+                                   " as a port. A port name must begin with an"
+                                   " alphabetic character. Rename the port, for"
+                                   " example [_my_port] to [my_port].";
+        throw RuntimeError(StrCat("A port with name [", port_name,
+                                  "] is found in the XML (", type_ID, ", line ",
+                                  std::to_string(element->GetLineNum()), ")", reason));
+      }
       other_attributes[port_name] = port_value;
     }
   }

tests/gtest_name_validation.cpp

@@ -414,3 +414,110 @@ TEST_F(NameValidationXMLTest, InvalidSubTreePortName_StartsWithDigit)
     </root>)";
   EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
 }
+
+// A SubTree model may declare a port whose name IsAllowedPortName refuses.
+// validatePortName only rejects a leading digit, so `_myPort` is accepted as a
+// declaration, but the instance attribute that would remap it is diverted to
+// other_attributes and the node reads its default instead. Silently.
+TEST_F(NameValidationXMLTest, RemappingADeclaredUnusablePortNameThrows)
+{
+  const char* xml = R"(
+    <root BTCPP_format="4" main_tree_to_execute="MainTree">
+      <BehaviorTree ID="MainTree">
+        <SubTree ID="MySubTree" _myPort="{outer}"/>
+      </BehaviorTree>
+      <BehaviorTree ID="MySubTree">
+        <AlwaysSuccess/>
+      </BehaviorTree>
+      <TreeNodesModel>
+        <SubTree ID="MySubTree">
+          <input_port name="_myPort" default="unwired"/>
+        </SubTree>
+      </TreeNodesModel>
+    </root>)";
+  try
+  {
+    factory.createTreeFromText(xml);
+    FAIL() << "expected a RuntimeError";
+  }
+  catch(const RuntimeError& err)
+  {
+    const std::string msg = err.what();
+    EXPECT_NE(msg.find("_myPort"), std::string::npos) << msg;
+    EXPECT_NE(msg.find("my_port"), std::string::npos) << msg;
+  }
+}
+
+// The same underscore attribute on a SubTree that does not declare it is UI
+// state, not a port. moveit_pro_example_ws carries 148 of these. Keep loading.
+TEST_F(NameValidationXMLTest, UndeclaredUnderscoreAttributeIsStillIgnored)
+{
+  const char* xml = R"(
+    <root BTCPP_format="4" main_tree_to_execute="MainTree">
+      <BehaviorTree ID="MainTree">
+        <SubTree ID="MySubTree" _collapsed="true"/>
+      </BehaviorTree>
+      <BehaviorTree ID="MySubTree">
+        <AlwaysSuccess/>
+      </BehaviorTree>
+      <TreeNodesModel>
+        <SubTree ID="MySubTree">
+          <input_port name="goal" default="g"/>
+        </SubTree>
+      </TreeNodesModel>
+    </root>)";
+  EXPECT_NO_THROW(factory.createTreeFromText(xml));
+}
+
+// A declared-but-unusable port that nobody remaps keeps its default and keeps
+// loading. This shape ships in moveit_pro_example_ws.
+TEST_F(NameValidationXMLTest, DeclaredUnusablePortNameLoadsWhenNotRemapped)
+{
+  const char* xml = R"(
+    <root BTCPP_format="4" main_tree_to_execute="MainTree">
+      <BehaviorTree ID="MainTree">
+        <SubTree ID="MySubTree"/>
+      </BehaviorTree>
+      <BehaviorTree ID="MySubTree">
+        <AlwaysSuccess/>
+      </BehaviorTree>
+      <TreeNodesModel>
+        <SubTree ID="MySubTree">
+          <inout_port name="_collapsed" default="false"/>
+        </SubTree>
+      </TreeNodesModel>
+    </root>)";
+  EXPECT_NO_THROW(factory.createTreeFromText(xml));
+}
+
+namespace
+{
+struct UnusablePortAction : public SyncActionNode
+{
+  UnusablePortAction(const std::string& name, const NodeConfig& config)
+    : SyncActionNode(name, config)
+  {}
+  NodeStatus tick() override
+  {
+    return NodeStatus::SUCCESS;
+  }
+};
+}  // namespace
+
+// The same trap reaches a registered node, not only a SubTree. `CreatePort`
+// screens names through IsAllowedPortName, but the PortsList overload of
+// registerNodeType takes the map as given, so a hand-built list can carry a key
+// no instance attribute will ever bind.
+TEST_F(NameValidationXMLTest, RemappingADeclaredUnusablePortOnARegisteredNodeThrows)
+{
+  factory.registerNodeType<UnusablePortAction>(
+      "UnusablePortAction", PortsList{ { "_foo", PortInfo(PortDirection::INPUT) } });
+
+  const char* xml = R"(
+    <root BTCPP_format="4" main_tree_to_execute="MainTree">
+      <BehaviorTree ID="MainTree">
+        <UnusablePortAction _foo="{outer}"/>
+      </BehaviorTree>
+    </root>)";
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}