LS: Rewrite from `tower_lsp` to `lsp_server` #6431

Draggu · 2024-09-27T11:30:28Z

Closes #6132
Closes #6330

Rewrite done by @piotmag769 with additional test fixes.

This change is

orizi

Reviewed 5 of 61 files at r1.
Reviewable status: 5 of 61 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @mkaput, and @piotmag769)

clippy.toml line 2 at r1 (raw file):

enum-variant-name-threshold = 1
ignore-interior-mutability = ["lsp_types::Uri"]

is the issue is very local?
if so - i'd rather have #[allow(clippy::ignore_interior_mutability)] in that area.

Code quote:

ignore-interior-mutability = ["lsp_types::Uri"]

Arcticae

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

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

        (init, client)
    }

Shouldn't this be wrapped in cfg[test] ?

Code quote:

    fn new_for_testing(
        tricks: Tricks,
    ) -> (Box<dyn FnOnce() -> Self + Send>, lsp_server::Connection) {
        let (connection_initializer, client) = ConnectionInitializer::memory();

        let init = Box::new(|| Self::new_inner(tricks, connection_initializer).unwrap());

        (init, client)
    }

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

        let dynamic_registrations = collect_dynamic_registrations(&state.client_capabilities);

        let mut scheduler = Scheduler::new(&mut state, worker_threads, connection.make_sender());

[nit]: Wouldn't it make sense to extract it higher up so this loop only handles execution?

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

        };

        scheduler.request::<lsp_types::request::RegisterCapability>(

Is this happening before the whole handling loop? I think that was the assumption here?

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

    #[tracing::instrument(level = "debug", skip_all)]
    fn refresh_diagnostics(state: &mut State, notifier: &Notifier) -> LSPResult<()> {
        // TODO: implement a pop queue of size 1 for diags

Can we mark it with an issue number if it's not something we want to address here?

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

        })
        .await
    }

What happened to those?

Code quote:

    #[tracing::instrument(level = "trace", skip_all)]
    async fn view_analyzed_crates(&self) -> LSPResult<String> {
        let db = self.db_snapshot().await;
        self.catch_panics(move || lang::inspect::crates::inspect_analyzed_crates(&db)).await
    }

    #[tracing::instrument(level = "trace", skip_all)]
    async fn expand_macro(&self, params: TextDocumentPositionParams) -> LSPResult<Option<String>> {
        let db = self.db_snapshot().await;
        self.catch_panics(move || ide::macros::expand::expand_macro(&db, &params)).await
    }

    #[tracing::instrument(level = "trace", skip_all)]
    async fn vfs_provide(
        &self,
        params: ProvideVirtualFileRequest,
    ) -> LSPResult<ProvideVirtualFileResponse> {
        let db = self.db_snapshot().await;
        self.catch_panics(move || {
            let content = db
                .file_for_url(&params.uri)
                .and_then(|file_id| db.file_content(file_id))
                .map(|content| content.to_string());
            ProvideVirtualFileResponse { content }
        })
        .await
    }

crates/cairo-lang-language-server/src/state.rs line 82 at r2 (raw file):

    pub _config: Snapshot<Config>,
    pub _client_capabilities: Snapshot<ClientCapabilities>,
    pub _tricks: Snapshot<Tricks>,

What's the point here?

Code quote:

    pub _open_files: Snapshot<HashSet<Uri>>,
    pub _config: Snapshot<Config>,
    pub _client_capabilities: Snapshot<ClientCapabilities>,
    pub _tricks: Snapshot<Tricks>,

