Deserialize diagnostics with no location correctly #213
Conversation
owenv
requested review from
abertelrud,
aciidgh,
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please test |
There was a problem hiding this comment.
Looks good to me, but had one question about whether it's the filename or the whole source location that should be optional, semantically.
BTW, I'm really hoping to soon be able to start making use of this native ability to deserialize diagnostics files in SwiftPM. There are places where it's looking at the stderr of the compiler output and it's going to be great to start cleaning those up!
| @@ -185,7 +185,7 @@ extension SerializedDiagnostics { | |||
|
|
|||
| public struct SourceLocation: Equatable { | |||
| /// The filename associated with the diagnostic. | |||
| public var filename: String | |||
| public var filename: String? | |||
There was a problem hiding this comment.
What do the line and column mean (semantically) in the absence of a filename? If the .dia still has those fields without the filename then this does seem like the right change, but what should an IDE (for example) do with such a source location?
There was a problem hiding this comment.
Or should this be modeled so that it's the SourceLocation itself that is optional? (even if that's indicated by just a nil filename field in the file).
There was a problem hiding this comment.
I updated this to just make the entire SourceLocation optional. Currently, Swift uses the absence of a filename to indicate either a missing or invalid location, and it's always paired with a line/column of 0. I'm fairly certain diagnostics formatted from Clang will look the same, but even if it encodes additional info there's really nothing an IDE can do to present that in a useful way.
There was a problem hiding this comment.
That makes a lot of sense to me. Thanks!
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
@swift-ci please test Linux |
|
@swift-ci please test Linux |
|
@owenv do you want to cherry pick this into release/5.5 too? |
|
@tomerd thanks for merging! None of the higher-level tools use this code on the 5.5 branches, so I don't think it's worth cherry-picking. |
When a diagnostic doesn't have a source location, no filename record will be emitted. This PR makes the filename of
SourceLocationoptional so these can be deserialized successfully.