Stream: t-compiler/wg-rls-2.0

Topic: Null in the client code


Kirill Bulatov (Feb 24 2020 at 16:22, on Zulip):

image.png

One of the users got this during the client update.
It got away and updated normally after a few attempts, he reported, so I could not squeeze any extra info out of him unfortunately.

But, IMO, we could process null's better and show some meaningful message instead.
I have a suspicion that this behavior is caused by this line (simply because it seems to be the only place destructuring releaseName): https://github.com/rust-analyzer/rust-analyzer/blob/58d44c6ba2de32a31a09bbcaf61365a69b69374c/editors/code/src/installation/download_artifact.ts#L18

which might be caused by the fetchArtifactReleaseInfo function returning null here: https://github.com/rust-analyzer/rust-analyzer/blob/49b9c8a0524e53f9bd75b50b6e87d7d88587629f/editors/code/src/installation/fetch_artifact_release_info.ts#L38

Or maybe I'm completely wrong and this is an artifact caused by something else.

Laurențiu Nicola (Feb 24 2020 at 16:24, on Zulip):

@Kirill Bulatov see the "Update the bin server?" thread

Kirill Bulatov (Feb 24 2020 at 16:25, on Zulip):

Interesting, a different field caused the issue here and there.

But glad that it's a single thing we need to just tolerate, thanks for the pointer.

std::Veetaha (Feb 24 2020 at 16:26, on Zulip):

I do want an assert() there ...

std::Veetaha (Feb 24 2020 at 16:27, on Zulip):

Postfix ! is nothing like unwrap, the null error may appear in any unrelated part of your code...

std::Veetaha (Feb 24 2020 at 16:28, on Zulip):

But, IMO, we could process null's better and show some meaningful message instead.

agree on that

std::Veetaha (Feb 24 2020 at 16:31, on Zulip):

Let me clarify, @Kirill Bulatov , you are right in your assumptions. fetchArtifactReleaseInfo() did return null, but we just ignored it with a postfix ! operator (which just silences the compiler and does nothing in runtime)

 const releaseInfo = (await fetchArtifactReleaseInfo(source.repo, source.file, source.version))!;

There was an assert(releaseInfo !== null) before, but we removed it...

matklad (Feb 24 2020 at 16:36, on Zulip):

the main reason to remvoe that assert was that it pulled in a 3rd party lib. Sorry if didn't clarified that initially, I'll be totally ok with adding

function checkNonNull<T>(value: T | undefined): T

function to utils.ts

Kirill Bulatov (Feb 24 2020 at 16:37, on Zulip):

Thanks for the clarification.

It's not like you've asked for my opinion, but here it is anyway :slight_smile:
I would have used neither ! nor assert and did a nullability check instead, seems like a logical way to be happy both with the compiler and with the unknown disturbances in the network, that may cause the function return null (if I got it right, we return null in the function if we fail to get the release via the network, seems like a possible enough usecase to process it instead of silencing).

Laurențiu Nicola (Feb 24 2020 at 16:38, on Zulip):

Can we show a message instead of the assert? I don't expect the LSP server name to change often, but it's data coming from GitHub and it's reasonable to handle errors and such.

std::Veetaha (Feb 24 2020 at 16:42, on Zulip):

The function returns null only when the request was successful but the artifact was not found, otherwise all network errors are considered unrecoverable and are just caught by the catch clause here, an assertion error would be caught too.

 catch (err) {
        vscode.window.showErrorMessage(
            `Failed to download language server from ${source.repo.name} ` +
            `GitHub repository: ${err.message}`
        );

        log.error(err);

        dns.resolve('example.com').then(
            addrs => log.debug("DNS resolution for example.com was successful", addrs),
            err => {
                log.error(
                    "DNS resolution for example.com failed, " +
                    "there might be an issue with Internet availability"
                );
                log.error(err);
            }
        );
        return false;
    }
std::Veetaha (Feb 24 2020 at 16:44, on Zulip):

Another approach would be to make featchArtifactReleaseInfo() return non-nullable value and throw Artifact was not found error instead of returning null.

Last update: Sep 22 2020 at 02:30UTC