LS: Support multi-root-workspaces

[Arcticae]

This PR adds support for opening multiple scarb workspaces in one vscode window, like so:

Example diagnostics:

This solution, in contrast to the previous one, uses only one LS instance - so the resolved scarb versions must align, otherwise it won't work.

[reviewable-StarkWare]

This change is 

[Draggu]

Reviewable status: 0 of 2 files reviewed, 12 unresolved discussions (waiting on @Arcticae)

---

vscode-cairo/src/cairols.ts line 32 at r1 (raw file):

		return true;
	}
  let primaryExecutable = executables[0]!;

const is equal to let in rust and let is equal to let mut

Suggestion:

const

---

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

  let primaryExecutable = executables[0]!;
	
	return executables.slice(1).find((x) => primaryExecutable.run.command !== x.run.command) ? false : true;

Suggestion:

.every((x) => primaryExecutable.run.command === x.run.command)

---

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

export async function setupLanguageServer(ctx: Context): Promise<lc.LanguageClient> {  
  let executables = await Promise.all(

Suggestion:

const

---

vscode-cairo/src/cairols.ts line 40 at r1 (raw file):

  let executables = await Promise.all(
    (vscode.workspace.workspaceFolders || []).map(
      async (workspaceFolder) => await getLanguageServerExecutable(workspaceFolder, ctx)

In current js versions this does not matter (in older ones it did), but more readable.

Suggestion:

(workspaceFolder) => getLanguageServerExecutable(workspaceFolder, ctx)

---

vscode-cairo/src/cairols.ts line 41 at r1 (raw file):

    (vscode.workspace.workspaceFolders || []).map(
      async (workspaceFolder) => await getLanguageServerExecutable(workspaceFolder, ctx)
    ).filter((x) => !!x)

Also this filter should be after Promise.all, currently it is non effective as you check if Promise is truthy that is always true, we should check on Promise value instead.

Suggestion:

Boolean

---

vscode-cairo/src/cairols.ts line 42 at r1 (raw file):

      async (workspaceFolder) => await getLanguageServerExecutable(workspaceFolder, ctx)
    ).filter((x) => !!x)
  ) as LSExecutable[];

.filter() in correct place should make this cast unnecessary.

---

vscode-cairo/src/cairols.ts line 49 at r1 (raw file):

   // First one is good as any of them since they should be all the same at this point
  let lsExecutable = executables[0];

Suggestion:

const

---

vscode-cairo/src/cairols.ts line 53 at r1 (raw file):

  if (!lsExecutable) {
    throw new Error("Failed to start Cairo LS");
  }

https://nodejs.org/api/assert.html#assertvalue-message

this wil make type guard like if too

Suggestion:

  assert(lsExecutable, "Failed to start Cairo LS");

---

vscode-cairo/src/cairols.ts line 54 at r1 (raw file):

    throw new Error("Failed to start Cairo LS");
  }
  let { run, scarb } = lsExecutable; 

Suggestion:

const

---

vscode-cairo/src/cairols.ts line 189 at r1 (raw file):

async function getLanguageServerExecutable(workspaceFolder: vscode.WorkspaceFolder | undefined, ctx: Context): Promise<LSExecutable | undefined> {
  let scarb = await findScarbForWorkspaceFolder(workspaceFolder, ctx);

Suggestion:

const

---

vscode-cairo/src/cairols.ts line 191 at r1 (raw file):

  let scarb = await findScarbForWorkspaceFolder(workspaceFolder, ctx);
  try {
    let provider = await determineLanguageServerExecutableProvider(

Suggestion:

const

---

vscode-cairo/src/cairols.ts line 200 at r1 (raw file):

    ctx.log.error(`${e}`);
  }
  return undefined;

No need for explicit undefined but if you see it clearer im ok with keeping it.

Suggestion:

return;

[Arcticae]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @Draggu)

---

vscode-cairo/src/cairols.ts line 42 at r1 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

.filter() in correct place should make this cast unnecessary.

TS still complains after placing it differently so i'm leaving that

---

vscode-cairo/src/cairols.ts line 200 at r1 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

No need for explicit undefined but if you see it clearer im ok with keeping it.

Typescript emits a warning if you don't

---

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

  let primaryExecutable = executables[0]!;
	
	return executables.slice(1).find((x) => primaryExecutable.run.command !== x.run.command) ? false : true;

It's less optimal in most cases but i guess it's not a big deal here. I'll change it.

[Draggu]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)

---

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

Previously, Arcticae (Tomasz Rejowski) wrote…

It's less optimal in most cases but i guess it's not a big deal here. I'll change it.

It's not less optimal

---

vscode-cairo/src/cairols.ts line 42 at r4 (raw file):

    const primaryVersion = versions[0];

    return versions.slice(1).every((x) => x === primaryVersion);

Array adapters are eager, this is not rust iterator. we check here if all elements are equal so we an compare first with first, this will lead to 1 more compare and 1 less array allocation.

---

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

  const primaryExecutable = executables[0]!;
  return executables.slice(1).every((x) => primaryExecutable.run.command === x.run.command);

Vide supra

---

vscode-cairo/src/cairols.ts line 194 at r4 (raw file):

}

interface LSExecutable {

Don't like the name here, but no idea for better. We add scarb here and keep type name of run field, it is confusing whenever we uses one with scarb or not

[Draggu]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 56 at r4 (raw file):

      ),
    )
  ).filter((x) => !!x) as LSExecutable[];

Checked and it works without cast, maybe it was confusing with Boolean constructor that in fact won't type guard here (my mistake).

[Arcticae]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)

---

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

Previously, Draggu (Piotr Figiela) wrote…

It's not less optimal

