The flow-language-server responds with different error messages for changing files than it does for saving files

[w0rp]

Originally reported here: dense-analysis/ale#2000

If you send a textDocument/didSave event to the flow language server, you will get some diagnostic information revealing some type errors, like so:

{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///home/w0rp/Downloads/ale-flow/demo.js","diagnostics":[{"range":{"start":{"line":3,"character":13},"end":{"line":3,"character":24}},"severity":1,"code":"InferError","source":"Flow","message":"Cannot assign object literal to `a` because property `a` is missing in  object literal [1] but exists in  `A` [2].","relatedInformation":[{"location":{"uri":"file:///home/w0rp/Downloads/ale-flow/demo.js","range":{"start":{"line":3,"character":13},"end":{"line":3,"character":24}}},"message":"[1] object literal"},{"location":{"uri":"file:///home/w0rp/Downloads/ale-flow/demo.js","range":{"start":{"line":3,"character":9},"end":{"line":3,"character":10}}},"message":"[2] `A`"}],"relatedLocations":[{"location":{"uri":"file:///home/w0rp/Downloads/ale-flow/demo.js","range":{"start":{"line":3,"character":13},"end":{"line":3,"character":24}}},"message":"[1] object literal"},{"location":{"uri":"file:///home/w0rp/Downloads/ale-flow/demo.js","range":{"start":{"line":3,"character":9},"end":{"line":3,"character":10}}},"message":"[2] `A`"}]}]}}

Sending a textDocument/didChange event with the contents of the entire file will cause flow to respond with no errors, like so.

{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///home/w0rp/Downloads/ale-flow/demo.js","diagnostics":[]}}

You can repeat the bug with some LSP clients like ALE with the following project: https://github.com/joeybaker/ale-flow

This seems to be broken since 0.83.0. What flow should do instead is return the same error messages in both cases. If there are errors that can only be resolved after saving the file, those errors should be returned in response to the textDocument/didChange event until the next textDocument/didSave event is sent.

[TrySound]

Hi @jbrown215. Currently flow lsp is unusable because of this. Is this on flow team radar?

[jbrown215]

This is the first I'm hearing of it; thanks for bringing it to my attention.

I wonder if this is something that might be handled as part of @gabelevi's current work. I'll get back to you about this on Tuesday.

[jbrown215]

Also,

flow lsp is unusable because of this

I'm curious to know how this breaks LSP's usability. Is the issue that LSP is only returning errors on save instead of change? Do the errors get cleared between a didSave and didChange?

[TrySound]

Yes, after any change errors are cleared and even if not it's too wasteful to save after each change. Especially this is painful when refactoring.

[andreypopp]

@jbrown215 I'm testing 0.89 and what I see is that flow diagnostics are being sent only after didSave but not after didChange — therefore from UX point of view one needs to save file before seeing errors.

Note that this is only about type errors as syntax errors are reported by flow just fine on didChange.

[andreypopp]

Just to elaborate — I don't see the behaviour where errors are being cleared (using flow 0.89 and ALE).

[andreypopp]

Although more elaboration — if I do further changes then errors are being cleared so yeah — pretty sad LSP experience! Hope this can be fixed soon! Thank you for looking into this.

[jbrown215]

Great, thanks! I'm not very familiar with the LSP implementation and how it should work, so this extra information is very helpful.

[andreypopp]

Seems like this is the by-design behaviour for flow lsp —

flow/src/commands/lspCommand.ml

Lines 711 to 720 in 99e9433

	(* update cenv.c_diagnostics... we don't need to send updated squiggle locations *)
	(* right now ourselves, since all editors take care of that; but if ever we *)
	(* re-send the server's existing diagnostics for this file then that should take *)
	(* into account any user edits since then. This isn't perfect - e.g. if the user *)
	(* modifies a file we'll update squiggles, but if the user subsquently closes the *)
	(* file unsaved and then re-opens it then we'll be left with wrong squiggles. *)
	(* It also doesn't compensate if the flow server starts a typecheck, then receives *)
	(* a DidChange, then sends error spans from as it was at the start of the typecheck. *)
	(* Still, at least we're doing better on the common case -- where the server has sent *)
	(* diagnostics, then the user types, then we re-send live syntax errors. *)

[w0rp]

"It's a feature."

[jbrown215]

Definitely not a feature. Probably a limitation of the implementation at the time, but it's not clear to me if that limitation still exists. I will have a response for you tomorrow.

[gabelevi]

How I understand Flow's current behavior

I'm definitely not an expert in LSP and I've only used it via Atom. My understanding at the moment of LSP and Flow's behavior is thus:

• textDocument/didOpen is sent when the IDE opens a file. Flow uses incremental sync, so this is the only time we get sent the full file contents. Flow will keep a local copy of that file in memory. If this is a new file, Flow will send all the known errors.
• textDocument/didChange is sent when a file buffer changes in the IDE (e.g. when you're typing). This notification contains the file changes, and Flow will update it's local copy of the file. We don't respond to this notification.
• textDocument/didClose is sent when the IDE closes the file. Flow will then throw away its copy of the file.
• textDocument/didSave is a no-op. Flow watches the file system and will respond automatically to the file changing on disk, so Flow ignores this notification

The local copy of the file is used to respond to methods like textDocument/completion but isn't actually used for the regular typechecking.

So we don't really respond to textDocument/didChange. ad6688a added some behavior which tries to adjust squiggles on textDocument/didChange, but that's it.

How I understand this issue

As I understand it, this is what is being requested: As a user types in their IDE, the IDE will send textDocument/didChange notifications for the file being edited. You would like Flow to run as the file is edited and send back Flow errors. Flow waiting until the file is saved is not sufficient.

Or am I misunderstanding this issue? Is the problem that an incorrectly empty response is being sent after a textDocument/didChange? Like a file with an error starts getting empty textDocument/publishDiagnostics notifications while the user types?

[andreypopp]

As I understand it, this is what is being requested: As a user types in their IDE, the IDE will send textDocument/didChange notifications for the file being edited. You would like Flow to run as the file is edited and send back Flow errors. Flow waiting until the file is saved is not sufficient.

That's correct.

With vim/ALE the problem is very visible because the editor continues to show old locations of errors while changes are made to the sources and thus highlighting incorrect locations now.

[w0rp]

I concur. I think that understanding of the issue is correct.

[gabelevi]

With vim/ALE the problem is very visible because editors continues to show old locations of errors while changes are made to the sources and thus highlighting incorrect locations now.

Hmm, looking closer at ad6688a, I now see that it is updating the error locations but the comment says we're not sending the update errors. @ljw1004 - did you have a reason to avoid sending those updated errors to the IDE?

[ljw1004]

I coded this and can explain the design.

Short answer 1: If you change your editor to send incremental didChange events like Nuclide/VSCode then I bet you'd find it works satisfactorily. Or if you change the editor's initialization method to have initializationOptions.liveSyntaxErrors set to false then it'll be sort of okay.

Short answer 2: Flow doesn't have the ability to report typecheck errors for a single file at a time. The only way it can report typecheck errors for a file is by doing a whole-program typecheck, which is triggered by a filesystem change event.

(The painful reason is that sometimes you're typechecking file B, and the flow typechecker discovers that one type can't be unified with another, and it reports an error for file A where one of those types was defined. Flow's typechecker doesn't have the "locality" invariant that the entire set of typecheck errors for file A are discovered solely by typechecking file A.)

Let's walk through the example by @w0rp. First a file was saved. This triggered the editor to send a didSave event. It also separately caused a file-change on disk which was observed by Watchman and which triggered Flow to do a whole-program typecheck. As a result of this typecheck it reported the error in file A. We don't know whether that error came about from typechecking A, or B, or C. Let's suppose for argument's sake that it came from checking B.

Next a didChange event arrived with updated contents for the entire file A. Sure we could ask Flow to re-typecheck file A. But that wouldn't do any good. We'd have to somehow know to typecheck B. That's obviously not feasible (we can't expect Flow to do a global typecheck in response to every keystroke!)

So the design of FlowLSP is this:

1. When a filesystem change event happens, Flow does a whole-program typecheck, and reports all InferErrors.
2. As for syntax errors, they're a different kettle of fish because (1) it's usually important for a user to know immediately that they have a syntax error so they can avoid saving a file and triggering a whole-program recheck, (2) they do have the "locality" invariant...
3. In the LSP initialize method, Flow has the custom field initializationOptions.liveSyntaxErrors. It defaults to true. When true, flow will treat syntax errors differently. They will be updated upon didOpen and didChange events.

• Bcause Flow is sending an updated set of diagnostics during didChange, it therefore must update EVERY diagnostic for the file during didChange, including the typecheck errors (InferErrors). It's not able to recompute typecheck errors, as discussed. Instead all it does is, if you do a didChange event to insert or remove characters within that file, it will shift down or up or expand or contract those typecheck errors as appropriate. But if you do a didChange event to replace the span which contained the error, then the error gets removed as it has to.

4. If the editor specifically explicitly sets initializationOptions.liveSyntaxErrors to false, then (1) it won't report syntax errors as you type, (2) it doesn't need send updated errors in response to didChange. Therefore, it's left solely to the editor to shift errors up or down as you type, or shrink or expand or delete error squiggles.

If there are errors that can only be resolved after saving the file, those errors should be returned in response to the textDocument/didChange event

It does exactly that! My hunch is that you had a didChange which deleted the old span that contained the error squiggle, and then replaced it with a new load of text. Therefore, the error must be deleted until such time as the file is saved again. (You might imagine that Flow could observe that the new text is character-for-character identical to the old text and so figure out that the error squiggle could be restored. But you could more usefully imagine an editor sending only incremental didChange events...)

If your editor isn't yet set up to send incremental text edits, then turning off liveSyntaxErrors might be the cheapest option to get something working straight away.

[w0rp]

Sounds like Flow needs to be changed so it can support reporting on files that have changed without them being saved to disk, like TypeScript does.

[TrySound]

I have a good experience with regular flow server. I see errors as I edit not save. Why it works differently with lsp?

[ljw1004]

@w0rp I don't think TypeScript is comparable since (1) it doesn't do whole-program error reporting upon edits, (2) it was built from the ground up with the "locality" invariant, one that's quite hard to retrofit.

@TrySound What are you describing? Which flow server and which editor? I'm not aware of any ability for Flow to display errors on edit-rather-than-save other than via LSP.

[TrySound]

nvim + ale and non-lsp setup. I see it works via check-contents command. Is it possible to reuse it with lsp?
https://github.com/w0rp/ale/blob/master/ale_linters/javascript/flow.vim#L55

[ljw1004]

@TrySound That's an interesting idea. The check-contents approach would be guaranteed to fail to show certain errors (in particular, the ones reported in file A when encountered by checking file B). That might be good enough. There are two approaches:

(1) Users could simply live with their IDE failing to show all errors, because they periodically fall back to the command-line to see all errors.

(2) We could make flow lsp MERGE the results that come from check-contents with those that come from a whole program typecheck. Merging is tricky. Let's spell out how it's achieved in Visual Studio...

1. A whole-program typecheck will produce blue errors for the entire solution
2. While you're editing a file, it uses check-contents each keystroke to produce red errors for the current file
3. As you edit the document, it also shifts the blue errors around as appropriate for your insertions
4. If ever a red error arrives which (1) has the same span as an adjusted-span blue error, (2) has a heuristically similar enough text content to that blue error, then the blue error gets deleted.

This approach ends up delivering a nice user-experience in Visual Studio. It means that things are stateful, i.e. it knows which blue errors have so far been deleted due to similar red errors. It requires you to have a good idea for what heuristic to use - generally for you to have written your error messages with the foreknowledge that they will be compared in this way. If the first time red errors come about they're too much different from the existing blue errors then it's a poor user experience. For this reason it's important that red errors get computed promptly upon opening a file.

[w0rp]

Yes, I think that's the right solution, similar to something you might find in say, Eclipse with Java. Update the errors you can, keep the errors you can't prove are gone until a file has been saved again, and then update those errors.

[TrySound]

Hi @ljw1004 any news on this?

[TrySound]

Hey @mroch do you have any news about this issue?

[LoganBarnett]

@ljw1004 I really appreciate the breakdown you've given, and it does indeed seem tricky!

I have been switching my Emacs settings to using normal flow calls such as check-status over to a complete LSP setup, and this behavior came as quite a surprise. I thought it was a bug. That being said, I can also see strong value in global error reporting. Having my buffers sync errors automatically is great! I've been accustomed to this in other environments, but kind of lost the perspective with Flow+Emacs.

[SamChou19815]

Closing since this is by design