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

(bug) Closed document ownership not transferred back to the filesystem #879

Open
bradymadden97 opened this issue Feb 28, 2024 · 5 comments

Comments

@bradymadden97
Copy link

bradymadden97 commented Feb 28, 2024

I am fairly certain this is a bug, but I would like to confirm my understanding. The expectation I have is not explicitly confirmed or denied in the LSP spec, which is why I'm not 100% sure it's a bug. Here's my expectation that this bug report relies upon:

If a server sends WorkspaceEdits for a file that is not currently open, the client can apply those changes to the file without notifying the server - i.e. not send didChange - and rely on the server to watch the filesystem for the changes.

^^ If you disagree with this statement, or I have a misunderstanding of the spec, definitely let me know. If you agree with this, read on for the bug report:

Scenario:

  • We have three files:

export.ts

export const a = 10;

import1.ts

import { a } from './export';

import2.ts

import { a } from './export';
  • We open both import1.ts and import2.ts on the client, and send their initial contents to the langauge server.
  • We then close import1.ts on the client, and send a didClose notification to the language server
  • We prepare to rename export.ts and send a workspace/willRename request to the language server. The language server responds with the following WorkspaceEdits:
{
  "jsonrpc":"2.0",
  "id":"411457457",
  "result":{
    "changes":{
      "file:///home/runner/lsp-runner/src/import1.ts":[
        {"range":{"start":{"line":0,"character":19},"end":{"line":0,"character":27}},"newText":"./export2"}
      ],
      "file:///home/runner/lsp-runner/src/import2.ts":[
        {"range":{"start":{"line":0,"character":19},"end":{"line":0,"character":27}},"newText":"./export2"}
      ]
    }
  }
}
  • We apply the changes to both files, so the new file contents look like this:
    import1.ts
import { a } from './export2';

import2.ts

import { a } from './export2';
  • Since only import2.ts is open on the client, we only send didChange and didSave notifications from the client to the server for import2.ts with the above change.
  • We perform the rename of export.ts --> export2.ts

export2.ts

export const a = 10;
  • Now, we prepare to rename export2.ts back to export.ts. We make a second workspace/willRenameFiles request to the language server. The langauge server responds with the following WorkspaceEdits:
{
  "jsonrpc":"2.0",
  "id":"1057243199",
  "result":{
    "changes":{
      "file:///home/runner/lsp-runner/src/import2.ts":[
        {"range":{"start":{"line":0,"character":19},"end":{"line":0,"character":28}},"newText":"./export"}
      ]
    }
  }
}
  • Notice, it only gave us edits for import2.ts (the file that is open on the client), not import1.ts (the file that was opened and subsequently closed).
  • You can find a runnable reproduction of the above scenario here https://replit.com/@bradyatreplit/lsp-runner#index.js

At this point it seems good enough to make a bug report, as the above WorkspaceEdit is not correct. However, I wanted to add a few more details that led me to believe the root cause specifically is surrounding closed document ownership.

Further investigation

  1. If you repeat the above scenario, but do not send didOpen and didClose events for import1.ts - i.e. the file was never opened on the client at all - you will get the correct WorkspaceEdit results for the second willRenameFiles request.
  2. If you continue the original scenario, including sending a third willRenameFiles from export.ts to export2.ts (for a second time), you will see, once again, import1.ts is included in the list of WorkspaceEdits. This, I assume, I because typescript-language-server still thinks it's open on the client and it's seen no changes since its original version.

Attempts to reproduce elsewhere:

  1. I downloaded my reproduction repl (above) and also ran it locally, and was able to reproduce the same results
  2. I set up a similar directory structure on VSCode, which uses tsserver directly, and was unable to reproduce the results - VSCode correctly updated the imports for the closed file on every rename. (One caveat, a tsconfig.json in the workspace root was required for things to work - but we also have one here)

Thanks for bearing with me through the long bug report, please let me know if you have any questions!

@rchl
Copy link
Member

rchl commented Feb 28, 2024

@bradymadden97
Copy link
Author

I'm a bit confused. I've read those parts of the spec, but I don't know if it explains the inconsistent behavior that occurs when import1.ts has been open - and then closed - on the client vs. when it never has been opened on the client at all.

When it's never been opened at all, the imports get updated correctly. However when it's been opened and then closed, suddenly things do not work.

Are you saying that prior to running any ApplyWorkspaceEdit, the client must send didOpen (and then send a didChange explicitly as well). I'm struggling to understand how that would work for situations where a file on the filesystem has been edited programmatically, through any other means.

@predragnikolic
Copy link
Contributor

predragnikolic commented Feb 28, 2024

Are you saying that prior to running any ApplyWorkspaceEdit, the client must send didOpen (and then send a didChange explicitly as well).

That is the way I understood the spec.

If a file is closed
and a WorkspaceEdit comes in for the file,
the client would first need to open that file (that should send a didOpen notification)
and then apply the edits (that should send a didChange notification).

@bradymadden97
Copy link
Author

bradymadden97 commented Feb 28, 2024

It feels like there is some distinction being made for files that have programmatic edits triggered by an ApplyWorkspaceEdit, versus any other file that was edited programmatically.

Here's an example:

  • I create a file in my editor import3.ts and save it with the contents:
import { a } from './export';
  • I then close it. I then open it in vi and change the contents to:
import { a } from './mangledimportpath.xyz'
  • Since there is no file open in vscode - there is not a didChange notification made to tsserver - it just has file watchers running.
  • I then rename export.ts to export2.ts
  • In vscode, tsserver sends a getEditsForFileRename and knows that import3.ts has been updated, no longer imports from export.ts, and thus, it doesn't include import3.ts in its list of file changes.

The only difference here between the above edit of a closed file, and this edit of a closed file, is that one was initiated by an ApplyWorkspaceEdit and one was not. So is the language server tracking the WorkspaceEdits it proposes to see if the client actually applies them or not?

@rchl
Copy link
Member

rchl commented Feb 28, 2024

Here's an example:

I've tried it here and it worked fine for me.

If you want me to have a look into it then please:

  • provide a repo that reproduces this simplified case
  • set initializationOptions.tsserver.logVerbosity to verbose and provide the tsserver log that is created in the .log directory after reproducing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants