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

Synchronize code_lens request & provide better UX for forc-fmt <> sway-lsp interaction. #5094

Merged
merged 17 commits into from Sep 15, 2023

Conversation

JoshuaBatty
Copy link
Member

@JoshuaBatty JoshuaBatty commented Sep 6, 2023

Description

This PR adds 3 things:

  1. Adds a benchmark for the format LSP request.

  2. Calls session.wait_for_parsing(); before computing the code_lens request. This fixes the original issue reported in Conflicts between language server and forc-fmt #4893 where run buttons where being placed incorrectly after formatting.

  3. If a file open in a code editor contains unsaved changes we write a lock file to .forc/lsp_locks/. This file is removed when the file is saved and there are no pending changes. If forc-fmt is run in a terminal we check if the path has a lock file associated with it. If unsaved changes are detected we bail from formatting with an error message instructing the user to save changes before continuing. See video below.

Screen.Recording.2023-09-13.at.4.34.34.pm.mov

closes #4893

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@JoshuaBatty JoshuaBatty self-assigned this Sep 6, 2023
@JoshuaBatty JoshuaBatty requested a review from a team September 6, 2023 05:18
@anton-trunov
Copy link
Contributor

What if the user changes their files on disk using something different from forc-fmt, e.g. sed? I understand that it's less likely, though.

I believe, if possible, it would be nice to have forc-fmt and sway-lsp not aware of each other.

@JoshuaBatty
Copy link
Member Author

I believe, if possible, it would be nice to have forc-fmt and sway-lsp not aware of each other.

Yeah I agree this would be ideal. I'm going to put this PR in draft mode while I ponder on this a bit more. Thanks Anton

@JoshuaBatty JoshuaBatty marked this pull request as draft September 6, 2023 12:24
@tritao
Copy link
Contributor

tritao commented Sep 6, 2023

I believe, if possible, it would be nice to have forc-fmt and sway-lsp not aware of each other.

Not sure what's the best approach here, but just as another data point, Cargo and rust-analyzer also seem to have some kind of integration, as if cargo is running from the VS Code LSP session, then cargo from the command line will wait until it's finished before starting.

@anton-trunov
Copy link
Contributor

Ah, that's interesting, didn't know about that integration. I'd guess they use some kind of file system lock. @tritao can you share some more details how they do it?

@tritao
Copy link
Contributor

tritao commented Sep 6, 2023

Ah, that's interesting, didn't know about that integration. I'd guess they use some kind of file system lock. @tritao can you share some more details how they do it?

I don't know how they do it, but I would assume a filesystem lock too.

@tritao
Copy link
Contributor

tritao commented Sep 7, 2023

Ah, that's interesting, didn't know about that integration. I'd guess they use some kind of file system lock. @tritao can you share some more details how they do it?

This is the message I see: Blocking waiting for file lock on build directory

@sdankel
Copy link
Member

sdankel commented Sep 8, 2023

We should get thing 1 and thing 2 in regardless!

This is the message I see: Blocking waiting for file lock on build directory

Taking a lock on the build directory is clever because it links it to the specific sway project, doesn't prevent editing the file in other editors at the same time, and should only ever be touched by forc.

Both forc-fmt and forc-client (forc build) would also need to wait for a lock on the build directory.

We could have LSP take a lock on the build directory on format_text, but I'm concerned it would block other operations for a long time because sometimes it's very slow. What results did you get from benchmarking? Is get_page_text_edit the reason it's sometimes slow?

@JoshuaBatty
Copy link
Member Author

What results did you get from benchmarking? Is get_page_text_edit the reason it's sometimes slow?

It's actually pretty fast. Averaging ~98ms to complete. I think the slowness is coming from the client <> server interaction.

@JoshuaBatty
Copy link
Member Author

Thanks for the tips everyone. I'm going to look into a nice solution this week.

@JoshuaBatty JoshuaBatty force-pushed the josh/lsp_fmt branch 3 times, most recently from 4c95f5d to 607119f Compare September 14, 2023 01:51
@JoshuaBatty JoshuaBatty marked this pull request as ready for review September 14, 2023 01:52
@JoshuaBatty
Copy link
Member Author

Ok i'm pretty happy with this now.

Any unsaved changes for a file in LSP now creates a hashed lock file in ~/.forc/.lsp_locks/. When a file is saved or closed and changes are discarded then the lock file is removed. The formatter checks for the existence of this lock file before formatting. If one exists then the user is instructed to save the file before continuing. See the updated video in the description.

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just need to clean up eprintlns

sway-lsp/src/core/document.rs Outdated Show resolved Hide resolved
@JoshuaBatty JoshuaBatty force-pushed the josh/lsp_fmt branch 3 times, most recently from 049b8e2 to 45fb2ec Compare September 15, 2023 00:58
@JoshuaBatty JoshuaBatty requested review from sdankel and a team September 15, 2023 01:08
@JoshuaBatty JoshuaBatty enabled auto-merge (squash) September 15, 2023 02:57
@JoshuaBatty JoshuaBatty requested a review from a team September 15, 2023 06:09
@JoshuaBatty JoshuaBatty merged commit 44f46a0 into master Sep 15, 2023
32 checks passed
@JoshuaBatty JoshuaBatty deleted the josh/lsp_fmt branch September 15, 2023 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicts between language server and forc-fmt
5 participants