Is there any way to enforce this without looking at the code with human eyes? Are the tests going to fail if we switch to std filesystem and have an incorrect string conversion?

Documentation review ACK 9b575f1

Are the tests going to fail if we switch to std filesystem and have an incorrect string conversion?

Yes, especially the Windows tests will fail, as the situation is most dire there. Both the functional and unit tests, as they both have some tests that use unicode characters in paths (that are not in any of the "narrow" codepages).

ACK 9b575f1

re: #23522 (comment)

Is there any way to enforce this without looking at the code with human eyes? Are the tests going to fail if we switch to std filesystem and have an incorrect string conversion?

I think ideally PathToString and PathFromString functions would be eliminated. I wrote these functions to allow switching from boost::filesystem to std::filesystem while preserving current behavior, but current behavior is actually a little broken.

PathToString is mostly used for logging, but this is not ideal because the result is usually not quoted, and paths can contain spaces, quotes, punctuation and text that make resulting log messages ambiguous. They could also contain arbitrary characters including newlines, invalid UTF-8 sequences, invalid multibyte sequences that break log parsing. I think ideally these PathToString calls would be replaced with calls to a new PathToLogString function, and that function would add surrounding quotes around paths, replace internal quotes with \", replace special ASCII characters like tabs and newlines with C \t \n escapes, replace non-latin unicode characters with \uXXXX escapes on windows, and replace non-latin bytes with \xXX escapes on POSIX.

PathFromString is mostly used to parse command line and configuration arguments. I think ideally these calls would be removed and replaced with calls to a new ArgsManager::GetPathArg method. The method would treat command line arguments and configuration file paths as bytes on POSIX with no encoding or decoding. On windows, it would decode configuration file paths as UTF-8, and it would either decode command line arguments according to the current code page, or it would bypass the code page encoding and decoding done by the windows C library by overriding wmain and getting the original unicode arguments passed by the parent process in wchar_t *argv[]

So ideally we could get rid of PathToString to PathFromString entirely. But more narrowly if we just wanted to try minimize new encoding bugs and avoid PathToString results being passed to the UniValue constructor, we would need to use distinct string types like std::string for byte strings and std::u8string for unicode strings and make it an error to construct a UniValue from a byte string. We could also try to renaming the PathToString function to discourage use. Maybe to something like PathToByteString, PathToInternalString, PathToSystemString, LegacyPathToString

Thanks for all reviews and thanks @shaavan for suggested edits. I didn't get to apply them here, but they could be used in another followup or maybe #20744