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

[takeover] Change in one ts file does not reflect diagnostics in another file #3229

Closed
rchl opened this issue May 26, 2023 · 9 comments
Closed

Comments

@rchl
Copy link
Collaborator

rchl commented May 26, 2023

I'm not sure if this affects only takeover mode (ts/js files) or also Vue files but I would suspect that it does affect all.

  1. Create files:

file1.ts

export const x = 1;

file2.ts

import { x } from './file1';

export const y = x + 2;
  1. change 1 in the first file to 'str'.
  2. Switch to file 2

Expected: file2 shows an error because number and string can't be added.
Actual: file2 does not show any error until something is changed in the file.

It doesn't seem like Volar listens to the tsserver events (syntaxDiag, semanticDiag) that update diagnostics.

@lucasavila00
Copy link
Contributor

lucasavila00 commented May 27, 2023

I'm also able to reproduce it with another Volar-based LS, unrelated to Vue. It does not require takeover mode.

I haven't published its source yet, but the gist of the LS is: https://gist.github.com/lucasavila00/e7b8014c0d2aec6a8082f2f5b578578d Basically if one adds embedded TS documents to volar starter https://github.com/volarjs/starter this bug is reproduceable.

@johnsoncodehk
Copy link
Member

johnsoncodehk commented Jul 4, 2023

What is your language server diagnosticModel setting? For value 1 it's handle by language server push, but for value 2 it's handle by language client pull, so we can't control.

For diagnosticModel : 2, you should also make sure the LSP client supports interFileDependencies.

@johnsoncodehk
Copy link
Member

@lucasavila00 volarjs/starter used diagnosticModel : 2 for example purposes, changing to diagnosticModel: 1 or remove the option may fix the problem.

@rchl
Copy link
Collaborator Author

rchl commented Jul 4, 2023

It was actually set to diagnosticModel: 2 when I've tested but it also reproduces with 1.

@rchl
Copy link
Collaborator Author

rchl commented Jul 4, 2023

And BTW. According to the LSP spec the pull diagnostics also support notifying about changes in related documents which means that the server can notify about updated diagnostics even in the not-currently-active document.

(whether it would be possible to handle with typescript or the current volar architecture, I can't tell)

@rchl
Copy link
Collaborator Author

rchl commented Jul 4, 2023

Also note: I'm testing in ST which does support relatedDocuments but since it also reproduces with push diagnostics, I don't think that matters much.

@johnsoncodehk
Copy link
Member

I think it's expected that x + 2; doesn't report an error, since string+number is valid code.

You can change it to x.toPrecision() + 2; and try again.

@johnsoncodehk
Copy link
Member

And BTW. According to the LSP spec the pull diagnostics also support notifying about changes in related documents which means that the server can notify about updated diagnostics even in the not-currently-active document.

According to the description of LSP refresh diagnostics it doesn't seem to be for individual files, I think the language client needs to handle the refresh itself with re-pull diagnostics.

@rchl
Copy link
Collaborator Author

rchl commented Jul 5, 2023

I think it's expected that x + 2; doesn't report an error, since string+number is valid code.

You can change it to x.toPrecision() + 2; and try again.

No. The error gets eventually reported, even in Volar. The issue is that the file needs to be explicitly modified (by adding a space or whatever other change) and then Volar correctly picks up the updated diagnostic.

According to the description of LSP refresh diagnostics it doesn't seem to be for individual files, I think the language client needs to handle the refresh itself with re-pull diagnostics.

Not sure what you mean.
Server can respond with DocumentDiagnosticReport that can include relatedDocuments array which tells the client in which files the diagnostics need to be pulled. So it's client's responsibility to pull still but now it knows which files to update.

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