Never return nil for position conversions
#1168
Conversation
|
@swift-ci Please test |
| @@ -56,10 +56,9 @@ public struct SourceKitdRequestCommand: AsyncParsableCommand { | |||
| let requestInfo = try RequestInfo(request: requestString) | |||
|
|
|||
| let lineTable = LineTable(requestInfo.fileContents) | |||
| if let offset = lineTable.utf8OffsetOf(line: line - 1, utf8Column: column - 1) { | |||
There was a problem hiding this comment.
I'm not sure it's worth it, but maybe we should keep a nil returning variant? In this case in particular, it would make sense to display an error about invalid line/col (though... we're currently just ignoring it anyway).
| let start = snapshot.positionOf(zeroBasedLine: line - 1, utf8Column: column - 1) | ||
| let end = snapshot.positionOf(zeroBasedLine: endLine - 1, utf8Column: endColumn - 1) |
There was a problem hiding this comment.
And in this case I really don't think it makes sense to do closest match (really anywhere we're making modifications), so... I'm more leaning towards having nil variants 😅
There was a problem hiding this comment.
I would prefer to not have nil returning position conversion variants. The cases such a variant would catch are:
- If we have some offset error in the line and the token to rename happens to be right at the beginning or end of the file so that the line kicks it out of the valid line range. But then rename in the middle of the file is already very broken so I doubt that it would help much
- Same argument for column indices.
We already have so many position conversion functions and I don’t want to add even more. Actually, I think it’s more likely that we would end up calling the nil returning variant somewhere in the future where recovery would have been sensible. And, in practice I’ve never seen us hit position conversion to fail, and the code is easier to maintain if you don’t have to deal with failed conversions.
Instead of returning `nil` to indicate that the position conversion failed, log a fault and perform a best-effort recovery. I think this allows us to perform better recovery and also makes code calling these position conversions a lot simpler because it doesn’t need to make decisions about what to do if position conversions fail.
ahoppen
force-pushed
the
never-fail-position-conversions
branch
from
ffc0c9d to
08f1595
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Linux |
1 similar comment
|
@swift-ci Please test Linux |
Instead of returning
nilto indicate that the position conversion failed, log a fault and perform a best-effort recovery.I think this allows us to perform better recovery and also makes code calling these position conversions a lot simpler because it doesn’t need to make decisions about what to do if position conversions fail.