Report a declared port whose name cannot be bound (fixes #1215) (#1216)

A SubTree model can declare a port that nothing is able to bind, and nothing
says so.

`validatePortName` rejects only a leading digit, so `<input_port
name="_myPort"/>` is accepted. `IsAllowedPortName` requires an alphabetic first
character, and `createNodeFromXML` uses it to classify every instance
attribute, so `_myPort="{outer}"` is diverted into `other_attributes`, the
remapping is dropped, and the SubTree reads its declared default:

  Parent sets outer='FROM_PARENT'. Declared default is DEFAULT_NOT_WIRED.
    myPort  inside SubTree -> FROM_PARENT
    _myPort inside SubTree -> DEFAULT_NOT_WIRED

`InputPort("_myPort")` already throws "Underscore is reserved", so the C++ API
and the XML declaration path disagree about the same name.

Throw when an instance attribute names a port the node declares but cannot
bind. Declared ports live in two places, so the check reads `manifest->ports`
for a registered node and `subtree_models` for a SubTree.

Rejecting the declaration in `validatePortName` would be the smaller change. It
would also break XML that loads today: a port declared and never remapped is
inert, and generated Objectives can carry such a declaration. This fires only
where a remapping is actually being lost, so an inert declaration keeps loading.

Author: dv-picknik

src/xml_parsing.cpp

@@ -847,6 +847,29 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element,
     }
     else if(!IsReservedAttribute(port_name))
     {
+      // The name may still match a declared port. validatePortName only rejects
+      // a leading digit, so a SubTree model can declare
+      // <input_port name="_myPort"/>, a name IsAllowedPortName refuses and no
+      // C++ InputPort() can create. Falling through to other_attributes would
+      // drop the remapping without a word and leave the node on its declared
+      // default, so report it instead.
+      bool is_declared_port = manifest != nullptr && manifest->ports.count(port_name) > 0;
+      if(!is_declared_port && node_type == NodeType::SUBTREE)
+      {
+        auto model_it = subtree_models.find(type_ID);
+        is_declared_port = model_it != subtree_models.end() &&
+                           model_it->second.ports.count(port_name) > 0;
+      }
+      if(is_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_subtree.cpp

@@ -1132,3 +1132,79 @@ TEST(SubTree, LiteralNonBooleanPortsRejectLogicalNot)
   ASSERT_THROW((void)tree.tickWhileRunning(), RuntimeError);
   ASSERT_EQ(tree.subtrees[1]->blackboard->get<std::string>("value"), "1not_bool");
 }
+
+// A SubTree model can declare a port whose name no node can bind.
+// validatePortName only rejects a leading digit, while IsAllowedPortName, which
+// classifies instance attributes and which InputPort()/OutputPort() enforce,
+// requires an alphabetic first character. Before the fix the remapping below was
+// diverted into other_attributes and the SubTree read its declared default, with
+// no exception and no log.
+TEST(SubTree, DeclaredPortNameThatCannotBeBound)
+{
+  static const char* xml_text = R"(
+<root BTCPP_format="4" main_tree_to_execute="MainTree">
+  <BehaviorTree ID="MainTree">
+    <Sequence>
+      <Script code="outer:='from_parent'"/>
+      <SubTree ID="Sub" _myPort="{outer}"/>
+    </Sequence>
+  </BehaviorTree>
+  <BehaviorTree ID="Sub">
+    <AlwaysSuccess/>
+  </BehaviorTree>
+  <TreeNodesModel>
+    <SubTree ID="Sub">
+      <input_port name="_myPort" default="never_wired"/>
+    </SubTree>
+  </TreeNodesModel>
+</root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml_text), RuntimeError);
+}
+
+// The same underscore attribute on a SubTree that does not declare it is not a
+// port. It must keep landing in other_attributes and must not throw.
+TEST(SubTree, UndeclaredUnderscoreAttributeIsNotAPort)
+{
+  static const char* xml_text = R"(
+<root BTCPP_format="4" main_tree_to_execute="MainTree">
+  <BehaviorTree ID="MainTree">
+    <SubTree ID="Sub" _my_editor_state="true"/>
+  </BehaviorTree>
+  <BehaviorTree ID="Sub">
+    <AlwaysSuccess/>
+  </BehaviorTree>
+  <TreeNodesModel>
+    <SubTree ID="Sub">
+      <input_port name="goal" default="g"/>
+    </SubTree>
+  </TreeNodesModel>
+</root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_NO_THROW(factory.createTreeFromText(xml_text));
+}
+
+// A declared port nobody remaps stays inert and keeps loading, so existing trees
+// carrying such a declaration are unaffected.
+TEST(SubTree, DeclaredUnbindablePortLoadsWhenNotRemapped)
+{
+  static const char* xml_text = R"(
+<root BTCPP_format="4" main_tree_to_execute="MainTree">
+  <BehaviorTree ID="MainTree">
+    <SubTree ID="Sub"/>
+  </BehaviorTree>
+  <BehaviorTree ID="Sub">
+    <AlwaysSuccess/>
+  </BehaviorTree>
+  <TreeNodesModel>
+    <SubTree ID="Sub">
+      <input_port name="_myPort" default="inert"/>
+    </SubTree>
+  </TreeNodesModel>
+</root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_NO_THROW(factory.createTreeFromText(xml_text));
+}