LS: multi-root workspace support

[Arcticae]

This change is 

[mkaput]

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

---

crates/cairo-lang-language-server/src/env_config.rs line 19 at r1 (raw file):

pub const CAIRO_LS_PROFILE: &'_ str = "CAIRO_LS_PROFILE";
pub const SCARB: &'_ str = "SCARB";

why this extra newline?

---

crates/cairo-lang-language-server/src/env_config.rs line 33 at r1 (raw file):

}

pub fn current_workspace_scope() -> Option<PathBuf> {

doc

---

crates/cairo-lang-language-server/src/lang/lsp/ls_proto_group.rs line 26 at r1 (raw file):

                .map(|path| FileId::new(self.upcast(), path)),
            "vfs" =>
            // For vfs://[workspace_folder]/1234.cairo -> 1234

This branch looks to be not covered?

---

crates/cairo-lang-language-server/src/lang/lsp/ls_proto_group.rs line 28 at r1 (raw file):

            // For vfs://[workspace_folder]/1234.cairo -> 1234
            // For vfs://1234.cairo -> 1234
            {

weird formatting

Suggestion:

            "vfs" => {
                // For vfs://[workspace_folder]/1234.cairo -> 1234
                // For vfs://1234.cairo -> 1234

---

crates/cairo-lang-language-server/src/lang/lsp/ls_proto_group.rs line 47 at r1 (raw file):

    /// Get the canonical [`Url`] for a [`FileId`].
    fn url_for_file(&self, file_id: FileId, workspace_folder: Option<PathBuf>) -> Url {

I don't like the fact that you're passing the workspace path basically everywhere.

FileIds are pretty stable. What are your thoughts on storing a HashMap<FileId, PathBuf> as an input (+ proxy query (FileId) -> Option<Arc<Path>>) in salsa and reading the workspace association from it?

Though, I wonder if workspace folder ever cannot be None nor Some(CAIRO_LS_WORKSPACE_FOLDER

---

crates/cairo-lang-language-server/src/toolchain/scarb.rs line 120 at r1 (raw file):

        let manifest_dir = manifest
            .parent()
            .expect("No parent of the manifest")

keep Scarb.toml here → no need to query for parent → less dangerous unwraps

Suggestion:

.expect("no parent of the manifest")

---

crates/cairo-lang-language-server/src/toolchain/scarb.rs line 124 at r1 (raw file):

            .expect("Path is not correct UTF-8")
            .to_string();

consider using to_string_lossy instead

---

crates/cairo-lang-language-server/src/toolchain/scarb.rs line 154 at r1 (raw file):

#[serde(rename_all = "camelCase")]
struct ScarbResolvingParams {
    /// Folder in which the `Scarb.toml` is located

Suggestion:

/// Folder in which the `Scarb.toml` is located.

---

crates/cairo-lang-language-server/src/toolchain/scarb.rs line 155 at r1 (raw file):

struct ScarbResolvingParams {
    /// Folder in which the `Scarb.toml` is located
    manifest_dir: String,

Scarb used to keep manifest_path in case somebody if crazy and uses different file name. Maybe it'd be safer to keep the Scarb.toml file name also here?

[mkaput]

Reviewed 1 of 14 files at r1.
Reviewable status: 11 of 14 files reviewed, 10 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

---

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

    ...logEnv,
    ...extraEnv,
  };

Suggestion:

  const workspaceEnv = {
    CAIRO_LS_WORKSPACE_FOLDER: workspaceFolder.uri.fsPath
  };
  
  const logEnv = {
    CAIRO_LS_LOG: buildEnvFilter(ctx),
    RUST_BACKTRACE: "1",
  };

  const extraEnv = ctx.config.get("languageServerExtraEnv");

  serverExecutable.options ??= {};
  serverExecutable.options.env = {
    // Inherit env from parent process.
    ...process.env,
    ...(serverExecutable.options.env ?? {}),
    ...workspaceEnv,
    ...logEnv,
    ...extraEnv,
  };

[mkaput]

partial review, I haven't dive deep into TS side yet

Reviewable status: 11 of 14 files reviewed, 12 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

---

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

function sortedWorkspaceFolders(): string[] {
  if (_sortedWorkspaceFolders === void 0) {

Suggestion:

if (!_sortedWorkspaceFolders)

---

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

  if (_sortedWorkspaceFolders === void 0) {
    _sortedWorkspaceFolders = vscode.workspace.workspaceFolders
      ? vscode.workspace.workspaceFolders

nit, consider if/else here

[mkaput]

Reviewed 3 of 14 files at r1.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

---

vscode-cairo/src/client.ts line 9 at r1 (raw file):

export type WorkspacePath = string;
export type ClientsMap = Map<WorkspacePath, lc.LanguageClient>;
const clients: ClientsMap = new Map();

shouldn't this be put in Context so that we will ensure this state is recreated on extension reactivation?

---

vscode-cairo/src/client.ts line 26 at r1 (raw file):

    const workspaceName = pathComponents[1];
    if (!workspaceName) {
      throw new Error(`Could not extract workspace name from path: ${uri.path}`);

Suggestion:

throw new Error(`could not extract workspace name from path: ${uri.path}`);

---

vscode-cairo/src/client.ts line 29 at r1 (raw file):

    }
    matchFunction = ([k]) => {
      return k.endsWith(workspaceName);

1. I don't think workspace name is unique, say what happens when I open openzeppelin/cairo-contracts and software-mansion/cairo-contracts in a single window?
2. I have suspicions this code is prone to problems when one workspace name is a prefix/suffix of another, say two workspaces: core and ekubo-core

---

vscode-cairo/src/client.ts line 56 at r1 (raw file):

    () => undefined,
  );
}

Suggestion:

export async function stopAllClients(): Promise<void> {
  await Promise.all(Array.from(clients.values()).map((client) => client.stop()));
}

---

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

  }
  return folder;
}

consider extracting all this logic to some black-box entity, say WorkspaceManager to avoid polluting this file. just moving this to client.ts (together with code added to activate) could also be fine

---

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

  async function didOpenTextDocument(document: vscode.TextDocument): Promise<void> {
    // We are only interested in language mode text

doesn't this refer to plaintext files? of so, this comment is misleading

---

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

      return;
    }
    const uri = document.uri;

variable used only once, quite superfluous

---

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

    // Files outside a folder
    // TODO(arcticae): Those should probably be handled with a "default client"
    // it is unclear on how it should work exactly

by making an indent here, some TODO highlighting tools (ekhem IntelliJ) will catch up that this line is a continuation

Suggestion:

    // TODO(arcticae): Those should probably be handled with a "default client"
    //   it is unclear on how it should work exactly.

---

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

      deleteClient(folder);
    }
  });

onDidOpenTextDocument and onDidChangeWorkspaceFolders return disposables that I guess we need to register

[orizi]

Reviewed all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @Arcticae, @Draggu, and @piotmag769)

[Arcticae]

Closing because other solution was chosen for this problem (implementation in progress)