LS: React to adding/deleting workspace folders

[Arcticae]

Additions and deletions of folders should cause the client to be stopped/restarted/started and assessed which action is necessary, based on some prerequisites. We don't want to stop it when it's not necessary, mostly because of diagnostics and wasted time on waiting (sometimes stopping is simply not required).

The rough idea can be described with those graphs:

[reviewable-StarkWare]

This change is 

[mkaput]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, @orizi, and @piotmag769)

---

vscode-cairo/src/extension.ts line 24 at r1 (raw file):

  client = undefined;
  runningExecutable = undefined;
}

consider extracting a separate module with this. in general this code grew to a level I would encapsulate it into some kind of Manager module or singleton, along all these command equality checks and event handlers

I have an impression that this PR smears added logic significantly. :(

Code quote:

let runningExecutable: LSExecutable | undefined;

export async function startClient(ctx: Context) {
  const setupResult = await setupLanguageServer(ctx);
  if (!setupResult) {
    return;
  }
  const [newClient, newExecutable] = setupResult;
  client = newClient;
  runningExecutable = newExecutable;
}

export async function stopClient() {
  await client?.stop();
  client = undefined;
  runningExecutable = undefined;
}

---

vscode-cairo/src/extension.ts line 46 at r1 (raw file):

        ctx.extension.subscriptions,
      ),
    );

could you please explain this fragment to me? why do you need this?

Code quote:

    // Notify the server when the client configuration changes.
    // CairoLS pulls configuration properties it is interested in by itself, so it
    // is unnecessary to attach any details in the notification payload.
    ctx.extension.subscriptions.push(
      vscode.workspace.onDidChangeConfiguration(
        async () => {
          if (client != undefined) {
            await client.sendNotification(lc.DidChangeConfigurationNotification.type, {
              settings: "",
            });
          }
        },
        null,
        ctx.extension.subscriptions,
      ),
    );

---

