Add bidirectional messaging proc-macro-srv #21249

Shourya742 · 2025-12-11T15:43:12Z

This PR adds a bidirectional proc-macro protocol so the server can ask the client for data while expanding a macro . The approach is still synchronous and single-request-at-a-time. The server may emit sub-requests during expansion, the client replies immediately, and the server then finishes with the final response. All messages use the same request id, and stdin/stdout remain strictly ordered. Legacy protocols are untouched

ChayimFriedman2

Overall good code, left some comments.

@Veykril would you like to review this too?

ChayimFriedman2 · 2025-12-21T06:33:55Z

crates/proc-macro-api/src/bidirectional_protocol/msg.rs

+impl From<(RequestId, Kind, Payload)> for Envelope {
+    fn from(value: (RequestId, Kind, Payload)) -> Self {
+        Envelope { id: value.0, kind: value.1, payload: value.2 }
+    }
+}


Suggested change

impl From<(RequestId, Kind, Payload)> for Envelope {

fn from(value: (RequestId, Kind, Payload)) -> Self {

Envelope { id: value.0, kind: value.1, payload: value.2 }

}

}

This is not used (and also not a good practice).

ChayimFriedman2 · 2025-12-21T06:36:03Z

crates/proc-macro-api/src/bidirectional_protocol/msg.rs

+    legacy_protocol::msg::{FlatTree, Message, PanicMessage, ServerConfig},
+};
+
+pub type RequestId = u32;


4 billion requests are a lot but still in the realm of possibility for this to overflow for a long-lived rust-analyzer process with a lot of macros, so I think better to have this as a u64.

Also, a newtype, maybe?

ChayimFriedman2 · 2025-12-21T06:40:09Z

crates/proc-macro-api/src/process.rs

 pub(crate) enum Protocol {
    LegacyJson { mode: SpanMode },
    LegacyPostcard { mode: SpanMode },
+    NewPostcard { mode: SpanMode },


Better to call this BidirectionalPostcard and ditto for NewJson, so we won't have to rename again when we replace it (and also it's consistent with the module names).

Let's call it BidirectionalPostcardPrototype (and similar for the cli flags). I expect us to break this a bit here and there so i want us to differentiate that.

In part because i will likely have another look at the proto definitions in general and restructure them

ChayimFriedman2 · 2025-12-21T06:42:27Z

crates/proc-macro-api/src/process.rs

        match self.protocol {
            Protocol::LegacyJson { mode } => mode == SpanMode::RustAnalyzer,
            Protocol::LegacyPostcard { mode } => mode == SpanMode::RustAnalyzer,
+            Protocol::NewJson { mode } => mode == SpanMode::RustAnalyzer,


@Veykril will the Jetbrains people want to use the new protocol (but not rust-analyzer span mode)? If not, it makes sense to remove the token id span+new protocols combination completely, unless it's too much work.

I haven't talked to them yet regarding this, but for now I think we should only do the postcard one for simplicity for now.

ChayimFriedman2 · 2025-12-21T06:44:44Z

crates/proc-macro-api/Cargo.toml

 tracing.workspace = true
 rustc-hash.workspace = true
 indexmap.workspace = true
+base-db.workspace = true


Generally we really do not want to bring in salsa and base-db and friends here, but given that we're going to get rid of this completely after this RP (#t-compiler/rust-analyzer > Non-generic spans), I suppose it's fine?

ChayimFriedman2 · 2025-12-21T06:59:17Z

crates/proc-macro-srv/src/lib.rs

    }
 }

+pub enum SubRequest {


Is there a reason you need to redefine this enum here, instead of just reuse it?

ChayimFriedman2 · 2025-12-21T07:02:55Z

crates/proc-macro-srv-cli/src/main_loop.rs

+    });
+
+    let (subreq_tx, subreq_rx) = unbounded::<proc_macro_srv::SubRequest>();
+    let (subresp_tx, subresp_rx) = unbounded::<proc_macro_srv::SubResponse>();


Generally it's better for channels to be bounded, this way a fast one side a slow other side can flood memory with requests.

ChayimFriedman2 · 2025-12-21T07:05:55Z

crates/proc-macro-srv/src/lib.rs

        result
    }

