LS: asynchronous project update

[piotmag769]

No description provided.

[reviewable-StarkWare]

This change is 

[mkaput]

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

---

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

    }

    pub fn init_channels_and_spawn_thread(

it looks to me like you can now merge these two functions into just a variant of new that spawns threads, just like DiagnosticsController does. avoiding these OnceCell will be very good

it should be straightforward, I think, to save the receiver in ProjectController and somehow access it in the main event loop

note: and yes, 🤦 I just realised that I wrote a new function that does spawning

Code quote:

    pub fn new() -> Self {
        ProjectController { requests_sender: Default::default(), thread: Default::default() }
    }

    pub fn init_channels_and_spawn_thread(

---

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

    }

    pub fn handle_update(state: &mut State, notifier: Notifier, project_update: ProjectUpdate) {

I know you're just moving stuff but can you please doc?

---

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

    }

    pub fn handle_update(state: &mut State, notifier: Notifier, project_update: ProjectUpdate) {

also maybe add tracing instrument here?

---

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

            .expect("requests sender should have been set")
            .send(request)
            // TODO maybe error?

yes I think this could be a log error

---

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

}

pub enum ProjectUpdate {

I know you're just moving stuff but can you please doc?

---

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

        self.project_updates_sender
            .send(project_update)
            // TODO maybe error?

I think it's fine because it's programmers error to skip the init call

[piotmag769]

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

---

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

Previously, mkaput (Marek Kaput) wrote…

it looks to me like you can now merge these two functions into just a variant of new that spawns threads, just like DiagnosticsController does. avoiding these OnceCell will be very good

it should be straightforward, I think, to save the receiver in ProjectController and somehow access it in the main event loop

note: and yes, 🤦 I just realised that I wrote a new function that does spawning

I wish I could, but then I would have to use OnceCell to State since it is where ProjectController is initialised (and I need the update receiver channel only in the main loop). Alternatively I could have a channel hanging somewhere in the state or intermediate structure like ProjectControllerHandle which is also quite bad. LMK what is the best approach in your opinion.

I could do some weird tricks making sure it is all typesafe and pretty like transitioning between PartiallyInitializedState -> State and use type families and stuff, but I doubt that this is what we want xD

[mkaput]

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

---

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

Previously, piotmag769 (Piotr Magiera) wrote…

I wish I could, but then I would have to use OnceCell to State since it is where ProjectController is initialised (and I need the update receiver channel only in the main loop). Alternatively I could have a channel hanging somewhere in the state or intermediate structure like ProjectControllerHandle which is also quite bad. LMK what is the best approach in your opinion.

I could do some weird tricks making sure it is all typesafe and pretty like transitioning between PartiallyInitializedState -> State and use type families and stuff, but I doubt that this is what we want xD

Receiver: Clone so you can easily store it in state and just expose a method that clones it (I actually think the same could be done in proc macros now)

[piotmag769]

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

---

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

Previously, mkaput (Marek Kaput) wrote…

also maybe add tracing instrument here?

Done.

---

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

Previously, mkaput (Marek Kaput) wrote…

I know you're just moving stuff but can you please doc?

Done.

---

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

Previously, mkaput (Marek Kaput) wrote…

yes I think this could be a log error

Agree, but we should still expect here, no? If it happens we are doomed anyway since we won't be able to load any new project

---

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

Previously, mkaput (Marek Kaput) wrote…

I know you're just moving stuff but can you please doc?

Done.

---

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

Previously, mkaput (Marek Kaput) wrote…

I think it's fine because it's programmers error to skip the init call

Done.

[mkaput]

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

---

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

since we won't be able to load any new project

I know

but we should still expect here, no?

that's the part I'm not 100% sure. it kinda makes sense because this should eventually cause LS to crash, but I wonder if we could just restart this thread or smth internally

[piotmag769]

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

---

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

Previously, mkaput (Marek Kaput) wrote…

Receiver: Clone so you can easily store it in state and just expose a method that clones it (I actually think the same could be done in proc macros now)

Done.

[Draggu]

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

---

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

Previously, mkaput (Marek Kaput) wrote…

since we won't be able to load any new project

I know

but we should still expect here, no?

that's the part I'm not 100% sure. it kinda makes sense because this should eventually cause LS to crash, but I wonder if we could just restart this thread or smth internally

Agree we should error! here, expect seems fine to me.

---

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

    }

    pub fn update_project_for_file(&self, file_path: PathBuf) {

This does not update project, only schedule this for later, rename please.

---

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

    }

    pub fn clear_loaded_workspaces(&self) {

This does not clear workspaces, only schedule this for later, rename please.

---

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

        scarb_toolchain: ScarbToolchain,
    ) -> JoinHandle {
        let mut this = Self {

Instead of mut we can consume it in the event loop, up to you.

---

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

    /// Runs project controller's event loop.
    fn event_loop(&mut self) {
        while let Ok(request) = self.requests_receiver.recv() {

Suggestion:

for request in self.requests_receiver

---

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

    /// It is meant to be run only in the background thread.
    #[tracing::instrument(skip_all)]
    fn send_project_update_for_file(&mut self, file_path: &Path) {

We can make 2 methods of this, first calculating, second sending, up to you.

[piotmag769]

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

---

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

Previously, Draggu (Piotr Figiela) wrote…

Agree we should error! here, expect seems fine to me.

Done.

---

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

Previously, Draggu (Piotr Figiela) wrote…

This does not update project, only schedule this for later, rename please.

Done.

---

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

Previously, Draggu (Piotr Figiela) wrote…

This does not clear workspaces, only schedule this for later, rename please.

Done.

---

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

    /// Runs project controller's event loop.
    fn event_loop(&mut self) {
        while let Ok(request) = self.requests_receiver.recv() {

The current one is clearer for me

[piotmag769]

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

[Draggu]

Reviewed 1 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @integraledelebesgue, @mkaput, and @orizi)

[mkaput]

Reviewed 1 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @integraledelebesgue, and @orizi)

---

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

Previously, piotmag769 (Piotr Magiera) wrote…

Done.

but if we panic then we get the log anyway from the panic hook. it's unnecessary to do both imo

[piotmag769]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @integraledelebesgue, @mkaput, and @orizi)

---

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

Previously, mkaput (Marek Kaput) wrote…

but if we panic then we get the log anyway from the panic hook. it's unnecessary to do both imo

Done in new repo software-mansion/cairols#2

[piotmag769]

Closing in favour of software-mansion/cairols#2