Use boost::filesystem::path in readFileAsString()

[cameel]

Small refactor of readFileAsString() requested in #11544 (comment).

Originally the function accepted the path as std::string, now it's boost::filesystem::path.

[cameel]

While updating this I noticed that solidity-upgrade has the file processing logic duplicated from CommandLineInterface and now, after the recent round of CLI refactors, it's out of sync. #11651.

[axic]

I think this may have been a conscious decision to keep boost's presence limited. I guess now there's an implicit constructor from string in cases it does not receive a path? Does having boost/filesystem in CommonIO.h gives any measurable compilation time effect? Also note that all the other path helpers in CommonIO.h such as absolutePath and sanitizePath still use strings, so one should decide whether we go all-in on boost::filesystem and change all of these, but should avoid the case where we have a mix.

I am not campaigning against this, but the change seems to have some other cleanups, and in the end we trade a single .string() at the caller side for using .string() four times within the helper.

[christianparpart]

I think this may have been a conscious decision to keep boost's presence limited. I guess now there's an implicit constructor from string in cases it does not receive a path? Does having boost/filesystem in CommonIO.h gives any measurable compilation time effect? Also note that all the other path helpers in CommonIO.h such as absolutePath and sanitizePath still use strings, so one should decide whether we go all-in on boost::filesystem and change all of these, but should avoid the case where we have a mix.

I am not campaigning against this, but the change seems to have some other cleanups, and in the end we trade a single .string() at the caller side for using .string() four times within the helper.

I partly agree here, @axic. I do remember though that I was once motivated to use boost::filesystem::path over std::string. I do not like boost and was once pushing for std::filesystem, but that wasn't possible at that time, because OS/X was lagging behind. Maybe that has changed? Actually it has, but what OS/X versions do we want to support?

I however prefer (std::)filesystem::path usage, as I at least assumed the string ctor is not implicit (maybe a boost-only thing?)

[christianparpart]

I would have actually approved this PR, but your comment made me hold back, axic. Maybe we want to re-evaluate the use of std::filesystem again?

[cameel]

I guess now there's an implicit constructor from string in cases it does not receive a path?

Yeah. path -> string conversion is unambiguous so it's implicit. It's the other way around where there are several possibilities (string(), generic_string(), native()).

Does having boost/filesystem in CommonIO.h gives any measurable compilation time effect?

I doubt it because it's not a header-only library. I did not notice any egregious change but I'd have to measure it to be sure how much it adds.

Also note that all the other path helpers in CommonIO.h such as absolutePath and sanitizePath still use strings, so one should decide whether we go all-in on boost::filesystem and change all of these, but should avoid the case where we have a mix.

I saw them and I left them unchanged on purpose. They are not generic path functions, they're meant for our VFS paths with all the quirks like ../x.sol relative to /a/b/../ resolving into /a/b/x.sol. We should not put that into path. I think that having them in CommonIO is highly misleading and I was going to move them to ImportRemapper or CompilerStack or a new VFS-specific class when I get to changing that part.

I am not campaigning against this, but the change seems to have some other cleanups, and in the end we trade a single .string() at the caller side for using .string() four times within the helper.

For me it's about the interface. I highly prefer to see a path type where an actual path is expected (could be std::filesystem::path but for now we only have boost::filesystem::path). The fact that we have internal VFS paths that work kinda like paths but not quite is bad enough and we should clearly distinguish them. I'm also not sure we can really always treat path -> string -> path conversion that is required otherwise as lossless.

I would have actually approved this PR, but your comment made me hold back, axic. Maybe we want to re-evaluate the use of std::filesystem again?

I think it's risky. There are subtle differences between the two that affect normalization (see for example boostorg/filesystem#88 (comment)) and I think we do not have enough test coverage related to paths to just swap one with another and expect things not to be broken in some way. The new stuff I'm adding is covered pretty extensively but the existing code mostly is not.

[leonardoalt]

@axic @christianparpart any conclusions here after @cameel 's comment?

[chriseth]

I'm fine with actual filesystem functions using boost::filesystem

[chriseth]

Can we get this merged?