Enforce unique, nonempty labels for strategies

src/games/game.h

@@ -369,11 +369,7 @@ class GameStrategyRep : public std::enable_shared_from_this<GameStrategyRep> {
   /// Returns the text label associated with the strategy
   const std::string &GetLabel() const { return m_label; }
   /// Sets the text label associated with the strategy
-  void SetLabel(const std::string &p_label)
-  {
-    CheckLabel(p_label);
-    m_label = p_label;
-  }
+  void SetLabel(const std::string &p_label);
 
   /// Returns the game on which the strategy is defined
   Game GetGame() const;
@@ -492,6 +488,8 @@ class GamePlayerRep : public std::enable_shared_from_this<GamePlayerRep> {
   GameStrategy GetStrategy(int st) const;
   /// Returns the collection of strategies available to the player
   Strategies GetStrategies() const;
+  /// Validate that p_label is a nonempty, valid, unique label for a strategy of this player.
+  void CheckStrategyLabel(const std::string &p_label) const;
   //@}
 
   /// @name Sequences
@@ -1311,6 +1309,27 @@ inline void GameOutcomeRep::SetPayoff(const GamePlayer &p_player, const Number &
 
 inline GamePlayer GameStrategyRep::GetPlayer() const { return m_player->shared_from_this(); }
 inline Game GameStrategyRep::GetGame() const { return m_player->GetGame(); }
+inline void GameStrategyRep::SetLabel(const std::string &p_label)
+{
+  if (p_label == m_label) {
+    return;
+  }
+  GetPlayer()->CheckStrategyLabel(p_label);
+  m_label = p_label;
+}
+
+inline void GamePlayerRep::CheckStrategyLabel(const std::string &p_label) const
+{
+  if (p_label.empty()) {
+    throw ValueException("Strategy label must not be empty");
+  }
+  CheckLabel(p_label);
+  for (const auto &strategy : m_strategies) {
+    if (strategy->GetLabel() == p_label) {
+      throw ValueException("Strategy label must be unique for the player");
+    }
+  }
+}
 
 inline Game GameSequenceRep::GetGame() const { return m_player->GetGame(); }
 inline GamePlayer GameSequenceRep::GetPlayer() const { return m_player->shared_from_this(); }

src/games/gametable.cc

@@ -531,13 +531,15 @@ GameStrategy GameTableRep::NewStrategy(const GamePlayer &p_player, const std::st
   if (p_player->GetGame().get() != this) {
     throw MismatchException();
   }
+  p_player->CheckStrategyLabel(p_label);
+  auto strategy = std::make_shared<GameStrategyRep>(p_player.get(),
+                                                    p_player->m_strategies.size() + 1, p_label);
   IncrementVersion();
   std::vector<long> old_radices;
   for (const auto &player : m_players) {
     old_radices.push_back(player->m_strategies.size());
   }
-  p_player->m_strategies.push_back(std::make_shared<GameStrategyRep>(
-      p_player.get(), p_player->m_strategies.size() + 1, p_label));
+  p_player->m_strategies.push_back(strategy);
   RebuildTable(old_radices);
   return p_player->m_strategies.back();
 }

src/gui/gamedoc.cc

@@ -497,7 +497,15 @@ void GameDocument::DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label
 
 void GameDocument::DoNewStrategy(GamePlayer p_player)
 {
-  m_game->NewStrategy(p_player, std::to_string(p_player->GetStrategies().size() + 1));
+  std::set<std::string> strategyLabels;
+  for (const auto &strategy : p_player->GetStrategies()) {
+    strategyLabels.insert(strategy->GetLabel());
+  }
+  int number = p_player->GetStrategies().size() + 1;
+  while (contains(strategyLabels, std::to_string(number))) {
+    number++;
+  }
+  m_game->NewStrategy(p_player, std::to_string(number));
   NotifyChanged(GameModificationType::GameForm);
 }
 

src/pygambit/game.pxi

@@ -2173,15 +2173,20 @@ class Game:
         resolved_outcome = cython.cast(Outcome, self._resolve_outcome(outcome, "set_outcome"))
         self.game.deref().SetOutcome(resolved_node.node, resolved_outcome.outcome)
 
-    def add_strategy(self, player: Player | str, label: str = None) -> Strategy:
+    def add_strategy(self, player: Player | str, label: str) -> Strategy:
         """Add a new strategy to the set of strategies for `player`.
 
+        .. versionchanged:: 16.7.0
+            A label is now required and must be nonempty and unique among the
+            player's strategies.
+
         Parameters
         ----------
         player : Player or str
             The player to create the new strategy for
-        label : str, optional
-            The label to assign to the new strategy
+        label : str
+            The label for the new strategy.  Must be nonempty and not already in use
+            by another of the player's strategies.
 
         Returns
         -------
@@ -2194,16 +2199,17 @@ class Game:
             If `player` is a `Player` from a different game.
         UndefinedOperationError
             If called on a game which has an extensive representation.
+        ValueError
+            If `label` is empty or is already the label of another of the player's strategies.
         """
         if self.is_tree:
             raise UndefinedOperationError(
                 "Adding strategies is only applicable to games in strategic form"
             )
         resolved_player = cython.cast(Player,
                                       self._resolve_player(player, "add_strategy"))
-        label_bytes = (str(label) if label is not None else "").encode("ascii")
         return Strategy.wrap(
-            self.game.deref().NewStrategy(resolved_player.player, label_bytes)
+            self.game.deref().NewStrategy(resolved_player.player, label.encode("ascii"))
         )
 
     def delete_strategy(self, strategy: Strategy | str) -> None:

src/pygambit/strategy.pxi

@@ -55,18 +55,15 @@ class Strategy:
         """Get or set the text label associated with the strategy.
 
         .. versionchanged:: 16.7.0
-            An invalid label now raises ``ValueError``: a label may contain only printable ASCII
-            characters and spaces, not begin/end with a space, nor have two consecutive spaces.
+            A strategy label must be nonempty and unique among the player's strategies;
+            an empty or duplicate label now raises ``ValueError``.  A label may contain only
+            printable ASCII characters and spaces, not begin/end with a space, nor have two
+            consecutive spaces.
         """
         return self.strategy.deref().GetLabel().decode("ascii")
 
     @label.setter
     def label(self, value: str) -> None:
-        if value == self.label:
-            return
-        if value == "" or value in (strategy.label for strategy in self.player.strategies):
-            warnings.warn("In a future version, strategies for a player must have unique labels",
-                          FutureWarning)
         self.strategy.deref().SetLabel(value.encode("ascii"))
 
     @property

tests/test_players.py

@@ -145,6 +145,27 @@ def test_add_strategy_label_invalid_raises_valueerror(label):
         game.add_strategy(next(iter(game.players)), label)
 
 
+def test_add_strategy_requires_label():
+    game = gbt.Game.new_table([2, 2])
+    with pytest.raises(TypeError):
+        game.add_strategy(next(iter(game.players)))
+
+
+def test_strategy_label_empty_raises_valueerror():
+    game = gbt.Game.new_table([2, 2])
+    strategy = next(iter(next(iter(game.players)).strategies))
+    with pytest.raises(ValueError):
+        strategy.label = ""
+
+
+def test_strategy_label_duplicate_within_player_raises_valueerror():
+    game = gbt.Game.new_table([2, 2])
+    pl1 = next(iter(game.players))
+    s1, s2 = pl1.strategies
+    with pytest.raises(ValueError):
+        s2.label = s1.label
+
+
 def test_player_strategy_bad_label():
     game = gbt.Game.new_table([2, 2])
     pl1 = next(iter(game.players))
@@ -232,7 +253,7 @@ def test_player_get_min_payoff_null_outcome():
     pl1, pl2 = game.players
     assert pl1.min_payoff == 1
     assert pl2.min_payoff == 2
-    game.add_strategy(pl1)
+    game.add_strategy(pl1, "new strategy")
     # Currently the outcomes associated with the new entries in the table
     # are null outcomes.  So now minimum payoff should be zero from those.
     for player in game.players:
@@ -258,8 +279,27 @@ def test_player_get_max_payoff_null_outcome():
     pl1, pl2 = game.players
     assert pl1.max_payoff == -1
     assert pl2.max_payoff == -2
-    game.add_strategy(pl1)
+    game.add_strategy(pl1, "new strategy")
     # Currently the outcomes associated with the new entries in the table
     # are null outcomes.  So now minimum payoff should be zero from those.
     for player in game.players:
         assert player.max_payoff == 0
+
+
+def test_add_strategy_duplicate_label_raises_and_leaves_game_unchanged():
+    game = gbt.Game.new_table([2, 2])
+    pl = next(iter(game.players))
+    existing = next(iter(pl.strategies)).label
+    count_before = len(pl.strategies)
+    with pytest.raises(ValueError):
+        game.add_strategy(pl, existing)
+    assert len(pl.strategies) == count_before
+
+
+def test_add_strategy_empty_label_raises_and_leaves_game_unchanged():
+    game = gbt.Game.new_table([2, 2])
+    pl = next(iter(game.players))
+    count_before = len(pl.strategies)
+    with pytest.raises(ValueError):
+        game.add_strategy(pl, "")
+    assert len(pl.strategies) == count_before