I meant that find would stop after the first match of a different version, that's why i chose it

---

vscode-cairo/src/cairols.ts line 42 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Array adapters are eager, this is not rust iterator. we check here if all elements are equal so we an compare first with first, this will lead to 1 more compare and 1 less array allocation.

It seems more logical to me this way but i'm not

---

vscode-cairo/src/cairols.ts line 56 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Checked and it works without cast, maybe it was confusing with Boolean constructor that in fact won't type guard here (my mistake).

No, it doesn't work

(IDE doesn't complain but TS does)

---

vscode-cairo/src/cairols.ts line 194 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Don't like the name here, but no idea for better. We add scarb here and keep type name of run field, it is confusing whenever we uses one with scarb or not

I can change provider's method languageServerExecutable to just executable if that makes things clearer

[Draggu]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)

---

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

Previously, Arcticae (Tomasz Rejowski) wrote…

I meant that find would stop after the first match of a different version, that's why i chose it

every stops on first false

---

vscode-cairo/src/cairols.ts line 56 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

No, it doesn't work

(IDE doesn't complain but TS does)

TS version problem, in 5.3.3 there is no overload for this.

---

vscode-cairo/src/cairols.ts line 194 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I can change provider's method languageServerExecutable to just executable if that makes things clearer

interface LSExecutable {
  run: lc.Executable;

this part is confusing for me, but leaving it to you

[mkaput]

Reviewed 1 of 2 files at r1, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 42 at r5 (raw file):

    return versions.every((x) => x === versions[0]);
  }

Shouldn't you just ensure executable.run.commandare the same (+/- environment, maybe even deepEqual entire executable object)?

1. You wouldn't need to rely on passing scarb instance around and do costly version checks.
2. I have suspicions that Scarb version output is not enough because this will at least not catch out-of-tree builds.
3. You will make this code also work in non-scarb contexts.

Code quote:

  // If every executable is scarb based, check if the versions match
  if (executables.every((v) => !!v.scarb)) {
    const versions = await Promise.all(executables.map((v) => v.scarb!.getVersion(ctx)));

    return versions.every((x) => x === versions[0]);
  }

---

vscode-cairo/src/cairols.ts line 57 at r5 (raw file):

  const sameProvider = await allFoldersHaveSameLSProvider(ctx, executables);
  assert(sameProvider, "Multiple versions of scarb in one workspace is not supported");

By using assertions, you will basically crash the extension, aren't you? This would be bad UX. Let's log an error + show a notification and bail out nicely by ignoring workspace, perhaps.

[Arcticae]

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

---

vscode-cairo/src/cairols.ts line 42 at r5 (raw file):

I have suspicions that Scarb version output is not enough

It's necessary because of the shim having the same executable path
There is a case where you could break it so i will add deepEquality based comparison here as well.

---

vscode-cairo/src/cairols.ts line 57 at r5 (raw file):

Previously, mkaput (Marek Kaput) wrote…

By using assertions, you will basically crash the extension, aren't you? This would be bad UX. Let's log an error + show a notification and bail out nicely by ignoring workspace, perhaps.

Ignoring workspace, vs. loading first folder? Can do either. WDYT?

[Arcticae]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 42 at r5 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I have suspicions that Scarb version output is not enough

It's necessary because of the shim having the same executable path
There is a case where you could break it so i will add deepEquality based comparison here as well.

Done

---

vscode-cairo/src/cairols.ts line 57 at r5 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Ignoring workspace, vs. loading first folder? Can do either. WDYT?

Done

[Draggu]

Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 38 at r6 (raw file):

without throwing an error

So without or not?

Just remove instanceof check, deepStrictEqual can throw something different only if its 3rd argument (not provided here) is Error

---

vscode-cairo/src/cairols.ts line 89 at r6 (raw file):

  const lsExecutable = executables[0];

  assert(lsExecutable, "Failed to start Cairo LS");

https://reviewable.io/reviews/starkware-libs/cairo/6645#-OBfSk6E8lxe3s_HaJXt

[mkaput]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 81 at r6 (raw file):

  if (!sameProvider) {
    await vscode.window.showErrorMessage(
      "Multiple versions of scarb in one workspace is not supported",

Suggestion:

      "Using multiple Scarb versions in one workspace are not supported.",

[Arcticae]

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

---

vscode-cairo/src/cairols.ts line 38 at r6 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

without throwing an error

So without or not?

Just remove instanceof check, deepStrictEqual can throw something different only if its 3rd argument (not provided here) is Error

I'd rather keep it extra safe without the additional assumptions

---

vscode-cairo/src/cairols.ts line 89 at r6 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

https://reviewable.io/reviews/starkware-libs/cairo/6645#-OBfSk6E8lxe3s_HaJXt

This would fail anyway on the previous version. Why handle it now? It's out of scope

---

vscode-cairo/src/cairols.ts line 81 at r6 (raw file):

  if (!sameProvider) {
    await vscode.window.showErrorMessage(
      "Multiple versions of scarb in one workspace is not supported",

Done

[mkaput]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Draggu, @orizi, and @piotmag769)

[Draggu]

Reviewed 1 of 2 files at r1, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 38 at r6 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I'd rather keep it extra safe without the additional assumptions

This is not additional assumption, it is part of its interface described in doc.

---

vscode-cairo/src/cairols.ts line 76 at r7 (raw file):

      ),
    )
  ).filter((x) => !!x) as LSExecutable[];

Leave TODO here to remove cast when ts version is upgraded

[Arcticae]

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)

---

vscode-cairo/src/cairols.ts line 76 at r7 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Leave TODO here to remove cast when ts version is upgraded

Done

[piotmag769]

This language hurts my soul

Reviewed 1 of 2 files at r1, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved