but first I'd like to know if this is desired, and if I'm not missing something important.

No, for two reasons, 1) to the professional intensive care unit people, your abort() is equivalent to a segfault, so none of this solves their problem(until they write to agree that we can perform a controlled crash with invalid inputs now), 2) before, people can yolo through some of the less harmful invalids without any problem, now they can't and they will anyways never figure out how to write a proper sanitizer, so in a sense this just made it more punishing to the noobs.

You did not miss anything "important", as it is obvious that it is nothing complicated to filter out positions that will crash SF, however it is complicated to define what is allowed afterwards, do you allow for anything that passes your filter, or is it still "legal positions reachable from startpos"? If the former, are the conditions subject to change or permanent?

Any above errors are handled by calling std::terminate. The motivation for such behaviour is lack of specification from the UCI protocol. UCI allows ignoring errors, but ignoring errors never a good idea as it leads to differences between expected and actual state. Crashing is the only responsible thing we can do in this case.

You are not ignoring the error if you are sending an info string message to the user (which can contain an explanation of what is wrong with the FEN).

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

The UCI protocol is not totally immutable and can evolve. For example UCI_Variant has been added in this way.

before, people can yolo through some of the less harmful invalids without any problem, now they can't

Why do we suddenly care about usage that violates preconditions?

You did not miss anything "important", as it is obvious that it is nothing complicated to filter out positions that will crash SF, however it is complicated to define what is allowed afterwards, do you allow for anything that passes your filter, or is it still "legal positions reachable from startpos"? If the former, are the conditions subject to change or permanent?

Crashing aside, this PR's purpose is to ensure there is no differences in observed and assumed state of the engine by the user - where sometimes input is ignored and sometimes everything can happen.

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

Why do we suddenly care about usage that violates preconditions?

Which precondition? I'm kinda lost in the chain of cause and consequences.

Why do we suddenly care about usage that violates preconditions?

Which precondition? I'm kinda lost in the chain of cause and consequences.

The precondition that positions passed to the engine must be reachable from startpos

The precondition that positions passed to the engine must be reachable from startpos

And why are we starting to care about usage that violates it?

The precondition that positions passed to the engine must be reachable from startpos

And why are we starting to care about usage that violates it?

you suggested such a use case as a reason to dismiss this PR

you suggested such a use case as a reason to dismiss this PR

If you argue that the outcome of such violations of precondition is not of concern, then I think we also won't need this PR to deal with the outcome of such violations of precondition to begin with. Whether it resulted in "happens to work" or "crashing" is irrelevant to the problem.

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

I am not getting your point. SF could emit a message like

info string Invalid Position. <further expanations>

The GUI could recognize the "Invalid Position" part and inform the user of the error (possibly with the further explanations). In that way SF and a compliant GUI would not get out of sync.

This is BTW the way the xboard protocol handles it.

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

I am not getting your point. SF could emit a message like

info string Invalid Position. <further expanations>

The GUI could recognize the "Invalid Position" part and inform the user of the error (possibly with the further explanations). In that way SF and a compliant GUI would not get out of sync.

This is BTW the way the xboard protocol handles it.

And what happens? What's the state after the error?

And what happens? What's the state after the error?

I would presume that the program gracefully exits, or at least this is the most accepted opinion in #4558. To the GUI and user itself it won't make much of a difference though.

The fact that GUIs can currently not parse such a message is a valid concern. But I am sure that if SF commits to a particular error message format, GUIs using SF will start to parse it. Then others engines will follow the lead.

Are you willing to specify the behaviour and state guarantees for every such case, for the other engines and GUIs to implement?

I am not getting your point. SF could emit a message like

info string Invalid Position. <further expanations>

The GUI could recognize the "Invalid Position" part and inform the user of the error (possibly with the further explanations). In that way SF and a compliant GUI would not get out of sync.
This is BTW the way the xboard protocol handles it.

And what happens? What's the state after the error?

Currently the GUI and the engine would be out of sync (which is not a fundamental problem since the GUI should not trust the engine output anyway, but it might be confusing for the user, although likely not since there is the info string message which the GUI has shown). Once GUIs parse the error message sent by the engines (like in the xboard case) this issue would go away,

