pleco-rs / Pleco

A Rust-based re-write of the Stockfish Chess Engine
https://crates.io/crates/pleco
GNU General Public License v3.0
358 stars 38 forks source link

`Board::get_piece_locations()` causes stack overflow #117

Closed AntonHermann closed 5 years ago

AntonHermann commented 5 years ago

Running Board::get_piece_locations() causes a stack overflow when run. This is the code that causes this bug for me: let board = Board::start_pos() for (sq, piece) in board.get_piece_locations() { debug!("{} {:?}", sq, piece); }`

sfleischman105 commented 5 years ago

Thanks for the report.

The problem can be reduced to:

let board = Board::start_pos();
let piece_locations = board.get_piece_locations();
for (sq, _) in piece_locations {
   println!("{}", sq);
}

Something about printing SQ's causes a stack overflow... weird. Looking at it right now.

AntonHermann commented 5 years ago

I've found the problem: The Display implementation of SQ uses self.toString(), which doesn't call the toString method defined earlier, it calls the toString implementation implementet for everything that implements Display (through the ToString trait I believe, not entirely sure). So renaming that or somehow directly specifying what method you mean should fix this &SQ::to_string(self) instead of &self.to_string works

AntonHermann commented 5 years ago

What's also blowing up the stack is that PieceLocationsIter::next() calls self.next() recursively if piece == Piece::None On default starting pos this means 4*8 = 32 recursive calls. You should go for a while loop to get to the next piece instead.

AntonHermann commented 5 years ago

I think I can make a PullRequest tomorrow or later this day, to fix these issueas

sfleischman105 commented 5 years ago

Thanks for the insights, that seems to be the problem. I remade the toString method to index into a static array, and modified the Iterator for PieceLocations to loop instead of using recursion. A good TIL for today: Apparently Rust doesn't support tail-call recursion, causing stack explosions like this one.

The fix is currently in the Beta-Branch right now, and I'll merge it once CI tests pass.