Improve FileSystemError by adding associated path #167
Conversation
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.
|
@swift-ci please test |
There was a problem hiding this comment.
This is great! The changes in this PR so far look good to me. It's unfortunate to have to switch to a struct, but adding a path to each enum case would be even more cumbersome, I think. There might also be cases in which a client might want to access the path without having to check each specific code, so this makes sense.
| /// path already contains a file or folder. | ||
| case alreadyExistsAtDestination | ||
| public struct FileSystemError: Swift.Error, Equatable { | ||
| public enum Kind: Equatable { |
There was a problem hiding this comment.
It's unfortunate to have to split this into a kind and a path, I think, but adding a path to every enum case would be even more unfortunate.
| /// to. | ||
| /// | ||
| /// Used in situations that correspond to the POSIX ENOENT error code. | ||
| case noEntry |
There was a problem hiding this comment.
Do we want to record any information here about whether symlink traversal was in effect when the error occurred, e.g. if I'm accessing path where a symlink is the last path component, I would get this error if a symlink was being resolved but not get it if it wasn't. This is known at the callsite but cannot be inferred from the error. I suppose that is actually something that could apply to any of the errors here.
There was a problem hiding this comment.
How would you prefer to record that? A separate property on this new struct?
There was a problem hiding this comment.
I'm not sure, actually — at first I was thinking a boolean on each of the enum cases where it made sense, but perhaps it would belong as a separate boolean next to the path. But this could easily be added later in a separate PR, I think.
MaxDesiatov
requested review from
aciidgh,
friedbunny and
neonichu
as code owners
|
This seems to have broken the Windows build :-( Can we revert this? |
|
How hard would it be to have a look at a build log with the error message? I'm happy to create a separate PR with a fix for Windows if that would be acceptable instead of the revert? |
|
@MaxDesiatov if it can be merged immediately, sure I suppose that might work, but changes like this should really be tested before merging - changes to the file system codepaths are very sensitive since the file system implementation in TSC really needs to abstract over the underlying system and does not do so. |
|
I'd love to be able to test it, is there any plan to set up Windows CI for SwiftPM? |
|
@tomerd was going to look into that I believe - there is already a periodic CI: https://dev.azure.com/compnerd/swift-build/_build?definitionId=57 |
|
Thanks! The other complication is that this is a cross-repo PR that needs to be reverted here, in SwiftPM, and SourceKit-LSP. I'll submit revert PRs in appropriate repos then... |
@compnerd I tried to have a look, but constnantly getting this error: |
|
Interesting, that is new, Ill take a look at the settings - its supposed to be visible to everyone; in any case put up #169 |
TSCBasic: repair Windows build after #167
Another attempt to apply #78, which was previously reverted due to breakage in SourceKit-LSP.
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.