crates/cairo-lang-language-server/src/lsp/controller.rs line 102 at r2 (raw file):

        _params: CancelParams,
    ) -> LSPResult<()> {
        Ok(())

This is a cancellation request that we were not handling, right?

crates/cairo-lang-language-server/src/lsp/controller.rs line 140 at r2 (raw file):

        _params: DidChangeConfigurationParams,
    ) -> LSPResult<()> {
        // TODO it was this way but we shouldn't reload here, just read changes from params

Is this addressable?

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 131 at r2 (raw file):

                    assert_eq!(res_id, id);

                    // TODO

TODO?

mkaput

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

clippy.toml line 2 at r1 (raw file):

Previously, orizi wrote…

is the issue is very local?
if so - i'd rather have #[allow(clippy::ignore_interior_mutability)] in that area.

Tip: since 1.81 it's better to use #[expect(clippy::ignore_interior_mutability)] so we can track obsolete lint allows

crates/bin/cairo-language-server/src/main.rs line 2 at r2 (raw file):

fn main() {
    cairo_lang_language_server::start();

unnecessary change?

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

libc = "0.2.155"
jod-thread = "0.1.2"
url = "2.5.2"

sort deps alphabetically

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

        Ok(backend) => {
            if let Err(err) = backend.run().map(|handle| handle.join()) {
                error!("language server encountered an unrecoverable error: {err}")

you should set exit code here

spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit

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

        let (profile_layer, profile_layer_guard) = ChromeLayerBuilder::new()
            .writer(profile_file)
            .trace_style(TraceStyle::Async)

awesome that you caught this 👌

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

Previously, Arcticae (Tomasz Rejowski) wrote…

Shouldn't this be wrapped in cfg[test] ?

yes it should, but in #[cfg(feature = "testing")]

mkaput

Reviewed 28 of 61 files at r1, 5 of 7 files at r2.
Reviewable status: 42 of 61 files reviewed, 14 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

Cargo.lock line 2423 at r2 (raw file):

[[package]]
name = "lsp-types"
version = "0.97.0"

I see a lot of stuff is being changed here just because of upgrading this crate.

For example, see that there is quite a fuss about Url -> Uri change upstream: gluon-lang/lsp-types#284

Also, this is a minefield for sneaking a breaking change if you think about this.

Please, let's just stay with 0.94.1 for now.

mkaput

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

mkaput

Reviewed 3 of 61 files at r1, 1 of 7 files at r2.
Reviewable status: 46 of 61 files reviewed, 15 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

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

lsp-types = "0.97.0"
rustc-hash = "1.1.0"
libc = "0.2.155"

this is a target-specific dependency so please pull this one only on targets that actually do use it

mkaput

Reviewed 4 of 61 files at r1.
Reviewable status: 50 of 61 files reviewed, 15 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

mkaput

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

crates/cairo-lang-language-server/src/lsp/mod.rs line 2 at r2 (raw file):

pub(crate) mod capabilities;
pub mod controller;

why is this becoming pub?

mkaput

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

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

impl Notifier {
    pub fn notify<N>(&self, params: N::Params) -> Result<()>

is this result used anywhere? can't we just log the error? this is making usage quite uglier (see ScarbToolchain changes)

crates/cairo-lang-language-server/src/server/client.rs line 151 at r2 (raw file):

            handler(response)
        } else {
            tracing::error!("Received a response with ID {}, which was not expected", response.id);

etc.

Suggestion:

error!("received a response with ID {}, which was not expected", response.id);

crates/cairo-lang-language-server/src/server/connection.rs line 82 at r2 (raw file):

            {
                self.sender.send(lsp::Response::new_ok(id.clone(), ()).into())?;
                tracing::info!("Shutdown request received. Waiting for an exit notification...");

etc. everywhere in this file

Suggestion:

info!("shutdown request received. waiting for an exit notification");

crates/cairo-lang-language-server/src/server/schedule/task.rs line 19 at r2 (raw file):

    /// The task should be run on the background thread designated
    /// for formatting actions. This is a high priority thread.
    Fmt,

seems like a blind copy-paste from ruff?

can you add source credit comments everywhere applicable?
drop this, we're not ruff

crates/cairo-lang-language-server/src/server/schedule/task.rs line 81 at r2 (raw file):

        Self::local(move |_, _, _, responder| {
            if let Err(err) = responder.respond(id, result) {
                tracing::error!("Unable to send immediate response: {err}");

Suggestion:

tracing::error!("unable to send immediate response: {err}");

mkaput

Reviewed 4 of 61 files at r1.
Reviewable status: 56 of 61 files reviewed, 39 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

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

rustc-hash = "1.1.0"
libc = "0.2.155"
jod-thread = "0.1.2"

unused?

crates/cairo-lang-language-server/src/server/connection.rs line 3 at r2 (raw file):

use std::sync::{Arc, Weak};

use lsp_server as lsp;

why this rename?

crates/cairo-lang-language-server/src/server/connection.rs line 43 at r2 (raw file):

    pub fn initialize_start(
        &self,
    ) -> anyhow::Result<(lsp::RequestId, lsp_types::InitializeParams)> {

use anyhow::Result

crates/cairo-lang-language-server/src/server/connection.rs line 82 at r2 (raw file):

            {
                self.sender.send(lsp::Response::new_ok(id.clone(), ()).into())?;
                tracing::info!("Shutdown request received. Waiting for an exit notification...");

etc

Suggestion:

info!("shutdown request received, waiting for an exit notification");

crates/cairo-lang-language-server/src/server/connection.rs line 90 at r2 (raw file):

                        Ok(true)
                    }
                    message => anyhow::bail!(

Suggestion:

bail!

crates/cairo-lang-language-server/src/server/api/mod.rs line 1 at r2 (raw file):

use lsp_server as server;

why

crates/cairo-lang-language-server/src/server/api/mod.rs line 21 at r2 (raw file):

use crate::state::State;

pub(crate) fn request<'a>(req: server::Request) -> Task<'a> {

this routing seems to be hidden quite deep in lsp implementation internals. shouldn't it be in lib.rs or somewhere nearby?

crates/cairo-lang-language-server/src/server/api/mod.rs line 53 at r2 (raw file):

        method => {
            tracing::warn!("Received request {method} which does not have a handler");

etc.

Suggestion:

warn!("received request {method} which does not have a handler");

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

    }
    .unwrap_or_else(|err| {
        tracing::error!("Encountered error when routing request with ID {id}: {err}");

always use debug impls for anyhow errors, their Display impl does not show error cause stack

Suggestion:

{err:?}

crates/cairo-lang-language-server/src/server/api/mod.rs line 64 at r2 (raw file):

}

pub(crate) fn notification<'a>(notif: server::Notification) -> Task<'a> {

same applies to this routing proc

crates/cairo-lang-language-server/src/server/api/mod.rs line 191 at r2 (raw file):

/// A trait to convert result types into the server result type, [`LSPResult`].
pub trait LSPResultConversionTrait<T> {

I like the contention of using Ex suffix for extension traits (because this is one, definitely it's not conversion)

Suggestion:

LSPResultEx

crates/cairo-lang-language-server/src/server/api/mod.rs line 219 at r2 (raw file):

        self.error.fmt(f)
    }
}

Suggestion:

// Right now, we treat the error code as invisible data that won't
// be printed.
impl fmt::Debug for Error {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.error.fmt(f)
    }
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.error.fmt(f)
    }
}

crates/cairo-lang-language-server/src/server/schedule/mod.rs line 20 at r2 (raw file):

pub(crate) fn event_loop_thread(
    func: impl FnOnce() -> anyhow::Result<()> + Send + 'static,
) -> anyhow::Result<thread::JoinHandle<anyhow::Result<()>>> {

Suggestion:

pub(crate) fn event_loop_thread(
    func: impl FnOnce() -> Result<()> + Send + 'static,
) -> Result<thread::JoinHandle<Result<()>>> {

crates/cairo-lang-language-server/src/server/schedule/mod.rs line 23 at r2 (raw file):

    // Override OS defaults to avoid stack overflows on platforms with low stack size defaults.
    const MAIN_THREAD_STACK_SIZE: usize = 2 * 1024 * 1024;
    const MAIN_THREAD_NAME: &str = "cairo-ls:main";

Suggestion:

const MAIN_THREAD_NAME: &str = "cairols:main";

crates/cairo-lang-language-server/src/server/schedule/mod.rs line 39 at r2 (raw file):

impl<'s> Scheduler<'s> {
    pub fn new(state: &'s mut State, worker_threads: NonZeroUsize, sender: ClientSender) -> Self {
        const FMT_THREADS: usize = 1;

we're not ruff so we don't need dedicated fmt threads

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

                        if let Err(err) = self.notifier.notify::<ScarbPathMissing>(()) {
                            error!("failed to send scarb path missing notification: {err:#}");

this log is irrelevant in this context

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

        if !self.is_silent {
            self.notifier.notify::<ScarbResolvingStart>(())?;

as I said earlier: failure to send a notification should not kill this procedure

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

        if !self.is_silent {
            self.notifier.notify::<ScarbResolvingFinish>(())?;

as I said earlier: failure to send a notification should not kill this procedure

mkaput

Reviewed 5 of 61 files at r1, 1 of 7 files at r2.
Reviewable status: 60 of 61 files reviewed, 40 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

crates/cairo-lang-language-server/src/state.rs line 82 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

What's the point here?

agree, if this is not used by snapshot then these fields should just disappear

crates/cairo-lang-language-server/src/lsp/controller.rs line 0 at r2 (raw file):
shouldn't this code lie closed to lib.rs?

crates/cairo-lang-language-server/src/lsp/controller.rs line 102 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This is a cancellation request that we were not handling, right?

I think this should be a TODO

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#cancelRequest

crates/cairo-lang-language-server/src/lsp/controller.rs line 140 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Is this addressable?

this comment is wrong, params of this request are deprecated and the recommendation is to request back for full or partial config

Arcticae

Reviewable status: 60 of 61 files reviewed, 40 unresolved discussions (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)

crates/cairo-lang-language-server/src/server/connection.rs line 3 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why this rename?

i quite like it tbh

mkaput

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

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

}

pub struct Backend {

why is this becoming pub? we want to limit amount of stuff that is publicly available from the crate

https://docs.rs/cairo-lang-language-server/latest/cairo_lang_language_server/

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

    }

    pub fn run(self) -> Result<JoinHandle<Result<()>>> {

why pub?

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

        event_loop_thread(move || {
            Self::event_loop(&self.connection, self.state, worker_threads)?;
            self.connection.close()?;

shouldn't we close the connection always, regardless of what Self::event_loop returns?

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

Previously, Arcticae (Tomasz Rejowski) wrote…

[nit]: Wouldn't it make sense to extract it higher up so this loop only handles execution?

👍

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

        // Register dynamic capabilities.
        let response_handler = |()| {
            info!("Configuration file watcher successfully registered");

no need to be so verbose

Suggestion:

debug!("configuration file watcher successfully registered");

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

Previously, Arcticae (Tomasz Rejowski) wrote…

Is this happening before the whole handling loop? I think that was the assumption here?

I think that's part of the initialize procedure (this was previously done within initialize request handler)

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

        )?;

        scheduler.dispatch(Task::Sync(SyncTask {

please add a comment explaining why we are eagerly triggering this code path

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

        // After handling of all diagnostics, attempting to swap the database to reduce memory
        // consumption.
        // This should be removed from here when diags are background job.

I'd rather tell here that this should be an independent job, that is maybe run periodically. Wdyt?

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

    fn get_files_modules<'a>(
        db: &AnalysisDatabase,
        files_ids: impl Iterator<Item = &'a FileId>,

I wonder: FileId is Copy so why can't we just use .copied() iterator here?

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

    #[tracing::instrument(level = "trace", skip_all)]
    fn detect_crate_for(
        scarb_toolchain: &ScarbToolchain,

move this parameter further down, so that db comes first by convention

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

Previously, Arcticae (Tomasz Rejowski) wrote…

What happened to those?

@Draggu moved them into the controller module

mkaput

Reviewable status: 60 of 61 files reviewed, 48 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

crates/cairo-lang-language-server/src/server/connection.rs line 3 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

i quite like it tbh

imho it obfuscates paths further down, and also I don't like that this rename is not uniform. sometimes there's lsp, sometimes there's server

mkaput

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

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 56 at r2 (raw file):

        };

        std::thread::spawn(|| init().run());

don't we want to join this thread while dropping the client? can this thread become zombie?

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 82 at r2 (raw file):

        });

        self.expect_request::<RegisterCapability>(|_req| {});

?

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 109 at r2 (raw file):

            match response_message {
                Message::Notification(_) => {
                    // This looks like a notification, skip it.

Suggestion:

// Skip notifications.

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 141 at r2 (raw file):

                    //     "expected more requests to be received from the client while \
                    //          processing the current server one: {expect_request_handlers:?}"
                    // );

if lsp_server is reordering messages, then I don't think it's possible to rewrite this assertion in a meaningful way. it's safe to drop it imo

piotmag769

Reviewed 39 of 61 files at r1, 1 of 7 files at r2, 5 of 26 files at r3.
Reviewable status: 33 of 61 files reviewed, 52 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

Cargo.lock line 2423 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I see a lot of stuff is being changed here just because of upgrading this crate.

For example, see that there is quite a fuss about Url -> Uri change upstream: gluon-lang/lsp-types#284

Also, this is a minefield for sneaking a breaking change if you think about this.

Please, let's just stay with 0.94.1 for now.

Changed to 0.95.0 since I need Request::Params to enforce Send which is not done in 0.94.1

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

Previously, mkaput (Marek Kaput) wrote…

this is a target-specific dependency so please pull this one only on targets that actually do use it

Done, good catch.

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

Previously, mkaput (Marek Kaput) wrote…

unused?

Nope, it is used

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

Previously, mkaput (Marek Kaput) wrote…

sort deps alphabetically

Done

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

Previously, mkaput (Marek Kaput) wrote…

yes it should, but in #[cfg(feature = "testing")]

Done.

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

Previously, mkaput (Marek Kaput) wrote…

why pub?

Done.

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

Previously, mkaput (Marek Kaput) wrote…

no need to be so verbose

Done.

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

Previously, mkaput (Marek Kaput) wrote…

I think that's part of the initialize procedure (this was previously done within initialize request handler)

Not neccessarily.

crates/cairo-lang-language-server/src/state.rs line 82 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

agree, if this is not used by snapshot then these fields should just disappear

If we remove them there is no point of our custom Snapshot struct even existing. I believe @Draggu wanted me to leave these to do some magic with diagnostics, but I am not sure if it is still relevant?

crates/cairo-lang-language-server/src/lsp/controller.rs line 102 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I think this should be a TODO

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#cancelRequest

@Draggu told me that he discussed it with @mkaput some time ago and that you told him this is the way it should be

crates/cairo-lang-language-server/src/lsp/controller.rs line 140 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

this comment is wrong, params of this request are deprecated and the recommendation is to request back for full or partial config

Done.

crates/cairo-lang-language-server/src/lsp/controller.rs line at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

shouldn't this code lie closed to lib.rs?

It should be in server.rs to be fair, but I didn't want to move this in this PR, will be done in a separate one

crates/cairo-lang-language-server/src/server/connection.rs line 3 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

imho it obfuscates paths further down, and also I don't like that this rename is not uniform. sometimes there's lsp, sometimes there's server

This file is copied almost 1:1 from ruff, will change it

crates/cairo-lang-language-server/src/server/connection.rs line 43 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

use anyhow::Result

Done.

crates/cairo-lang-language-server/src/server/connection.rs line 82 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

etc. everywhere in this file

Done.

crates/cairo-lang-language-server/src/server/connection.rs line 82 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

etc

Done.

crates/cairo-lang-language-server/src/server/connection.rs line 90 at r2 (raw file):

                        Ok(true)
                    }
                    message => anyhow::bail!(

Done.

crates/cairo-lang-language-server/src/server/api/mod.rs line 1 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why

Done, copied from ruff.

crates/cairo-lang-language-server/src/server/api/mod.rs line 53 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

etc.

Done.

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

Previously, mkaput (Marek Kaput) wrote…

always use debug impls for anyhow errors, their Display impl does not show error cause stack

This isn't an anyhow error but this

pub(crate) struct LSPError {  
    pub(crate) code: ErrorCode,  
    pub(crate) error: anyhow::Error,  
}

crates/cairo-lang-language-server/src/server/api/mod.rs line 191 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I like the contention of using Ex suffix for extension traits (because this is one, definitely it's not conversion)

Done.

crates/cairo-lang-language-server/src/server/api/mod.rs line 219 at r2 (raw file):

        self.error.fmt(f)
    }
}

Done.

crates/cairo-lang-language-server/src/server/schedule/mod.rs line 20 at r2 (raw file):

pub(crate) fn event_loop_thread(
    func: impl FnOnce() -> anyhow::Result<()> + Send + 'static,
) -> anyhow::Result<thread::JoinHandle<anyhow::Result<()>>> {

Done.

crates/cairo-lang-language-server/src/server/schedule/mod.rs line 23 at r2 (raw file):

    // Override OS defaults to avoid stack overflows on platforms with low stack size defaults.
    const MAIN_THREAD_STACK_SIZE: usize = 2 * 1024 * 1024;
    const MAIN_THREAD_NAME: &str = "cairo-ls:main";

Done.

crates/cairo-lang-language-server/src/server/schedule/mod.rs line 39 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

we're not ruff so we don't need dedicated fmt threads

Done.

crates/cairo-lang-language-server/src/server/schedule/task.rs line 19 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

seems like a blind copy-paste from ruff?

can you add source credit comments everywhere applicable?

drop this, we're not ruff

Sure

crates/cairo-lang-language-server/src/server/schedule/task.rs line 81 at r2 (raw file):

        Self::local(move |_, _, _, responder| {
            if let Err(err) = responder.respond(id, result) {
                tracing::error!("Unable to send immediate response: {err}");

Done.

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

Previously, mkaput (Marek Kaput) wrote…

this log is irrelevant in this context

Why?

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

Previously, mkaput (Marek Kaput) wrote…

as I said earlier: failure to send a notification should not kill this procedure

Done.

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

Previously, mkaput (Marek Kaput) wrote…

as I said earlier: failure to send a notification should not kill this procedure

Done.

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 56 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

don't we want to join this thread while dropping the client? can this thread become zombie?

pub struct JoinHandle<T = ()> { 
    // `inner` is an `Option` so that we can
    // take ownership of the contained `JoinHandle`. 
    inner: Option<jod_thread::JoinHandle<T>>,  
    allow_leak: bool,  
}

Don't think so, it should join itself on drop considering this struct has jod_thread's JoinHandle

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 82 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

?

@Draggu ?

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 109 at r2 (raw file):

            match response_message {
                Message::Notification(_) => {
                    // This looks like a notification, skip it.

Done.

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 131 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

TODO?

Done.

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 141 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

if lsp_server is reordering messages, then I don't think it's possible to rewrite this assertion in a meaningful way. it's safe to drop it imo

Done.

clippy.toml line 2 at r1 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Tip: since 1.81 it's better to use #[expect(clippy::ignore_interior_mutability)] so we can track obsolete lint allows

Done, not relevant anymore.

crates/bin/cairo-language-server/src/main.rs line 2 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

unnecessary change?

Done.

orizi

Reviewed 3 of 10 files at r4, all commit messages.
Reviewable status: 36 of 61 files reviewed, 51 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @piotmag769)

piotmag769

Reviewed 3 of 61 files at r1, 17 of 26 files at r3, 7 of 10 files at r4, 7 of 7 files at r5, all commit messages.
Dismissed @Arcticae and @mkaput from 30 discussions.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

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

Previously, mkaput (Marek Kaput) wrote…

I'd rather tell here that this should be an independent job, that is maybe run periodically. Wdyt?

Done.

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

Previously, mkaput (Marek Kaput) wrote…

I wonder: FileId is Copy so why can't we just use .copied() iterator here?

Done, good catch

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

Previously, mkaput (Marek Kaput) wrote…

move this parameter further down, so that db comes first by convention

Done.

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

Previously, mkaput (Marek Kaput) wrote…

@Draggu moved them into the controller module

Yep

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

Previously, mkaput (Marek Kaput) wrote…

is this result used anywhere? can't we just log the error? this is making usage quite uglier (see ScarbToolchain changes)

Done.

crates/cairo-lang-language-server/src/server/client.rs line 151 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

etc.

Done.

crates/cairo-lang-language-server/src/server/connection.rs line 3 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This file is copied almost 1:1 from ruff, will change it

Done.

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

Previously, piotmag769 (Piotr Magiera) wrote…

This isn't an anyhow error but this

pub(crate) struct LSPError {  
    pub(crate) code: ErrorCode,  
    pub(crate) error: anyhow::Error,  
}

Done anyways.

crates/cairo-lang-language-server/src/server/schedule/task.rs line 19 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Sure

Done, will add sources after everything looks good

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

Previously, piotmag769 (Piotr Magiera) wrote…

Why?

Logging now happens in notify method

piotmag769

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

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

Previously, Arcticae (Tomasz Rejowski) wrote…

Can we mark it with an issue number if it's not something we want to address here?

Done.

crates/cairo-lang-language-server/src/lsp/mod.rs line 2 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

why is this becoming pub?

Done.

orizi

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

piotmag769

Reviewed 1 of 61 files at r1, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

piotmag769

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

Draggu

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

crates/cairo-lang-language-server/src/state.rs line 82 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

If we remove them there is no point of our custom Snapshot struct even existing. I believe @Draggu wanted me to leave these to do some magic with diagnostics, but I am not sure if it is still relevant?

Part of it will be used when changing way we calculate diagnostics. Please leave it as is now.

crates/cairo-lang-language-server/src/lsp/controller.rs line 102 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

@Draggu told me that he discussed it with @mkaput some time ago and that you told him this is the way it should be

@mkaput actually we don't need to support this handler as we know on server when something should be canceled so returning Ok make sense here. We discussed this earlier and stated that this is not important.

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 56 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

pub struct JoinHandle<T = ()> { 
    // `inner` is an `Option` so that we can
    // take ownership of the contained `JoinHandle`. 
    inner: Option<jod_thread::JoinHandle<T>>,  
    allow_leak: bool,  
}

Don't think so, it should join itself on drop considering this struct has jod_thread's JoinHandle

Yep, threads join on drop

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 82 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

@Draggu ?

Was required to pass assert that is commented out later but since we remove it anyway this should be possibly to remove.

piotmag769

Dismissed @Arcticae and @mkaput from 3 discussions.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

crates/cairo-lang-language-server/tests/e2e/support/mock_client.rs line 82 at r2 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Was required to pass assert that is commented out later but since we remove it anyway this should be possibly to remove.

It is needed, without it tests fail

piotmag769

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

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

Previously, mkaput (Marek Kaput) wrote…

you should set exit code here

spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#exit

Done.

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

Previously, mkaput (Marek Kaput) wrote…

why is this becoming pub? we want to limit amount of stuff that is publicly available from the crate

https://docs.rs/cairo-lang-language-server/latest/cairo_lang_language_server/

Done, but it's ugly, probably can be refactored more, but I don't think it's worth it now

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

Previously, mkaput (Marek Kaput) wrote…

shouldn't we close the connection always, regardless of what Self::event_loop returns?

Done.

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

Previously, mkaput (Marek Kaput) wrote…

👍

Done.

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

Previously, mkaput (Marek Kaput) wrote…

please add a comment explaining why we are eagerly triggering this code path

Done.

crates/cairo-lang-language-server/src/lsp/controller.rs line at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

It should be in server.rs to be fair, but I didn't want to move this in this PR, will be done in a separate one

Moved to api/traits.rs

piotmag769

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

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

Is it happening before the whole handling loop?

It is not guaranteed that the response handler executes before every other request from client, but @Draggu investigated it and found no way to guarantee it.

I think that's part of the initalize prodecure

Was previously done as a reaction to initialized notification, but do to the same way I would have to do it in initialize_finish which is quite inconvenient. It doesn't really matter that we do it here instead imo

piotmag769

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

LS: Rewrite to lsp_server

3d0da24

Draggu requested review from orizi, mkaput, Arcticae and piotmag769 as code owners September 27, 2024 11:30

Draggu linked an issue Sep 27, 2024 that may be closed by this pull request

Rewrite from tower-lsp to lsp-server #6132

Open

LS: Fix tests

f72fedf

Draggu force-pushed the 6132-rewrite-from-tower-lsp-to-lsp-server branch from 41e44fa to f72fedf Compare September 27, 2024 11:40

LS: fix tracing

c88b631

orizi requested changes Sep 30, 2024

View reviewed changes

Arcticae requested changes Sep 30, 2024

View reviewed changes

mkaput requested changes Sep 30, 2024

View reviewed changes

mkaput reviewed Sep 30, 2024

View reviewed changes

piotmag769 self-assigned this Sep 30, 2024

mkaput requested changes Oct 1, 2024

View reviewed changes

mkaput reviewed Oct 1, 2024

View reviewed changes

mkaput requested changes Oct 1, 2024

View reviewed changes

Arcticae reviewed Oct 1, 2024

View reviewed changes

mkaput requested changes Oct 1, 2024

View reviewed changes

piotmag769 added 2 commits October 2, 2024 21:57

wip

304c6ed

review

d88daef

piotmag769 reviewed Oct 2, 2024

View reviewed changes

review wip

0738f6e

orizi reviewed Oct 2, 2024

View reviewed changes

fix tests

43e4567

piotmag769 requested a review from Arcticae October 3, 2024 10:49

piotmag769 reviewed Oct 3, 2024

View reviewed changes

review

a1533fe

piotmag769 reviewed Oct 3, 2024

View reviewed changes

remove url dep

952483c

orizi approved these changes Oct 3, 2024

View reviewed changes

piotmag769 reviewed Oct 3, 2024

View reviewed changes

review

b41fcd2

piotmag769 reviewed Oct 3, 2024

View reviewed changes

Draggu commented Oct 3, 2024

View reviewed changes

piotmag769 reviewed Oct 3, 2024

View reviewed changes

stuff

08a90bd

piotmag769 reviewed Oct 3, 2024

View reviewed changes

remove ruff

45618a1

piotmag769 reviewed Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LS: Rewrite from `tower_lsp` to `lsp_server` #6431

LS: Rewrite from `tower_lsp` to `lsp_server` #6431

Draggu commented Sep 27, 2024 •

edited

Loading

orizi left a comment

Arcticae left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

Arcticae left a comment

mkaput left a comment

mkaput left a comment

mkaput left a comment

piotmag769 left a comment

orizi left a comment

piotmag769 left a comment

piotmag769 left a comment

orizi left a comment

piotmag769 left a comment

piotmag769 left a comment

Draggu left a comment

piotmag769 left a comment

piotmag769 left a comment

piotmag769 left a comment

piotmag769 left a comment

LS: Rewrite from tower_lsp to lsp_server #6431

Are you sure you want to change the base?

LS: Rewrite from tower_lsp to lsp_server #6431

Conversation

Draggu commented Sep 27, 2024 • edited Loading

orizi left a comment

Choose a reason for hiding this comment

Arcticae left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

Arcticae left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

mkaput left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

Draggu left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

piotmag769 left a comment

Choose a reason for hiding this comment

LS: Rewrite from `tower_lsp` to `lsp_server` #6431

LS: Rewrite from `tower_lsp` to `lsp_server` #6431

Draggu commented Sep 27, 2024 •

edited

Loading