Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Send requirement from the lsp #18079

Open
dsherret opened this issue Mar 8, 2023 · 3 comments
Open

Remove Send requirement from the lsp #18079

dsherret opened this issue Mar 8, 2023 · 3 comments
Labels
chore something that we should get around to eventually

Comments

@dsherret
Copy link
Member

dsherret commented Mar 8, 2023

This is all single threaded, but the lsp code complains if we switch to RefCell and Rc. I tried spawning a local task for this, but that didn't help. Probably some simple fix I don't know about.

// todo(dsherret): for some reason the lsp errors when using an Rc<RefCell<NodeId>> here
// instead of an Arc<Mutex<NodeId>>. We should investigate and fix.

cc @bartlomieju

@bartlomieju
Copy link
Member

This is caused by tower_lsp requiring futures to be Send:

https://docs.rs/tower-lsp/latest/tower_lsp/struct.Server.html#method.serve

It's a low priority, we can revisit it in the future

@bartlomieju bartlomieju added the chore something that we should get around to eventually label Mar 8, 2023
@dsherret
Copy link
Member Author

I opened ebkalderon/tower-lsp#386

@ebkalderon
Copy link

Just left a few comments on that ticket. I would be more than happy to drop the Send bound going forward! As explained in that thread, the required Send bounds have already been dropped in an open WIP branch, but this was done as part of a larger-scale effort that goes beyond the scope of ebkalderon/tower-lsp#386.

If you would find it valuable, I wouldn't mind drafting a new release of tower-lsp which only drops the Send bounds from futures, leaving the remainder of the proposed changes to a future version? The removal of Send bounds is technically orthogonal to everything else that was proposed, so I'd be okay with doing this. 😄

@dsherret dsherret changed the title Remove Mutex/Arc usage in cli/npm/resolution/graph.rs Remove Send requirement from the lsp Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore something that we should get around to eventually
Projects
None yet
Development

No branches or pull requests

3 participants