vscode-cairo/src/statusBar.ts line 45 at r1 (raw file):

  }

  async updateScarbVersion(): Promise<void> {

you do not use this method anyway so please revert this change

---

vscode-cairo/src/workspace.ts line 8 at r1 (raw file):

export async function handleWorkspaceFoldersAdded(
  newWorkspaceFolders: readonly vscode.WorkspaceFolder[],
  runningExecutable: LSExecutable | undefined,

it is weird that you're passing this global as argument here. you won't need this if you extract the if(!runningExecutable) startClient() pattern to a function

---

vscode-cairo/src/workspace.ts line 23 at r1 (raw file):

    if (!runningExecutable) {
      ctx.log.debug(
        `Added folder(s) (${newWorkspaceFolders.map((x) => x.uri.fsPath)} ) have same provider - starting the client`,

logs start lowercase

---

vscode-cairo/src/workspace.ts line 25 at r1 (raw file):

        `Added folder(s) (${newWorkspaceFolders.map((x) => x.uri.fsPath)} ) have same provider - starting the client`,
      );
      await startClient(ctx);

It looks like you're not respecting the enableLanguageServer config here. In general, this pattern is pretty repetitive, consider extracting a function.

---

vscode-cairo/src/workspace.ts line 34 at r1 (raw file):

      ctx,
    );
    // If it's not, we need to stop LS and show an error

I am very much missing a doc why you're stopping the client in this scenario. It took me a while to realise that you're doing this because it's better to stop analysis than do broken analysis. And this behaviour explains stuff like why you're starting new clients in weird places like handleWorkspaceFoldersRemoved, so in general, this little context could explain a lot.

---

vscode-cairo/src/workspace.ts line 38 at r1 (raw file):

      await stopClient();
      vscode.window.showErrorMessage(
        "Added folder does not have the same scarb version. Please remove it from the workspace, or adjust the used scarb version.",

Scarb uppercase, also TBH this is a bit broken english to me, and also it'd be good to notify the user that analysis will stop until they fix the problem.

[mkaput]

Reviewed 3 of 8 files at r2, all commit messages.
Reviewable status: 3 of 8 files reviewed, 7 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, @orizi, and @piotmag769)

---

vscode-cairo/src/extension.ts line 46 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

could you please explain this fragment to me? why do you need this?

I'm dumb. I wrote this code and forgot about it

[mkaput]

Reviewed 5 of 8 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 46 at r2 (raw file):

  const run = lsExecutable.preparedInvocation;
  const scarb = lsExecutable.scarb;

nit, wdyt?

Suggestion:

  assert(executables[0], "executable should be present at this point");
  const [{ preparedInvocation: run, scarb }] = executables;

---

vscode-cairo/src/extensionManager.ts line 8 at r2 (raw file):

import assert from "node:assert";

export class CairoExtensionManager {

please document here why we are maintaining only one client

---

vscode-cairo/src/extensionManager.ts line 20 at r2 (raw file):

  // Starts the server, sets up the client and returns status
  public async tryStartClient(): Promise<boolean> {

tsdoc

Suggestion:

  /**
   * Starts the server, sets up the client and returns status.
   */
  public async tryStartClient(): Promise<boolean> {

---

vscode-cairo/src/extensionManager.ts line 20 at r2 (raw file):

  // Starts the server, sets up the client and returns status
  public async tryStartClient(): Promise<boolean> {

It looks like you're not respecting the enableLanguageServer config here. Mind that it can change in the runtime and I even think we should listen for this config changes and start/restart LS appropriately (though this is a topic for separate PR)

---

vscode-cairo/src/extensionManager.ts line 43 at r2 (raw file):

  public getClient(): lc.LanguageClient | undefined {
    return this.client;
  }

this can easily be a public readonly property

Code quote:

  public getClient(): lc.LanguageClient | undefined {
    return this.client;
  }

---

vscode-cairo/src/extensionManager.ts line 57 at r2 (raw file):

    }

    // Check if new ones are of same provider

all these comments are missing dots

---

vscode-cairo/src/extension.ts line 44 at r2 (raw file):

        null,
        ctx.extension.subscriptions,
      ),

wdyt about extracting entire logic into extensionManager

Suggestion:

      vscode.workspace.onDidChangeWorkspaceFolders(
        extensionManager.onDidChangeWorkspaceFolders,
        extensionManager // this argument sets proper `this` in the callback
        ctx.extension.subscriptions,
      ),

---

vscode-cairo/src/extension.ts line 46 at r2 (raw file):

      ),
    );
    ctx.extension.subscriptions.push({ dispose: extensionManager.stopClient });

I think extensionManager should just be a disposable directly

[Arcticae]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Draggu, @integraledelebesgue, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/extension.ts line 46 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I'm dumb. I wrote this code and forgot about it

Forgot to reply, but yes. It was just copied over

---

vscode-cairo/src/extension.ts line 44 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

wdyt about extracting entire logic into extensionManager

Can do, perhaps it's better

---

vscode-cairo/src/extension.ts line 46 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I think extensionManager should just be a disposable directly

Matter of taste but i think that rather pollutes the extensionManager, and the logic here is not much anyways

---

vscode-cairo/src/extensionManager.ts line 20 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

It looks like you're not respecting the enableLanguageServer config here. Mind that it can change in the runtime and I even think we should listen for this config changes and start/restart LS appropriately (though this is a topic for separate PR)

It wasn't supported before so it's def out of scope here - new issue: #6737

---

vscode-cairo/src/extensionManager.ts line 43 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

this can easily be a public readonly property

No it cannot because i want to mutate it during the course of the object lifetime (turning the client off and on again results in this behavior)

[mkaput]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, @orizi, and @piotmag769)

---

vscode-cairo/src/extension.ts line 46 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Matter of taste but i think that rather pollutes the extensionManager, and the logic here is not much anyways

slightly disagree because I find the fact that something is treated as a disposable pretty important

---

vscode-cairo/src/extensionManager.ts line 43 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

No it cannot because i want to mutate it during the course of the object lifetime (turning the client off and on again results in this behavior)

so perhaps rename this to currentClient that could be a getter (computed property) anyway?

[Arcticae]

Reviewable status: 4 of 8 files reviewed, 6 unresolved discussions (waiting on @Draggu, @integraledelebesgue, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 46 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

nit, wdyt?

Done

---

vscode-cairo/src/extension.ts line 44 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Can do, perhaps it's better

Done

---

vscode-cairo/src/extension.ts line 46 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

slightly disagree because I find the fact that something is treated as a disposable pretty important

I don't mind it but it'd be more confusing to read (to me). Done.

---

vscode-cairo/src/extensionManager.ts line 8 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

please document here why we are maintaining only one client

Done

---

vscode-cairo/src/extensionManager.ts line 20 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

tsdoc

Done

---

vscode-cairo/src/extensionManager.ts line 43 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

so perhaps rename this to currentClient that could be a getter (computed property) anyway?

I don't see anything wrong with the getter pattern i applied here. It's pretty much textbook

---

vscode-cairo/src/extensionManager.ts line 57 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

all these comments are missing dots

Done

[mkaput]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Draggu, @integraledelebesgue, @orizi, and @piotmag769)

[Draggu]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @integraledelebesgue, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 147 at r3 (raw file):

  await client.start();

  return [client, executables[0]];

Suggestion:

{client, executable: executables[0]}

---

vscode-cairo/src/extension.ts line 22 at r3 (raw file):

            .getClient()
            ?.sendNotification(lc.DidChangeConfigurationNotification.type, {
              settings: "",

Do we need to pass string here? will null not be more explicit?

---

vscode-cairo/src/scarb.ts line 96 at r3 (raw file):

      return this.version;
    }
    this.version = (await this.execWithOutput(["--version"], ctx)).trim();

Suggestion:

    this.version ??= (await this.execWithOutput(["--version"], ctx)).trim();

---

vscode-cairo/src/extensionManager.ts line 56 at r3 (raw file):

  public async handleDidChangeWorkspaceFolders(event: vscode.WorkspaceFoldersChangeEvent) {
    if (event.added.length) {

Can we make explicit !== 0 checks?

[Arcticae]

Moving to software-mansion/vscode-cairo#6