Inconsistent handling of illegal moves

[ianfab]

Some illegal moves in shogi, xiangqi, and janggi currently are handled as legal, but then adjudicated as losing. This is not fully consistent as some illegal moves are detected early and correctly identified as illegal, whereas others are just declared as losing, but the move itself is considered legal. The situations where this issue applies are:

• Checkmate by shogi pawn drops
• Perpetual check, illegal repetitions

Idea: Filter out such moves in pyffish.

• It needs to be considered that there can be cases where the losing move is the only legal move.

[thearst3rd]

I was curious and tried to take a stab at this, at least for the shogi pawn drop mates.

First of all, I was trying to find if there was indeed a position in which the only legal move is to drop a shogi pawn and checkmate the enemy king. I'm pretty sure in normal shogi that is not possible, but I did come up with this ridiculous position where that condition is true:

sfen: sssssssss/sssssssss/sssssssss/sssssssss/sssssnkns/ssssss1ss/sssssnb1r/sssssssRB/sssssssPK b P 1
fen: sssssssss/sssssssss/sssssssss/sssssssss/sssssnkns/ssssss1ss/sssssnb1r/sssssssRB/sssssssPK[P] w - - 0 1

So, we can't just filter out the moves from pyffish/ffish.js and call it a day, we do need to handle those ending cases (which I'm sure you already knew 😄)

I figured I'd give it a shot, ignoring any performance hits or whatnot just to get it working (maybe that would be enough for the libraries). The simple idea is to just make the move, check if we're mated, then undo the move. This code is kinda what I'm going for, but I am running into the critical problem that I'm trying to modify the board state in a const function which is not allowed and the compiler freaks out :) (I think there are more issues too.)

diff --git a/src/position.cpp b/src/position.cpp
index acce8a49..7592f42e 100644
--- a/src/position.cpp
+++ b/src/position.cpp
@@ -968,6 +968,23 @@ bool Position::legal(Move m) const {
               return false;
   }

+  #ifdef CHECK_SHOGI_PAWN_DROP_MATE_ILLEGAL
+  // Illegal shogi pawn drop mate
+  // SUPER slow!!
+  if (var->shogiPawnDropMateIllegal && type_of(m) == DROP && type_of(moved_piece(m)) == SHOGI_PAWN && gives_check(m))
+  {
+      StateInfo info;
+      do_move(m, info, true);
+      // Recursive, super slow!!
+      // Can even end calling this function multiple times, but in normal shogi this should only be reached a max of two
+      // times since there can only be two kings.
+      size_t num_moves = MoveList<LEGAL>(this).size();
+      undo_move(m);
+      if (num_moves == 0)
+        return false;
+  }
+  #endif // CHECK_SHOGI_PAWN_DROP_MATE_ILLEGAL
+
   // No legal moves from target square
   if (immobility_illegal() && (type_of(m) == DROP || type_of(m) == NORMAL) && !(moves_bb(us, type_of(moved_piece(m)), to, 0) & board_bb()))
       return false;

Now this solution was so simple that I'm not surprised it doesn't work, and this almost certainly isn't the best way to do this, but I figured I'd leave it here to get your thoughts. Maybe this is something that be adapted into working? ¯\_(ツ)_/¯

For completion's sake, here is my patch to the makefile as well (to define the identifier for the ifdef above). I figured this doesn't need a PR.

diff --git a/src/Makefile b/src/Makefile
index 6fd7134b..0ebebdc1 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -95,6 +95,7 @@ endif

 ### 2.1. General and architecture defaults
 largeboards = no
+check_shogi_pawn_drop_mate_illegal = no
 all = no
 precomputedmagics = yes
 nnue = no
@@ -353,6 +354,11 @@ ifneq ($(all),no)
        CXXFLAGS += -DALLVARS
 endif

+# Check for illegal shogi pawn drops when generating legal moves
+ifneq ($(check_shogi_pawn_drop_mate_illegal),no)
+       CXXFLAGS += -DCHECK_SHOGI_PAWN_DROP_MATE_ILLEGAL
+endif
+
 ifeq ($(COMP),)
        COMP=gcc
 endif

[ianfab]

Thanks for investigating. I did not actually know a position where a losing (delayed illegal) move is the only legal move, I just was assuming that considering Fairy-SF's generality there has to be such a position in some variants at least, but it is useful for testing to now have a concrete example.

Yes, the requirement that legal is const makes it impossible to apply moves unless we remove it, which in turn might require to remove even more const for other methods as well. Since the legality checking already is almost recursive, e.g., https://github.com/ianfab/Fairy-Stockfish/blob/edb54396978f099c1d72ea408a06643f91889637/src/position.cpp#L937-L952
but has just enough filtering to always break the recursion, adding further recursion however also further complicates thinking through those loops. I think if we decided to go the way of filtering out the moves only for the libraries, these strict legality checks should IMO be a separate method, which can then also be non-const without any consequences for the rest of the code.

One caveat to having it as an additional filtering that is only applied to the libraries is that suddenly the engine and the libraries would no longer be perfectly in sync regarding the rules. I am not entirely sure if this really is a significant issue, but at least has to be considered. Still, considering that there are even more complex cases than the shogi pawn drop checkmates, this might anyway be unavoidable, so might still be worth exploring further.

Another option could be to have special case handling when it is possible to detect checkmate without executing the move. E.g., one of the main reasons why I implemented it as a loss is that in variants with arbitrary fairy pieces, like cannons and lame leapers, it is close to impossible to detect if a position is checkmate without executing the move. However, just like I have special faster handling for attack generation (attackers_to) for the happy case with moderate fairy pieces and a fallback for the general case, one could also try to apply a similar logic here, i.e., only using the delayed loss if the rules make it impossible to determine legality upfront. Not sure if it really is feasible, but that might be an option to get it correct even in the engine. Might also be worth looking into other (SF based) shogi engines how they do it.

[ianfab]

One further consideration if we made it a two stage legality check would also be variants like checkless check (https://github.com/ianfab/Fairy-Stockfish/tree/checkless), where depending on the interpretation of the rules even the strict legality check can be recursive, also see http://www.chessvariants.org/usualeq.dir/checklss.html. Actually in variants with cannons this situation might even be possible for shogi pawn drops, since the pawn drop might give check by serving as a cannon mount, and a checkmate delivering pawn drop might be the only legal move to parry it.

Some food for thought :)

[ianfab]

Reconsidering this, the main inconsistency here is the handling of shogi pawn drop mates, since that is inconsistent with handling of nifu (doubled pawns), while for perpetual check and chasing rule handling the current behavior anyway is required to handle forced illegal moves correctly.

In shogi however playing an illegal move or getting stalemated due to lack of a legal move is the same result, so this issue doesn't really need to be considered here, even though due to nifu it actually is quite straightforward to construct a legal shogi position where the only "legal" move is an illegal pawn drop mate, e.g., https://www.pychess.org/analysis/shogi?fen=BRBRSSSGG/nPPPPPPPP/n8/n8/n8/ll7/kl7/9/K8[PPPPPPPPPPggsl]_w_-_1.

Since there already is a fastAttacks2 property that covers specialization of attack calculation for shogi, it should actually be doable to have an early checkmate detection for pawn drops in Position::legal() in this special case, but still use the current generic fallback for shogi variants where an early detection is not possible due to fairy pieces which go beyond fastAttacks2, e.g., cannons/hoppers.