You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I'm currently looking around for a chess AI written in rust. I found Pleco, and it seems like a neat project, it's only one of a handful of crates I've seen that actually implement a chess AI.
However- I have a lot of concerns about this crate.
For starters, the history. @chase-manning made a new version of the crate (Tanton, see #140), but then shortly after seems to be the owner of Pleco. All they've done is various updates to CI and fixing warnings, which is fair enough- but nothing really substantial.
Then, there's the code.
Oh boy, there's a lot of undefined behavior and unsafe code. It's honestly quite daunting to look over it all- so specifically, I'll focus on src/movepick/mod.rs, as it's generally pretty simple.
Let's look at the struct definition for MovePicker:
That's.. a lot of pointers. I can't comment on why they're there, but I can speculate.
In MovePicker::new, there are a bunch of references passed as arguments:
pubfnmain_search(board:&Board,depth:i16,main_hist:&ButterflyHistory,// there are a lot more) -> Self
..and all of them get stored as a pointer. If I had to guess, whoever wrote the code for MovePicker did not want to deal with lifetimes. Or is a C programmer who uses Rust as if it were C.
At the least this function should be unsafe. What happens if someone passes a Board that goes out of scope?
Then there's this (also in new):
MovePicker{
pick,board:&*board,// more fieldsrecapture_sq:unsafe{ mem::MaybeUninit::uninit().assume_init()},threshold:unsafe{ mem::MaybeUninit::uninit().assume_init()},/// more fields}
Those mem::MaybeUninit::uninit().assume_init()? Insta UB. There's a reason the compiler doesn't let you use uninitialized values- when you create a value on the stack and don't initialize it, the function call will be at the deepest the stack has ever been at best, or point to some random garbage that used to be there. That's assuming that rustc will behave like a C compiler or not put variables into registers. In any case, these can create invalid values (imagine a bool that is not true or false) which can cause a lot of problems.
Further down in the code, there's a similar thing with mem::zeroed!
No comments about why these may be safe by the way!
All of this kind of code is repeated throughout the codebase- it really calls into question the mindset of the person who wrote it.
The text was updated successfully, but these errors were encountered:
I'm currently looking around for a chess AI written in rust. I found Pleco, and it seems like a neat project, it's only one of a handful of crates I've seen that actually implement a chess AI.
However- I have a lot of concerns about this crate.
For starters, the history. @chase-manning made a new version of the crate (Tanton, see #140), but then shortly after seems to be the owner of Pleco. All they've done is various updates to CI and fixing warnings, which is fair enough- but nothing really substantial.
Then, there's the code.
Oh boy, there's a lot of undefined behavior and unsafe code. It's honestly quite daunting to look over it all- so specifically, I'll focus on
src/movepick/mod.rs
, as it's generally pretty simple.Let's look at the struct definition for
MovePicker
:That's.. a lot of pointers. I can't comment on why they're there, but I can speculate.
In
MovePicker::new
, there are a bunch of references passed as arguments:..and all of them get stored as a pointer. If I had to guess, whoever wrote the code for MovePicker did not want to deal with lifetimes. Or is a C programmer who uses Rust as if it were C.
At the least this function should be unsafe. What happens if someone passes a
Board
that goes out of scope?Then there's this (also in
new
):Those
mem::MaybeUninit::uninit().assume_init()
? Insta UB. There's a reason the compiler doesn't let you use uninitialized values- when you create a value on the stack and don't initialize it, the function call will be at the deepest the stack has ever been at best, or point to some random garbage that used to be there. That's assuming that rustc will behave like a C compiler or not put variables into registers. In any case, these can create invalid values (imagine a bool that is not true or false) which can cause a lot of problems.Further down in the code, there's a similar thing with
mem::zeroed
!No comments about why these may be safe by the way!
All of this kind of code is repeated throughout the codebase- it really calls into question the mindset of the person who wrote it.
The text was updated successfully, but these errors were encountered: