Alternative aproach to plugin auto update #3629

matklad · 2020-03-17T23:02:21Z

This is very much WIP (as in, I haven't run this once), but I like the result so far.

The primary focus here on simplification:

local simplification of data structures and control-flow: using union of strings instead of an enum, using unwrapped GitHub API responses
global simplification of control flow: all logic is now in main.ts, implemented as linear functions without abstractions. This is stateful side-effective code, so arguments from Carmack very much apply. We need all user interractions, all mutations, and all network requests to happen in a single file.
as a side-effect of condensing everything to functions, we can get rid of various enums. The enums were basically a reified control flow:

enum E { A, B }

fn foo() -> E {
    if cond { E::A } else { E::B }
}

fn bar(e: E) {
    match e {
        E::A => do_a(),
        E::B => do_b(),
    }
}

==>>

fn all() {
    if cond { do_a() } else { do_b() }
}

simplification of model: we don't need to reinstall on settings update, we can just ask the user to reload, we don't need to handle nightly=>stable fallback, we can ask the user to reinstall extension, (todo) we don't need to parse out the date from the version, we can use build id for nightly and for stable we can write the info directly into package.json.

editors/code/src/net.ts

editors/code/src/main.ts

Veetaha

All the bloat has gone away...

Downgrading to stable is tricky. nightly has a greater semver version and vscode doesn't delete extension bundle when you uninstall it right away. Thus, when the user clicks uninstall and install, vscode looks into the cache and sees that it already has the version that is greater than the one on the marketplace and just resurrects the already installed nightly back. Though I am not sure that this is true.

Veetaha · 2020-03-18T00:04:13Z

editors/code/src/config.ts

-    private get serverPath() { return this.cfg.get("serverPath") as null | string; }
-    get updatesChannel() { return this.cfg.get("updates.channel") as UpdatesChannel; }
+    get serverPath() { return this.cfg.get("serverPath") as null | string; }
+    get channel(): "stable" | "nightly" { return this.cfg.get("updates.channel") ?? "stable"; }


Suggested change

get channel(): "stable" | "nightly" { return this.cfg.get("updates.channel") ?? "stable"; }

get channel() { return this.cfg.get("updates.channel") as UpdatesChannel; }

The default stable should be returned due to the declaration in package.json configurations contribution point.
And I'd like to preserve the explicit as cast, since this is a downcast any -> UpdatesChannel

And I'd like to preserve the explicit as cast, since this is a downcast any -> UpdatesChannel

The type of get is

get<T>(section: string): T | undefined;

So, this is explicitely to get rid of |undefined I guess an ! would also work?

I guess we should change all other configs to assert non undefined instead of as, but that's for other PR

Let me elaborate here. The design of this get<T>() method is quite unsound. It lets you pass a type argument that will denote its return type.

const string | undefined = cfg.get<string>("name");

But that is very deceiving. By looking at this code you won't expect it to fail here since there is nothing unsound in it at the first glance.

However, what happens under the hood is the unchecked downcast any -> T.
Here in vscode impl you can see that this function returns a variable of type any:

And any is considered harmful, since it is implicitly convertible to literally any type (instead of any it is now recommended to use unknown).

So the user does can pass a value of some other type other than declared in our json schema in package.json and this will fail at runtime, i.e.: see this comment
https://github.com/rust-analyzer/rust-analyzer/blob/2a3543d1953daeb240c214ececd273f1040b0516/editors/code/src/config.ts#L186-L187

My concern is to mark such unsound casts explicitly with as instead of silently passing the type arguments to the function that does an implicit cast under the hood so we don't even notice the unsoundness.

I know you are used to trusting vscode guys, but I don't. They are the same people as we and they do the same mistakes as we too and this is one of their bad design decisions because the proper signature for this function would be

get(opt: string): unknown;

or they should throw an error if the input doesn't match the declared json schema type in package.json.

Anyway, I'll at other the code later today!

Note: type arguments, ! null assertion and as T casts have no influence on the runtime behavior. They are just to silence the compiler complaints.

This is out of scope of this PR, but I'd prefer to just run with upstream API. If they cast any to T, then they probably have the reasons to do so. In general, VS Code code base is of impecable code quality, so, in general, I trust them blindly when it comes to TypeScript.

For the case of settings in the editor, I independently think that lenient validation is the right approach. It's more important that the thing works with broken settings, than to bail loudly on errors, in this context.

matklad · 2020-03-18T11:32:53Z

Should be ready for review now!

r? @Veetaha

matklad · 2020-03-18T13:11:43Z

#3635 further simplifies tag discovery

Veetaha · 2020-03-18T21:24:07Z

.vscode/launch.json

      "request": "launch",
      "runtimeExecutable": "${execPath}",
      "args": [
+        // "--user-data-dir=${workspaceFolder}/target/code",


What is this option for? Does this overwrite the globalStoragePath?

Yup, this overrides storage path, extensions and settings, and it makes it easier to look inside the store. It’s deliberately commented out, so that it can be easily restored when needed

Veetaha · 2020-03-18T21:25:56Z

editors/code/src/commands/server_version.ts

-        }
-
-        const version = spawnSync(binaryPath, ["--version"], { encoding: "utf8" }).stdout;
+        const version = spawnSync(ctx.serverPath, ["--version"], { encoding: "utf8" }).stdout;


Maybe we should still use getServer() thing here?

This will require adding persistentState to ctx, which I’d rather avoid.

Veetaha · 2020-03-18T21:27:31Z

editors/code/src/config.ts

-    Stable = "stable",
-    Nightly = "nightly"
-}
+export type UpdatesChannel = "stable" | "nightly";


Good thing that TypeScript allows for stringly-typed apis, though I prefer enums, but no too strong arguments.

Yep, makes sense, will update tomorrow

Veetaha · 2020-03-18T21:31:33Z

editors/code/src/config.ts

-    private get serverPath() { return this.cfg.get("serverPath") as null | string; }
-    get updatesChannel() { return this.cfg.get("updates.channel") as UpdatesChannel; }
+    get serverPath() { return this.cfg.get("serverPath") as null | string; }
+    get channel(): "stable" | "nightly" { return this.cfg.get("updates.channel", "stable"); }


See my comments on the outdated code #3629 (comment).
And there is no need to specify the default stable. This is what I tried to eliminate in #3131, the second source of truth!

Veetaha · 2020-03-18T21:33:36Z

editors/code/src/ctx.ts

    private constructor(
        readonly config: Config,
-        readonly state: PersistentState,
+        // readonly state: PersistentState,


If we won't use getServer() in the command handler, then we don't need this comment

Suggested change

// readonly state: PersistentState,

Veetaha · 2020-03-18T21:35:50Z

editors/code/src/main.ts

+            vscode.window.showWarningMessage(`You are running a nightly version of rust-analyzer extension.
+To switch to stable, uninstall the extension and re-install it from the marketplace`);


I'd suggest testing that this does work. I.e. uninstalling and installing does switch us to stable.
(see my previous review summary comment #3629 (review))

yup, verified manually that it works.

Veetaha · 2020-03-18T21:38:24Z

editors/code/src/main.ts

+    };
+
+    const lastCheck = state.lastCheck;
+    const now = new Date().getTime();


Suggested change

const now = new Date().getTime();

const now = Date.now();

to your service )

Veetaha · 2020-03-18T21:39:17Z

editors/code/src/main.ts

+        return;
+    };
+
+    const lastCheck = state.lastCheck;


Suggested change

const lastCheck = state.lastCheck;

const { lastCheck } = state;

This looks more cyberpunky

Veetaha · 2020-03-18T21:42:29Z

editors/code/src/main.ts

+    if (!shouldDownloadNightly) return;
+
+    const release = await fetchRelease("nightly").catch((e) => {
+        log.error(e);


Suggested change

log.error(e);

log.downloadError(e);

Hm, no, we only need to show message if this was not an auto-update attempt.

Veetaha · 2020-03-18T21:44:44Z

editors/code/src/main.ts

+    const dest = path.join(config.globalStoragePath, "rust-analyzer.vsix");
+    await download(artifact.browser_download_url, dest, "Downloading rust-analyzer extension");
+
+    await vscode.commands.executeCommand("workbench.extensions.installExtension", vscode.Uri.file(dest));


Either we should use vscodeInstallExtension() from util.ts or remove that code from there.

Veetaha · 2020-03-18T22:01:39Z

editors/code/src/main.ts

+        if (state.releaseId === undefined) { // Show error only for the initial download
+            vscode.window.showErrorMessage(`Failed to download rust-analyzer nightly ${e}`);
+        }
+        return undefined;


We should definitely be consistent on using undefined vs null. I know that e.g. TypeScript compiler team never uses null and dart guys always use it , but there is no general opinion on this question and I don't have one too. Though, should we choose one and stick with it?

undefined it is: when in doubt, do what github.com/Microsoft/vscode-languageserver-node/ does

Veetaha · 2020-03-18T22:03:55Z

editors/code/src/main.ts

+    if (release === undefined || release.id === state.releaseId) return;
+
+    const updateOrCancel = await vscode.window.showInformationMessage(
+        "New version of rust-analyzer (nightly) is available (requires restart).",


Suggested change

"New version of rust-analyzer (nightly) is available (requires restart).",

"New version of rust-analyzer (nightly) is available (requires reload).",

Let's use reload, it seems to be pervasive in vscode ecosystem.

Veetaha · 2020-03-18T22:10:02Z

editors/code/src/main.ts

+    const exists = await fs.stat(dest).then(() => true, () => false);
+    if (!exists) {


Suggested change

const exists = await fs.stat(dest).then(() => true, () => false);

if (!exists) {

if (!fs.existsSync(dest)) {

FIY: the async fs.exists() version is deprecated...

Oh, and for this we should amend the import:

import { promises as fs, existsSync as doesFileExist } from "fs";

I believe async stat is the more correct approach.

Veetaha · 2020-03-18T22:21:43Z

editors/code/src/net.ts

-    await fs.promises.mkdir(installationDir).catch(err => assert(
-        err?.code === "EEXIST",
-        `Couldn't create directory "${installationDir}" to download ` +
-        `${artifactFileName} artifact: ${err?.message}`
-    ));


This mkdir is for a reason

Excellent catch!

Veetaha · 2020-03-18T22:24:01Z

editors/code/src/persistent_state.ts

-
-    get(): T {
-        const val = this.storage.get(this.key, this.defaultVal);
-        log.debug(this.key, "==", val);


I''d suggest to log the persistent state at least somewhere.

Everything now happens in main.ts, in the bootstrap family of functions. The current flow is: * check everything only on extension installation. * if the user is on nightly channel, try to download the nightly extension and reload. * when we install nightly extension, we persist its release id, so that we can check if the current release is different. * if server binary was not downloaded by the current version of the extension, redownload it (we persist the version of ext that downloaded the server).

matklad · 2020-03-19T08:06:31Z

bors r+

bors · 2020-03-19T08:13:16Z

Build succeeded

matklad · 2020-03-19T08:37:09Z

bors r+

bors · 2020-03-19T08:37:10Z

Already running a review

matklad · 2020-03-19T08:37:19Z

bors merge

bors · 2020-03-19T08:37:19Z

Already running a review