LS: Proc macro server connection

[Draggu]

closes #6141

---

Stack:

• LS: Proc macro disable option in config #6706
• LS: Proc macro server failed notification #6705
• LS: Proc macro handle responses #6704
• LS: Proc macro - scarb code #6638
• LS: Proc macro request handling #6703
• LS: Proc macro plugins #6702
• LS: Proc macro server connection #6637 ⬅
• LS: Refactor main loop to support select! #6701

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

[reviewable-StarkWare]

This change is 

[piotmag769]

Reviewed 10 of 19 files at r1.
Reviewable status: 9 of 19 files reviewed, 15 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 116 at r1 (raw file):

        new_db.set_proc_macro_client_status(old_db.proc_macro_client_status());

        // TODO probably this should not be part of migration as it will be ever growing.

TODO we should have an issue for that

---

crates/cairo-lang-language-server/Cargo.toml line 28 at r1 (raw file):

cairo-lang-test-plugin = { path = "../cairo-lang-test-plugin", version = "~2.8.4" }
cairo-lang-utils = { path = "../cairo-lang-utils", version = "~2.8.4" }
cairo-lang-macro = { git = "https://github.com/software-mansion/scarb", rev = "fc99b54" } # not registry version because conflict with scarb-proc-macro-server-types local version

Marking to make sure we remember to wait for the release

---

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

/// Notifies about `proc-macro-server` fatal fail.
#[derive(Debug)]
pub struct ProcMacroServerFatalFailed;

And change the METHOD pls + add client side handling in separate PR

Suggestion:

ProcMacroServerInitializationFailed

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/id_generator.rs line 5 at r1 (raw file):

use scarb_proc_macro_server_types::jsonrpc::RequestId;

/// Atomic Id generator

dot

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 55 at r1 (raw file):

    /// Runs in post sync task hook. Starts proc-macro-server after config reload.
    /// Note that this will only try to go from `ClientStatus::Disabled` to
    /// `ClientStatus::Initializing` if config now allows this.

Suggestion:

    /// Start proc-macro-server after config reload.
    /// Note that this will only try to go from `ClientStatus::Disabled` to
    /// `ClientStatus::Initializing` if config allows this.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 62 at r1 (raw file):

    }

    /// Tries starting proc-macro-server initialization process, if enabled.

Suggestion:

/// Tries starting proc-macro-server initialization process, if allowed by config.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 64 at r1 (raw file):

    /// Tries starting proc-macro-server initialization process, if enabled.
    ///
    /// Returns value indicating if attempted to initialize.

Suggestion:

