Force Unwraps

[eugeneego]

Recently read the codebase and found that force unwraps are widely used for casting SourceKit response values.
It leads to dangerous unstable behavior.
Every single change in SourceKit response will crash the framework and the client application.
It doesn't feel right.

Did you plan some validation and user friendly errors?
Or maybe replacing dictionaries with strong types (like CodeCompletionItem)?

[jpsim]

Which force casts or force unwraps can fail in practice? If you point me to some concrete examples, I'd be able to understand what you're referring to as "dangerous unstable behavior".

Force casts & force unwraps are runtime assertions, where the programmer tells the compiler "despite what the type system can guarantee here, I know this cannot be nil, or this is this type". A quick audit of SourceKitten doesn't reveal any of these force casts/unwraps that can happen in practice.

I look forward to seeing concrete examples. Thanks.

[eugeneego]

For example:
https://github.com/jpsim/SourceKitten/blob/master/Source/SourceKittenFramework/File.swift#L71
https://github.com/jpsim/SourceKitten/blob/master/Source/SourceKittenFramework/File.swift#L239
https://github.com/jpsim/SourceKitten/blob/master/Source/SourceKittenFramework/File.swift#L311
Because responses are received from external SourceKit daemon they can contain any unexpected data.

[jpsim]

When the connection to sourcekit fails, SourceKit will actually log helpful messages such as "Connection interrupt", "pinging service" and "request dropped while
restoring service".

We could upgrade all send() invocations to failableSend(), but the improvements (if any) are dubious.

[eugeneego]

I'm talking about SourceKit API changes and internal errors when we could receive wrong responses.
For example "key.offset" field is expected as Int64, but got String in response instead.
Or "key.substructure" instead of array would be a dictionary by some internal SourceKit error.

Technically we are not controlling SourceKit daemon and its API and should treat all its responses as network responses that need validation.
Yeah, I know it can be some (not small) amount of work to refactor current codebase.

Also I see some PRs related to my question:

• parsing and error [WIP] Introduce SourcekitVariant #274 - looks promising
• and strong types [WIP] Typed declarations. #112 - looks abandoned :(.

[jpsim]

I'm not exactly keen on doing a bunch of work for theoretical improvements down the line when things change in an unexpected manner. If you want to produce a pull request that helps along this area without making the current code either less performant or less maintainable, I support you 100%.

[eugeneego]

Thanks.
I'll look for #274 and try to implement strongly typed API.

[owensd]

Here's an example of one that fails:

fromSourceKit(_ sourcekitObject: sourcekitd_variant_t) return a SourceKitRepresentable?. One of the cases looks like this:

    case SOURCEKITD_VARIANT_TYPE_NULL:
        return nil

This is called from here:

    public func send() -> [String: SourceKitRepresentable] {
        initializeSourceKit
        let response = sourcekitd_send_request_sync(sourcekitObject)
        defer { sourcekitd_response_dispose(response!) }
        return fromSourceKit(sourcekitd_response_get_value(response!)) as! [String: SourceKitRepresentable]
    }

This crashes for me when I'm trying to use the --spm-module argument. I have no idea why the argument is failing because it works from the command line, but it crashes here. I've already spent a few hours trying to figure out why this is failing, but no luck yet.