I don't think this is currently a serious issue though since at this time GUIs should be somewhat conservative in the positions they send to UCI engines, since they have no way of knowing what positions the engine can handle. Indeed the party that knows best which positions an engine can handle is of course the engine itself and not the GUI. This is the philosophy of the xboard protocol and it makes a lot of sense IMHO.

The best way to change the UCI protocol is to set the tone and return info error INVALID_FEN. Maybe everyone else follows :)

The best way to change the UCI protocol is to set the tone and return info error INVALID_FEN. Maybe everyone else follows :)

I think it is not a good idea to make an incompatible change. The GUIs will ignore the info error message (as it is not valid UCI) and so no one will see it.

As I noted elsewhere, we already have similar actions relating to the net:

Crashing aside, this PR's purpose is to ensure there is no differences in observed and assumed state of the engine by the user - where sometimes input is ignored and sometimes everything can happen.

To be very honest, I agree that removing undefined behavior is a good idea. I'm for this proposed change.

However, might I suggest this be released as part of a major release, being a backwards compatibility breaking change (it can't be considered as a bug fix)? I'm not sure if Stockfish even cares about semantic versioning — but it's a nice thing for the end users.

Stockfish doesn't use semantic versioning so there's no way to indicate a breaking change like that. It is also not a breaking change, in a sense, since inputs that would be experiencing a change would have been already violating the precondition.

now. exit(EXIT_FAILURE) or std::terminate?

Stockfish doesn't use semantic versioning so there's no way to indicate a breaking change like that. It is also not a breaking change, in a sense, since inputs that would be experiencing a change would have been already violating the precondition.

now. exit(EXIT_FAILURE) or std::terminate?

std::terminate is obviously the right way to go, and the modern C++ way.

However, such a change should be reflected in all parts of Stockfish's code. If that cannot be done and the other parts of the code use exit(EXIT_FAILURE), then it's best to follow the project's convention.

why not throw a runtime error?

why not throw a runtime error?

You mean without crashing?

why not throw a runtime error?

You mean without crashing?

throw std::runtime_error("Unexpected fen, failed to parse: ....");

kills the program unless a try and catch block surrends it.

I think it is worth noting that restricting the engine to positions with 16 pieces for each side does limit some use cases for it. I've seen many crazy chess puzzles involving more than 32 pieces, and it would be a bit unfortunate to not be able to run Stockfish on those special cases.

Well, that's the mess you started. Screaming wolf has consequences.

Regardless, as per @Sopel97's comment:

inputs that would be experiencing a change would have been already violating the precondition.

This means that the puzzles/challenges you speak of already violate the precondition (that only positions reachable from the starting position are acceptable ... i.e. limited to 32 pieces). So yes, they'll be experiencing a change. This removes undefined behavior, and completely stamps down on the "Stockfish is a chess engine, not imaginary chess engine."

I think it is worth noting that restricting the engine to positions with 16 pieces for each side does limit some use cases for it. I've seen many crazy chess puzzles involving more than 32 pieces, and it would be a bit unfortunate to not be able to run Stockfish on those special cases.

We have no way to guarantee quality analysis for such positions, so it would be dubious even if we supported them. Look at how Stockfish does in matches with odds for example. There are engines better suited for analysing non-chess positions.

I think it is worth noting that restricting the engine to positions with 16 pieces for each side does limit some use cases for it. I've seen many crazy chess puzzles involving more than 32 pieces, and it would be a bit unfortunate to not be able to run Stockfish on those special cases.

The fallout of all this will be Stockfish will be only for the purists. If anyone wants to do something thats illegal in Standard or FRC, they need to fork and make the changes themselves or use some other alternative.
If anything apart from that is allowed, there will be someone else creating a mess here or reddit or fishcooking or someplace else :)

I think it is worth noting that restricting the engine to positions with 16 pieces for each side does limit some use cases for it. I've seen many crazy chess puzzles involving more than 32 pieces, and it would be a bit unfortunate to not be able to run Stockfish on those special cases.

Stockfish already can't guarantee quality analysis of such positions due to limitations imposed for performance reasons. Not allowing people to feed such positions to Stockfish and then have them make clickbait YouTube videos about how stockfish is blind is actually a good thing. Use fairy sf or stockfish mv for such positions, anyway.

Probably this comment has been made before. Although UCI is supposed to be stateless, there is still a little bit of state introduced by the setposition command.

So it is seems to be ok that the engine, after receiving a setposition command it does not want to handle, refuses to execute certain commands (e.g. returning bestmove null on go) until a new valid setposition command is received.

People have argued (including me) that UCI mandates that the engine should simply ignore an invalid setposition commands. Now I don't support this opinion anymore.

Moved the actual error handling to the UCI layer. Position::set now returns the error. Updated the comment on Position::set.

This is close to the final form. Sanity check: https://tests.stockfishchess.org/tests/view/6461f8ae87f6567dd4df2ad8

I settle on crashing. Per UCI spec:

• if the engine or the GUI receives an unknown command or token it should just ignore it and try to
  parse the rest of the string in this line.
  Examples: "joho debug on\n" should switch the debug mode on given that joho is not defined,
  "debug joho on\n" will be undefined however.

incorrect arguments result in undefined behaviour. The "ignore" clause only applies to commands.

One little issue I have is that the final error message does not seem to say which command is actually the culprit of the error. I would prefer something like Invalid command (setposition): <message> instead of the generic "CRITICAL ERROR".

Although it seems the idea is regarded as controversial, I would still like to make it possible for the GUI to parse the error message in a simple way if it wants to do this (rather than just relaying the message to the user).

One little issue I have is that the final error message does not seem to say which command is actually the culprit of the error. I would prefer something like Invalid command (setposition): instead of the generic "CRITICAL ERROR".

It does have a somewhat granular error message, though for batched commands it could be even more precise. I'll see if it can be done in a reasonable way.

Although it seems the idea is regarded as controversial, I would still like to make it possible for the GUI to parse the error message in a simple way if it wants to do this (rather than just relaying the message to the user).

I'd like to see UCI specification first. Then we can implement it in Stockfish.

I'd like to see UCI specification first. Then we can implement it in Stockfish.

Sure but the UCI specification is essentially frozen. The only way to make it move is to implement something reasonable and then hope it is copied (this is what happened with UCI_variant). Anyway my opinion on this is known.

The only way to make it move is to implement something reasonable and then hope it is copied

I'd like to avoid implementation defined behaviour

The whole command that failed is now printed along the error message.

Not a big deal, and maybe even a little bit nitpicky, but from my limited testing:

• when compiled using make build

position fen 7k/7p/8/8/8/8/7P/K6K w - - 0 1 
info string CRITICAL ERROR: Command `position fen 7k/7p/8/8/8/8/7P/K6K w - - 0 1 ` failed. Reason: Invalid FEN.

terminate called without an active exception
Aborted

• when compiled using make build debug=yes

position fen 7k/7p/8/8/8/8/7P/K6K w - - 0 1 
stockfish: position.cpp:1428: bool Stockfish::Position::pos_is_ok() const: Assertion `0 && "pos_is_ok: King count"' failed.
Aborted

I think the explicit crash with the info string was good but it would be nice to also get a more specific message without recompiling if possible.

I think the explicit crash with the info string was good but it would be nice to also get a more specific message without recompiling if possible.

With debug=yes it leaks implementation details and suggests to the user it is a bug in Stockfish.

I think the explicit crash with the info string was good but it would be nice to also get a more specific message without recompiling if possible.

With debug=yes it leaks implementation details and suggests to the user it is a bug in Stockfish.

I agree we shouldn't leak implementation details. I was thinking more of, for example, adding the "king count" part to the info string message rather than "it's in pos.is_ok()". Although I don't immediately see an easy way to change it.

From the user's point of view, it seems inconsistent that some info string errors give details about what's wrong with the position, and some just say it's incorrect.

So, what's the consensus about this pull request?

I think we should merge something along these lines, but was holding off until after the SF16 release, and busy afterwards. Probably, this could be revisited now as this gives some time to fix-up eventual fallout.

In the back of my mind, I have been thinking also that there is possibly some overlap with #4626 or said differently, if we move in the direction of having a shared library of SF, we will have to think a bit more about error handling (i.e. a library should not abort).

I agree with merging, though we should keep it rather simple in the future otherwise people will constantly try to add new rules to prohibit non legal chess positions. I think all we really care about is avoiding positions where a crash can happen (or which could later cause a crash).
Btw needs a rebase ; )