+    pub fn expand_with_channels<S: ProcMacroSrvSpan>(


There's some code reuse potential with expand() here, perhaps take a callback.

ChayimFriedman2 · 2025-12-21T07:06:44Z

crates/proc-macro-srv/src/dylib/proc_macros.rs

+        Err(bridge::PanicMessage::String(format!("proc-macro `{macro_name}` is missing")).into())
+    }
+
+    pub(crate) fn expand_with_channels<S: ProcMacroSrvSpan>(


Make this take Option<Cahnnel>s and make expand() call it, to reuse code.

ChayimFriedman2 · 2025-12-21T07:07:30Z

.github/workflows/ci.yaml


-      - name: Check salsa dependency
-        run: "! (cargo tree -p proc-macro-srv-cli | grep -q salsa)"
-


Better to comment this only temporarily with a FIXME until we do #t-compiler/rust-analyzer > Non-generic spans.

Veykril · 2025-12-21T13:33:37Z

crates/proc-macro-srv/src/lib.rs

+        cli_to_server: crossbeam_channel::Receiver<SubResponse>,
+        server_to_cli: crossbeam_channel::Sender<SubRequest>,


I would expect us to model this via a callback instead impl FnMut(SubRequest) -> Result<SubResponse>, modelling this with channels seems weird given us sending a request has to be blocking anyways

My understanding is that it's structured this way so that it's easier to make it concurrent in the future.

That's fine, a single proc-macro execution will still run sequential so a callback works out there. The cli-server should manage the connection of the channels here, the proc-macro server impl should merely be able to say, I want to emit this subrequest, give me the corresponding reply: That's best modeled by a callback.

Veykril · 2025-12-21T13:36:03Z

crates/proc-macro-api/src/process.rs

        match self.protocol {
            Protocol::LegacyJson { mode } => mode == SpanMode::RustAnalyzer,
            Protocol::LegacyPostcard { mode } => mode == SpanMode::RustAnalyzer,
+            Protocol::NewJson { mode } => mode == SpanMode::RustAnalyzer,


I haven't talked to them yet regarding this, but for now I think we should only do the postcard one for simplicity for now.

Veykril · 2025-12-21T13:38:01Z

crates/proc-macro-api/src/process.rs

                    version: 0,
                    protocol: protocol.clone(),
                    exited: OnceLock::new(),
+                    next_request_id: AtomicU32::new(1),


I don't think we need this for now? This PR should only allow blocking bidirectionality, no concurrency so we don't run into the risk of having multiple requests outstanding. If we do, havent checked the server-cli loop code yet then we should move that out of this PR and only focus on bidirectionality for now

Veykril · 2025-12-21T13:44:23Z

crates/proc-macro-api/src/bidirectional_protocol.rs

+    struct Callbacks<'de> {
+        db: &'de dyn SourceDatabase,
+    }
+    impl<'db> ClientCallbacks for Callbacks<'db> {
+        fn handle_sub_request(&mut self, req: SubRequest) -> Result<SubResponse, ServerError> {
+            match req {
+                SubRequest::SourceText { file_id, start, end } => {
+                    let file = FileId::from_raw(file_id);
+                    let text = self.db.file_text(file).text(self.db);
+
+                    let slice = text.get(start as usize..end as usize).map(|s| s.to_owned());
+
+                    Ok(SubResponse::SourceTextResult { text: slice })
+                }
+            }
+        }
+    }


This to me feels like the wrong place to put this. I think the handler implementations need to happen in the client side crate, that is rust-analyzer. That will also allow us to remove the unwanted dependency on base-db in here. This code shouldn't need to know about databases and such.

Shourya742 force-pushed the 2025-11-27-bidirectional-protocol branch 4 times, most recently from 0a41ccb to bb61e2e Compare December 11, 2025 17:12

Add bidirectional messaging proc-macro-srv

376bca9

Shourya742 force-pushed the 2025-11-27-bidirectional-protocol branch from bb61e2e to 376bca9 Compare December 15, 2025 14:00

Shourya742 added 4 commits December 15, 2025 20:02

remove salsa dependency check on proc_macro_srv_cli

80705fb

make json-new as default protocol choice for proc-macro-srv-cli

178d6ce

add request id to requests

28a3b80

refactor the main loop in proc_macro-srv-cli

57fdf52

Shourya742 force-pushed the 2025-11-27-bidirectional-protocol branch from 339c074 to 57fdf52 Compare December 18, 2025 08:36

Shourya742 added 2 commits December 18, 2025 14:25

make imports proper

58f2e17

change request id to u32 to make powerpc happy

19e816d

Shourya742 force-pushed the 2025-11-27-bidirectional-protocol branch from a01d06d to 19e816d Compare December 18, 2025 09:17

Shourya742 marked this pull request as ready for review December 18, 2025 09:34

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2025

ChayimFriedman2 requested changes Dec 21, 2025

View reviewed changes

Veykril reviewed Dec 21, 2025

View reviewed changes


		- name: Check salsa dependency
		run: "! (cargo tree -p proc-macro-srv-cli \| grep -q salsa)"

		cli_to_server: crossbeam_channel::Receiver<SubResponse>,
		server_to_cli: crossbeam_channel::Sender<SubRequest>,

Add bidirectional messaging proc-macro-srv #21249

Are you sure you want to change the base?

Add bidirectional messaging proc-macro-srv #21249

Uh oh!

Conversation

Shourya742 commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChayimFriedman2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Shourya742 commented Dec 11, 2025 •

edited

Loading