/// Returns value indicating if initialization was attempted.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 67 at r1 (raw file):

    pub fn try_initialize(&mut self, db: &mut AnalysisDatabase, config: &Config) -> bool {
        // Do not initialize if not yet received client config (None) or received `true`.
        // Also if disabled skip rate limiter check as this should not reduce burst.

Suggestion:

        // Do not initialize if not yet received client config (None) or received `true`.
        // Keep the rate limiter check as second condition if when config doesn't allow it to make sure it is not impacted.

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 59 at r1 (raw file):

            get_default_plugin_suite(),
            starknet_plugin_suite(),
            // test_plugin_suite()

?

---

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

    }

    pub fn proc_macro_server(&self) -> Result<Child> {

Suggestion:

start_proc_macro_server

---

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

            scheduler.on_sync_task(Self::refresh_diagnostics);

            // Starts proc-macro-server after config has been changed, this counts first load.

In this context you don't know what first load is

Suggestion:

// Starts proc-macro-server after config has been changed.

---

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

    fn event_loop(connection: &Connection, mut scheduler: Scheduler<'_>) -> Result<()> {
        // 1 per second.
        let idle_job_limit = RateLimiter::direct(Quota::per_second(NonZeroU32::new(1).unwrap()));

[nit]

Suggestion:

idle_job_limiter

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 23 at r1 (raw file):

pub enum ClientStatus {
    // Disabled is default because it is initialized before receiving client config, though we
    // should not start it if config can prohibit this.

Suggestion:

    // Disabled is default because it is initialized before receiving client config.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 28 at r1 (raw file):

    Initializing,
    Ready(Arc<ProcMacroClient>),
    // After retries it does not work. No more actions will be done.

Suggestion:

/// Failed to initialize multiple times. No more actions will be taken.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 29 at r1 (raw file):

    Ready(Arc<ProcMacroClient>),
    // After retries it does not work. No more actions will be done.
    InitializingFailed,

Suggestion:

FailedToInitialize,

[Draggu]

Reviewable status: 9 of 19 files reviewed, 11 unresolved discussions (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 59 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

?

#6551 added TODO

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 116 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

TODO we should have an issue for that

#6646 added issue link

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 55 at r1 (raw file):

    /// Runs in post sync task hook. Starts proc-macro-server after config reload.
    /// Note that this will only try to go from `ClientStatus::Disabled` to
    /// `ClientStatus::Initializing` if config now allows this.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 62 at r1 (raw file):

    }

    /// Tries starting proc-macro-server initialization process, if enabled.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 64 at r1 (raw file):

    /// Tries starting proc-macro-server initialization process, if enabled.
    ///
    /// Returns value indicating if attempted to initialize.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 67 at r1 (raw file):

    pub fn try_initialize(&mut self, db: &mut AnalysisDatabase, config: &Config) -> bool {
        // Do not initialize if not yet received client config (None) or received `true`.
        // Also if disabled skip rate limiter check as this should not reduce burst.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 28 at r1 (raw file):

    Initializing,
    Ready(Arc<ProcMacroClient>),
    // After retries it does not work. No more actions will be done.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 29 at r1 (raw file):

    Ready(Arc<ProcMacroClient>),
    // After retries it does not work. No more actions will be done.
    InitializingFailed,

Done.

---

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

    }

    pub fn proc_macro_server(&self) -> Result<Child> {

Disagree here, it follows command name scarb proc-macro-server. Same applies to .metadata().

[piotmag769]

Reviewed 2 of 19 files at r1, all commit messages.
Reviewable status: 6 of 19 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

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

    /// [`None`] means that config has not yet been loaded from client.
    /// We want to default it to `false`, but to prevent proc-macro-server from starting
    /// immediately even if client want to set it to `true` we uses this 3rd value.

Suggestion:

immediately, even if the client wants to set the config value to `true`, we uses the `None` value.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 37 at r1 (raw file):

    }

    pub fn disabled(&self) -> bool {

Suggestion:

is_disabled

[Draggu]

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

---

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

    /// [`None`] means that config has not yet been loaded from client.
    /// We want to default it to `false`, but to prevent proc-macro-server from starting
    /// immediately even if client want to set it to `true` we uses this 3rd value.

Done.

[piotmag769]

Reviewed 1 of 19 files at r1, 2 of 2 files at r2, 5 of 7 files at r3.
Reviewable status: 12 of 19 files reviewed, 6 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

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

Previously, Draggu (Piotr Figiela) wrote…

Done.

Commas missing

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 105 at r3 (raw file):

    /// Removes all plugins from `previous_plugin_suite` if exists, plugins are considered
    /// equal if uses same [`Arc`]. Then adds `new_plugin_suite`, unlike
    /// [`AnalysisDatabase::apply_plugin_suite`] this will keep all other plugins in place.

Suggestion:

    /// Modifies plugins in the db:
    ///  - removes plugins from `previous_plugin_suite`,
    ///  - adds plugins from `new_plugin_suite`,
    ///  - retains other plugins present in the db.
    ///
    /// Plugins are considered equal if they use the same [`Arc`].

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 91 at r3 (raw file):

        ensure!(
            id == 0,
            "fetching defined macros should be first sended request, it is {id} (zero counting)"

Suggestion:

"fetching defined macros should have been the first sent request, it was {id}. (zero counting)"

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 111 at r3 (raw file):

        new_db.set_inline_macro_plugins(old_db.inline_macro_plugins());
        // Currently there is no analyzer plugins here, but keep it because it would be hard to
        // track it if we need it in future.

This will become outdated and no one will change it

[piotmag769]

Reviewed 1 of 2 files at r2, 1 of 3 files at r4, all commit messages.
Reviewable status: 13 of 19 files reviewed, 6 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 136 at r4 (raw file):

        }

        macro_plugins.extend(new_plugin_suite.plugins);

Isn't there a chance that we add the same plugins if they are both in new_plugin_suite and the db?

[piotmag769]

Reviewed 3 of 19 files at r1, 1 of 7 files at r3, 2 of 3 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lib.rs line 297 at r4 (raw file):

            scheduler.on_sync_task(Self::refresh_diagnostics);

            // Starts proc-macro-server after config has been changed.

This is not fully true, no? I mean it happens after config change, but also if it just failed to start when it was meant to start it

---

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

            .stdin(Stdio::piped())
            .stdout(Stdio::piped())
            // We uses this channel for debugging.

Suggestion:

We use 

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

                // `initialize` is blocking.
                std::thread::spawn(move || client.initialize());

Are we fine with detaching this thread? cc @mkaput

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 22 at r4 (raw file):

#[derive(Debug, Default, Clone)]
pub enum ClientStatus {
    /// Disabled is default because it is initialized before receiving client config.

Suggestion:

it is not initialized before receiving client config.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 49 at r4 (raw file):

    Failed,
    // Even if we retry it probably won't work anyway.
    FatalFailed,

Suggestion:

    // We can retry.
    RecoverablyFailed,
    // Even if we retry it probably won't work anyway.
    UnrecoverablyFailed,

[piotmag769]

Reviewed 1 of 7 files at r3.
Reviewable status: 17 of 19 files reviewed, 16 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lang/proc_macros/db.rs line 66 at r4 (raw file):

    db.inline_macro_resolution().get(&params).cloned().unwrap_or_else(|| {
        // we can't return original node because it will make infinite recursive resolving.
        let token_stream = TokenStream::new("()".to_string());

Good catch

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 32 at r4 (raw file):

#[derive(Debug)]
pub struct ProcMacroClient {

Why is this in the mod client when you don't expose it anywhere, while the exposed ProcMacroClientController is deeper down?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 104 at r4 (raw file):

        ensure!(
            response.id == id,
            "fetching defined macros should be waited before any other request is send, received \

Vide supra

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 117 at r4 (raw file):

    }

    fn send_request<M: Method>(&self, params: &M::Params) -> Result<RequestId> {

And the one below should be send_request

Suggestion:

send_request_untracked

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 27 at r4 (raw file):

/// Manages lifecycle of proc-macro-server client.
pub struct ProcMacroClientController {
    status_change: ProcMacroClientStatusChange,

It is borderline unintuitive that the connection to the client is there under 10 layers of abstraction. It cannot stay this way.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 67 at r4 (raw file):

    pub fn try_initialize(&mut self, db: &mut AnalysisDatabase, config: &Config) -> bool {
        // Do not initialize if not yet received client config (None) or received `true`.
        // Keep the rate limiter check as second condition if when config doesn't allow it to make

Suggestion:

condition when 

[piotmag769]

Reviewable status: 16 of 19 files reviewed, 18 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 8 at r4 (raw file):

#[derive(Debug, Clone, Default)]
pub struct ProcMacroClientStatusChange(Arc<Mutex<Option<ClientStatusChange>>>);

Why is it in both ProcMacroClientController and ProcMacroController? The abstractions are leaking everywhere in this PR

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 158 at r4 (raw file):

        for response in client.available_responses() {
            // TODO investigate this as it somehow panicks if there is \0 problem.

Still relevant? If so we should have an issue for that

[piotmag769]

Reviewable status: 16 of 19 files reviewed, 20 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 28 at r4 (raw file):

pub struct ProcMacroClientController {
    status_change: ProcMacroClientStatusChange,
    notifier: Notifier,

Same as below, it is only needed in function called from run_idle_tasks which gets the notifier anyways as arg

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 29 at r4 (raw file):

    status_change: ProcMacroClientStatusChange,
    notifier: Notifier,
    scarb: ScarbToolchain,

[nit] It is only used in spawn_server which is initially called from maybe_start_proc_macro_server which has access to the state. You can just pass ScarbToolchain to spawn_server to prevent this structure from growing.

[piotmag769]

Reviewed 1 of 19 files at r1, 2 of 3 files at r4.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 8 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Why is it in both ProcMacroClientController and ProcMacroController? The abstractions are leaking everywhere in this PR

1. I meant ProcMacroClient
2. I know why, but this is unintuitive and took me an hour to figure out. Just put it in a client at this point and get it directly inProcMacroClientController.

[orizi]

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

[Draggu]

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

---

crates/cairo-lang-language-server/Cargo.toml line 28 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Marking to make sure we remember to wait for the release

Done.

---

crates/cairo-lang-language-server/src/lib.rs line 297 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This is not fully true, no? I mean it happens after config change, but also if it just failed to start when it was meant to start it

There is check if current state is disabled, no no

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 136 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Isn't there a chance that we add the same plugins if they are both in new_plugin_suite and the db?

Yes, there are, in current use cases this can not happen, but if you think we should add validation/comment here feel free to write here.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 27 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

It is borderline unintuitive that the connection to the client is there under 10 layers of abstraction. It cannot stay this way.

Any proposition? I don't find it unintuitive here, controller Manages lifecycle of proc-macro-server client. also starting it.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 28 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Same as below, it is only needed in function called from run_idle_tasks which gets the notifier anyways as arg

Keeping it here is straightforward and consistent with DiagnosticsController, also when we will want to add progress bar for proc-macros may be used more as all state transitions are handled here.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 29 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

[nit] It is only used in spawn_server which is initially called from maybe_start_proc_macro_server which has access to the state. You can just pass ScarbToolchain to spawn_server to prevent this structure from growing.

Used approach from AnalysisDatabaseSwapper, also i think it is cleaner this way.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 32 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Why is this in the mod client when you don't expose it anywhere, while the exposed ProcMacroClientController is deeper down?

Honestly, no idea how to name it other way.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 104 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Vide supra

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 117 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

And the one below should be send_request

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 8 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

1. I meant ProcMacroClient
2. I know why, but this is unintuitive and took me an hour to figure out. Just put it in a client at this point and get it directly inProcMacroClientController.

It can not be only part of ProcMacroClient, because it is used for example when spawning with scarb proc-macro-server failed (in this case there is no client).

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 67 at r4 (raw file):

    pub fn try_initialize(&mut self, db: &mut AnalysisDatabase, config: &Config) -> bool {
        // Do not initialize if not yet received client config (None) or received `true`.
        // Keep the rate limiter check as second condition if when config doesn't allow it to make

Done.

---

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

            .stdin(Stdio::piped())
            .stdout(Stdio::piped())
            // We uses this channel for debugging.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 91 at r3 (raw file):

        ensure!(
            id == 0,
            "fetching defined macros should be first sended request, it is {id} (zero counting)"

Done.

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 105 at r3 (raw file):

    /// Removes all plugins from `previous_plugin_suite` if exists, plugins are considered
    /// equal if uses same [`Arc`]. Then adds `new_plugin_suite`, unlike
    /// [`AnalysisDatabase::apply_plugin_suite`] this will keep all other plugins in place.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 22 at r4 (raw file):

#[derive(Debug, Default, Clone)]
pub enum ClientStatus {
    /// Disabled is default because it is initialized before receiving client config.

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 49 at r4 (raw file):

    Failed,
    // Even if we retry it probably won't work anyway.
    FatalFailed,

Done.

[piotmag769]

Reviewed 6 of 7 files at r5, all commit messages.
Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

[piotmag769]

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

[mkaput]

Reviewed 1 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Are we fine with detaching this thread? cc @mkaput

It is fine for MVP purposes, but for production code we need a way to abort this waiting whenever we need (see AbortController pattern for example)

---

crates/cairo-lang-language-server/src/config.rs line 49 at r6 (raw file):

    /// immediately, even if the client wants to set the config value to `true`, we uses the `None`
    /// value.
    pub disable_proc_macros: Option<bool>,

this is a topic for separate PR. MVP can be always-on

[mkaput]

Reviewed 2 of 19 files at r1, 4 of 7 files at r3, 1 of 7 files at r5, 2 of 4 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Arcticae, @Draggu, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 111 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This will become outdated and no one will change it

I agree that the comment will be forgotten, but code line kind of makes sense, isn't it? Maybe let's just remove this comment?

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 7 at r6 (raw file):

use scarb_proc_macro_server_types::methods::defined_macros::DefinedMacrosResponse;

/// Important: NEVER make it pub outside this crate.

why is this so important? we are not a library crate anyway

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 26 at r6 (raw file):

}

/// Important: NEVER make it public.

why is this important?

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 53 at r6 (raw file):

}

/// Important: NEVER make it public.

why is this important?

---

crates/cairo-lang-language-server/src/lib.rs line 366 at r6 (raw file):

                continue;
            };

this is very bad, you are still busy waiting basically. there's no way OS/CPU will ever put our process in the background as it will never reliably hang on IO and this will be bad for battery life for users

maybe you need to select! here?

Code quote:

        // 1 per second.
        let idle_job_limiter = RateLimiter::direct(Quota::per_second(NonZeroU32::new(1).unwrap()));

        for msg in connection.incoming() {
            let Some(msg) = msg else {
                if idle_job_limiter.check().is_ok() {
                    scheduler.local(Self::run_idle_tasks);
                }

                // Avoid busy-waiting, sleep for a short duration
                std::thread::sleep(Duration::from_millis(1));

                continue;
            };

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 76 at r6 (raw file):

        db.set_inline_macro_resolution(Default::default());
        // We read this to check if client is available so it should be initialized.
        db.set_proc_macro_client_status(Default::default());

It'd be good to extract this as an init_* function like init_files_group

Code quote:

        // proc-macro-server can be restarted many times but we want to keep these data across
        // multiple server starts, so init it once per database, not per server.
        db.set_attribute_macro_resolution(Default::default());
        db.set_derive_macro_resolution(Default::default());
        db.set_inline_macro_resolution(Default::default());
        // We read this to check if client is available so it should be initialized.
        db.set_proc_macro_client_status(Default::default());

[mkaput]

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

---

crates/cairo-lang-language-server/src/lang/proc_macros/db.rs line 11 at r6 (raw file):

#[salsa::query_group(ProcMacroCacheDatabase)]
pub trait ProcMacroCacheGroup {

caching could be a separate PR

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 34 at r6 (raw file):

}

impl ProcMacroClientController {

I think you could easily make a split into two PRs here: first you would just initialize the connection, and second you'd create compiler plugin and start communicating with the connection

[Arcticae]

Reviewed 5 of 19 files at r1, 1 of 2 files at r2, 2 of 7 files at r3, 1 of 4 files at r6.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @Draggu and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 105 at r3 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Done.

Let's rename the parameters so that they better reflect what is actually their purpose 🙏

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 111 at r4 (raw file):

it would be hard to track it if we need it in future.

WDYM?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Are we fine with detaching this thread? cc @mkaput

Isn't there a case where we want to restart?

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 103 at r6 (raw file):

    /// Check if there was status change reported and applies it.
    /// If client is ready applies all available responses.
    pub fn maybe_update_and_apply_responses(&mut self, db: &mut AnalysisDatabase, config: &Config) {

I feel like this function handles two separate concerns, do we really need it?

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 108 at r6 (raw file):

        };

        // TODO we should check here if there are live snapshots, but no idea if it is possible with

It needs an issue, doesn't it?

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 117 at r6 (raw file):

    /// Process status change update.
    fn update_status(

This is more than that, it's status update, and an action in tandem

---

crates/cairo-lang-language-server/src/lang/proc_macros/db.rs line 34 at r4 (raw file):

    params: ExpandAttributeParams,
) -> ProcMacroResult {
    db.attribute_macro_resolution().get(&params).cloned().unwrap_or_else(|| {

Correct me if i'm wrong, but shouldn't this (cache vs query) be also handled by salsa somehow? (instead of using a map for input here)

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 34 at r4 (raw file):

}

impl MacroPlugin for ProcMacroPlugin {

What's this code for exactly?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 12 at r4 (raw file):

impl ProcMacroClientStatusChange {
    pub fn update(&self, change: ClientStatusChange) {
        *self.0.lock().unwrap() = Some(change);

In what case can this fail? Is this something we can account for?

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 116 at r4 (raw file):

        new_db.set_proc_macro_client_status(old_db.proc_macro_client_status());

        // TODO(#6646) probably this should not be part of migration as it will be ever growing.

How big of a problem is that exactly? What's the estimated size of those maps when accumulating those results in practise?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 14 at r4 (raw file):

impl ProcMacroServerConnection {
    pub fn new(mut proc_macro_server: std::process::Child) -> Self {

Let's split it into 2 methods and have the threads named somehow via the functions name

---

crates/cairo-lang-language-server/src/lib.rs line 298 at r4 (raw file):

            // Starts proc-macro-server after config has been changed.
            scheduler.on_sync_task(Self::maybe_start_proc_macro_server);

Why is it run on each sync task? isn't it a bit excessive?

---

crates/cairo-lang-language-server/src/config.rs line 46 at r4 (raw file):

    /// configuration.
    /// [`None`] means that config has not yet been loaded from client.
    /// We want to default it to `false`, but to prevent proc-macro-server from starting

I don't understand what you're trying to say here. Simple terms: false stops server from starting, true doesn't and None stops it as well for the time being (because lack of config)?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 42 at r4 (raw file):

    fn new(
        connection: ProcMacroServerConnection,
        status_change: ProcMacroClientStatusChange,

The naming is off, i would say it's a status changer, or manager, or something else along those lines

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 91 at r4 (raw file):

        ensure!(
            id == 0,
            "fetching defined macros should be first sended request, it is {id} (zero counting)"

Suggestion:

            "fetching defined macros should be the first sent request, expected id=0 instead it is: {id}"

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 104 at r4 (raw file):

        ensure!(
            response.id == id,
            "fetching defined macros should be waited before any other request is send, received \

Suggestion:

            "fetching defined macros should be done before any other request is sent, received \

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 125 at r4 (raw file):

                id,
                method: M::METHOD.to_string(),
                value: serde_json::to_value(params).unwrap(),

I would give context, what exactly failed here

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 128 at r4 (raw file):

            })
            .with_context(|| anyhow!("sending request {id} failed"))
            .map(|_| id)

Why bother mapping it, when you return id from outer scope

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 134 at r4 (raw file):

        &self,
        params: M::Params,
        map: impl FnOnce(M::Params) -> RequestParams,

Why apply the mapping here instead of the calling function?

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 142 at r4 (raw file):

        self.set_macro_plugins(macro_plugins);
        self.set_analyzer_plugins(analyzer_plugins);
        self.set_inline_macro_plugins(Arc::new(inline_macro_plugins));

I guess this is a topic out of scope but:
Why are only inline macro plugins wrapped in an Arc ?

[Draggu]

Reviewable status: 10 of 19 files reviewed, 29 unresolved discussions (waiting on @Arcticae, @mkaput, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 105 at r3 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Let's rename the parameters so that they better reflect what is actually their purpose 🙏

Done.

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 76 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

It'd be good to extract this as an init_* function like init_files_group

Done.

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 111 at r3 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I agree that the comment will be forgotten, but code line kind of makes sense, isn't it? Maybe let's just remove this comment?

Done.

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 111 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

it would be hard to track it if we need it in future.

WDYM?

If this code was not added here now, despite the fact that it is not used now, finding source of this bug would be hard. Removing comment as @mkaput suggested.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 42 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

The naming is off, i would say it's a status changer, or manager, or something else along those lines

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 125 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I would give context, what exactly failed here

You mean this part serde_json::to_value(params).unwrap()? It can not fail.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 128 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Why bother mapping it, when you return id from outer scope

Could use ?; Ok(id) as well, can change if you see it more readable

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 134 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Why apply the mapping here instead of the calling function?

Different params types, see RequestParams

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/status.rs line 12 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

In what case can this fail? Is this something we can account for?

When lock is poisoned, and lock can be poisoned if thread that had this lock panicked, there is only 1 place where it can happen and only if there is \0 problem, we get more meaningful message there and can not recover from this easily, so failing here is reasonable option.

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 7 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why is this so important? we are not a library crate anyway

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 26 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why is this important?

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 53 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why is this important?

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Isn't there a case where we want to restart?

WDYM?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 91 at r4 (raw file):

        ensure!(
            id == 0,
            "fetching defined macros should be first sended request, it is {id} (zero counting)"

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 104 at r4 (raw file):

        ensure!(
            response.id == id,
            "fetching defined macros should be waited before any other request is send, received \

Done.

[Arcticae]

Reviewable status: 10 of 19 files reviewed, 23 unresolved discussions (waiting on @Draggu, @mkaput, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 128 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Could use ?; Ok(id) as well, can change if you see it more readable

Yep i'd do that

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 134 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Different params types, see RequestParams

Still, it's not exactly necessary to do this tbh, you can pass RequestParams here directly, and have the outside function provide two different param types. Less cognitive load

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Isn't there a case where we want to restart?

WDYM?

Restart the whole procmacro server, so deliberately kill the process and respawn it

[Draggu]

Reviewable status: 10 of 19 files reviewed, 23 unresolved discussions (waiting on @Arcticae, @mkaput, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/db/mod.rs line 142 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I guess this is a topic out of scope but:
Why are only inline macro plugins wrapped in an Arc ?

cairo/crates/cairo-lang-defs/src/db.rs

Line 99 in a952721

fn inline_macro_plugins(&self) -> Arc<OrderedHashMap<String, Arc<dyn InlineMacroExprPlugin>>>;

This is how inline macros are defined.

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 116 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

How big of a problem is that exactly? What's the estimated size of those maps when accumulating those results in practise?

Did not checked this.

---

crates/cairo-lang-language-server/src/lang/proc_macros/db.rs line 34 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Correct me if i'm wrong, but shouldn't this (cache vs query) be also handled by salsa somehow? (instead of using a map for input here)

It is cached on query level (when we call this in plugins) but request is not waited here so we need to provide this external state with input here.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 134 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Still, it's not exactly necessary to do this tbh, you can pass RequestParams here directly, and have the outside function provide two different param types. Less cognitive load

This won't work, with this approach we have to then match on RequestParams to unwrap it and also make send_request_untracked untyped. Current solutions is much clearer than this.

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 34 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

What's this code for exactly?

It is part of compiler plugin system, so compiler will actually run our macros.

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 103 at r6 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I feel like this function handles two separate concerns, do we really need it?

Here is stuff we have to do on idle_job, keeping it here instead exposing all details it use is better i think. If you think that we can rename this fn to express intent more clearly, im ok with that.

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 108 at r6 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

It needs an issue, doesn't it?

More like rethink if we need this comment at all

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 117 at r6 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This is more than that, it's status update, and an action in tandem

WDYM?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Restart the whole procmacro server, so deliberately kill the process and respawn it

Every time some proc-macro-server related operation fails and there are restart attempts left.

[piotmag769]

Reviewed 2 of 9 files at r7, all commit messages.
Reviewable status: 12 of 19 files reviewed, 24 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

to abort this waiting

We won't wait for it though since it is deatched, no? I mean the proc-macro-server process will not die until client.initialize()

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 36 at r7 (raw file):

                if bytes == 0 {
                    // Wait for process to exit as it will not produce any more data.
                    let _ = proc_macro_server.wait();

Now I realised - when do we ever kill this process? E.g., when sender.send returns Err then this process becomes a zombie, right?

[Draggu]

Reviewable status: 12 of 19 files reviewed, 24 unresolved discussions (waiting on @Arcticae, @mkaput, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 36 at r7 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Now I realised - when do we ever kill this process? E.g., when sender.send returns Err then this process becomes a zombie, right?

This process dies when it's pipe is broken, if you want double check if it is closed we can call .kill() here too.

[piotmag769]

Reviewable status: 12 of 19 files reviewed, 26 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 21 at r7 (raw file):

        let (requester, receiver) = crossbeam::channel::bounded(0);

        std::thread::spawn(move || {

Don't we want to store the io threads somewhere to join them when they are not needed? Or are we fine with leaving them detached?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 60 at r7 (raw file):

        });

        std::thread::spawn(move || {

It could be named to check manually if they are not detached and hanging there forever

[piotmag769]

Reviewed 7 of 9 files at r7.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 36 at r7 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

This process dies when it's pipe is broken, if you want double check if it is closed we can call .kill() here too.

Up 2 u, I guess we could?

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 117 at r6 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

WDYM?

handle_status_change maybe?

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

to abort this waiting

We won't wait for it though since it is deatched, no? I mean the proc-macro-server process will not die until client.initialize()

(continuation, forgot about this comment) (...) finishes. However it will die if sth goes wrong there:

• we drop the ProcMacroClient if there is an error ->
• which drops ProcMacroServerConnection ->
• which makes threads created and detached in ProcMacroServerConnection::new end ->
• which makes file descriptors for stdout and stding of proc-macro-server drop ->
• which terminates proc macro server (it is coded this way on Scarb's side)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 105 at r7 (raw file):

            response.id == id,
            "fetching defined macros should be done before any other request is sent, received \
             for id: {} <- should be {id}",

Suggestion:

            "fetching defined macros should be done before any other request is sent, received \
             response id: {}, expected {id}",

[Draggu]

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

---

crates/cairo-lang-language-server/src/config.rs line 46 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I don't understand what you're trying to say here. Simple terms: false stops server from starting, true doesn't and None stops it as well for the time being (because lack of config)?

Done.

---

crates/cairo-lang-language-server/src/lib.rs line 298 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Why is it run on each sync task? isn't it a bit excessive?

Most of the time it is simple if so performance is not affected, however this can be done in config reload response handler, will move it there.

---

crates/cairo-lang-language-server/src/lib.rs line 366 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

this is very bad, you are still busy waiting basically. there's no way OS/CPU will ever put our process in the background as it will never reliably hang on IO and this will be bad for battery life for users

maybe you need to select! here?

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/db.rs line 11 at r6 (raw file):

Previously, mkaput (Marek Kaput) wrote…

caching could be a separate PR

Discussed offline. Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 36 at r7 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Up 2 u, I guess we could?

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 128 at r4 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Yep i'd do that

Moved ID to params so no need to return it anymore.

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 117 at r6 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

handle_status_change maybe?

Done.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 158 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Still relevant? If so we should have an issue for that

Fixed. Discussed offline.

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/mod.rs line 105 at r7 (raw file):

            response.id == id,
            "fetching defined macros should be done before any other request is sent, received \
             for id: {} <- should be {id}",

Done.

[piotmag769]

Reviewed 14 of 14 files at r8, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

(continuation, forgot about this comment) (...) finishes. However it will die if sth goes wrong there:

• we drop the ProcMacroClient if there is an error ->
• which drops ProcMacroServerConnection ->
• which makes threads created and detached in ProcMacroServerConnection::new end ->
• which makes file descriptors for stdout and stding of proc-macro-server drop ->
• which terminates proc macro server (it is coded this way on Scarb's side)

@Arcticae @mkaput resolving since we no longer detach it. @Draggu resolve if no one has any remarks

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 23 at r8 (raw file):

impl ProcMacroServerConnection {
    pub fn new(mut proc_macro_server: std::process::Child, response_notify: Sender<()>) -> Self {

This could be better abstracted, but didn't think about how

Suggestion:

response_channel_sender

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 147 at r8 (raw file):

            }
            ClientStatus::Ready(client) => {
                // TODO we should check here if there are live snapshots, but no idea if it is

Marking it here so we don't forget about the TODO

[Arcticae]

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

---

crates/cairo-lang-language-server/src/lang/db/swapper.rs line 116 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Did not checked this.

It's a topic that i feel like we shouldn't ignore tbh. Let's assess how big of an issue this is, and then we can properly prioritize it

---

crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs line 34 at r4 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

It is part of compiler plugin system, so compiler will actually run our macros.

It needs docs

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 103 at r6 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Here is stuff we have to do on idle_job, keeping it here instead exposing all details it use is better i think. If you think that we can rename this fn to express intent more clearly, im ok with that.

if it's not reused i don't see a benefit, if those actions are not related. This is a nitpick though so resolve if you feel it's fine

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/controller.rs line 91 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

@Arcticae @mkaput resolving since we no longer detach it. @Draggu resolve if no one has any remarks

LGTM

[Draggu]

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @Arcticae and @mkaput)

---

crates/cairo-lang-language-server/src/lang/proc_macros/controller.rs line 103 at r6 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

if it's not reused i don't see a benefit, if those actions are not related. This is a nitpick though so resolve if you feel it's fine

When removing busy waiting this functions was changed, probably outdated.

[Draggu]

Reviewable status: 6 of 20 files reviewed, 14 unresolved discussions (waiting on @integraledelebesgue, @mkaput, and @piotmag769)

---

crates/cairo-lang-language-server/src/lang/proc_macros/client/connection.rs line 23 at r8 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This could be better abstracted, but didn't think about how

Done.