Improve FileSystemError by adding associated path #3055
Conversation
MaxDesiatov
requested review from
abertelrud,
aciidgh,
friedbunny and
neonichu
as code owners
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| public class InMemoryFileSystem: FileSystem { | ||
|
|
||
| /// Private internal representation of a file system node. | ||
| /// Not threadsafe. |
There was a problem hiding this comment.
These differences are all from syncing the vendored TSC, right? For historical reasons, it would be clearer to make the vendoring sync be a separate commit, I think.
There was a problem hiding this comment.
The vendoring sync is already separate in 27bd6a8, or is there anything that I missed?
There was a problem hiding this comment.
Ah, right: it was a mixture of familiar and unfamiliar diffs, but that was only because the new diffs are actually in TSC and so are being vendored along with the previous diffs. I see what my mistake was there. Please ignore the comment, I just misunderstood (I think I also expected the commit to be something like "syncing TSC" rather than "Improve FileSystemError by adding associated path", which made me think that it was something more than vendoring.
Maybe just squashing the new diffs here (unless you want them in the history) so that this can be merged without squashing would be good. Basically, at the end it would be good to end up with one diff in the final history for vendoring TSC and another for the SwiftPM diffs. Sorry for the extra trouble.
There was a problem hiding this comment.
No problem, thanks for the clarification. I've squashed the last two commits and changed the title of the first one to mention the vendored TSC copy sync. Let me know if anything else is needed here.
| /// True if there were any IO error during writing. | ||
| private var error: Bool = false | ||
| /// Set to an error value if there were any IO error during writing. | ||
| private var error: FileSystemError? |
There was a problem hiding this comment.
This is such an improvement. Thanks!
|
Also, I think the tests will keep failing until the TSC changes are merged, since at least some of the testing uses the actual TSC and not the vendored copy, I think. |
|
Yes, self-hosted tests are using the main branch of TSC and do not support cross-repository testing. These jobs are optional, though, so not relevant for merging any PRs. |
I've recently stumbled upon this error when running `swift build`: ``` error: noEntry ``` This is the exact and only output of this `swift build` invocation. Unfortunately, I think it's impossible to diagnose without an attached debugger. It's also hard to pinpoint where exactly the error comes from in the source code, since `FileSystemError.noEntry` is thrown from multiple places. In my opinion, for an error value to be helpful it should have associated information attached to it. In this case, it would make sense for almost all cases of `FileSystemError` to have an associated `AbsolutePath` value. `FileSystemError` now also has to be explicitly declared `Equatable` to make the tests compile, but previously it was `Equatable` anyway, although implicitly by virtue of being an enum with no associated values.
MaxDesiatov
force-pushed
the
maxd/fserror
branch
from
6506c84 to
164476a
Compare
|
swiftlang/swift-tools-support-core#167 |
|
Nice, thank you! This looks good to me! |
Mirrors swiftlang/swift-tools-support-core#167 change to the TSC package.
I've recently stumbled upon this error when running
swift build:This is the exact and only output of this
swift buildinvocation. Unfortunately, I think it's impossible to diagnose without an attached debugger. It's also hard to pinpoint where exactly the error comes from in the source code, sinceFileSystemError.noEntryis thrown from multiple places.In my opinion, for an error value to be helpful it should have associated information attached to it. In this case, it would make sense for almost all cases of
FileSystemErrorto have an associatedAbsolutePathvalue.FileSystemErrornow also has to be explicitly declaredEquatableto make the tests compile, but previously it wasEquatableanyway, although implicitly by virtue of being an enum with no associated values.