feat(analysis): add diagnostics syntax

[aspeddro]

Example:

syntax_error.res:

let = 1 + 1.0
let add = =2
lett a = 2

dune exec rescript-editor-analysis diagnosticSyntax ~/Desktop/learning-rescript/src/intro/syntax_error.res

[                                      
{
  "range: {"start": {"line": 1, "character": 4}, "end": {"line": 1, "character": 5}},
  "message": "I was expecting a name for this let-binding. Example: `let message = \"hello\"`",
  "severity": 1,
},
{
  "range: {"start": {"line": 2, "character": 9}, "end": {"line": 2, "character": 11}},
  "message": "This let-binding misses an expression",
  "severity": 1,
},
{
  "range: {"start": {"line": 3, "character": 4}, "end": {"line": 3, "character": 6}},
  "message": "consecutive statements on a line must be separated by ';' or a newline",
  "severity": 1,
}
]

[aspeddro]

I need more cases for the test

[cristianoc]

This feature is progressing nicely. Thanks a lot for looking into it.

[cristianoc]

There's the question of what to do with parser recovery, see if this is too noisy. Always possible to report only the first thing.
Something to evaluate after hooking this command to the editor in server.ts.

[aspeddro]

Can you elaborate a little more? It is related to this PR?

[cristianoc]

So the parser runs in recovery mode, and keeps on parsing after the first syntax error.
You can see this in your test where 3 things are reported for the same issue.
Now, one could just stop after the first report. Which would take care of the noise in this example.
However, it would be nice to report more than one (syntax) error at once, if possible, when these errors happen in different regions.
Since this PR exposes the syntax errors to users, it's part of this PR to evaluate how those errors look like in practice.
Then based on that, decide how to proceed.

[cristianoc]

It might just be OK the way it is now. But one would need to experiment once this command is hooked up to the extension.

[aspeddro]

Ok, I'll test when #453 is finished because @zth will update the vscode client.

[cristianoc]

Another related aspect, that also needs to wait, is about making sure there is no double reporting when the compiler is already reporting the syntax error.

[cristianoc]

E.g.:
1 introduce syntax error
2 compile
3 edit unrelated part of the file

Then you get a file that is unsaved, with syntax errors, but where the syntax error was reported already.

[zth]

@aspeddro awesome that you're having a look at this! The client upgrade will be merged soon, so this will shortly be unblocked.

[zth]

@aspeddro merged! You can go ahead an pull the latest master into this PR, and the client will be upgraded.

[aspeddro]

I'm implemented workspace diagnostic to catch syntax error for all files inside a project but dune returns an error:

dune exec rescript-editor-analysis workspaceSyntax ~/Desktop/learning-rescript/
Fatal error: exception Sys_error("Value too large for defined data type")

In this directory there is only one file with syntax error.

Is it some dune setting, any idea how to get around this error?

[cristianoc]

I think we want current file only.
So the command would receive the path to the current file, parse it, return the results.

[aspeddro]

I've made some advances to syntax diagnostics. May be related to #448. I added a second argument to the analysis, this allows capturing diagnostics when the document changes.

rescript-editor-analysis diagnosticSyntax eval "let = 1 + 1.0"

Is it a good feature to add to this PR?

Testing on vscode client (still need to fix some issues)

screencast-2022-06-17_21.05.01.mp4

[cristianoc]

Definitely we want the command to run on unsaved files, in fact that's the only case where the command is needed. Otherwise, the compiler already reports the syntax error.

The only thing is: one passes the path to a file containing the current contents of the window, not a string.
See what other commands do. E.g. autocomplete.

[aspeddro]

Another related aspect, that also needs to wait, is about making sure there is no double reporting when the compiler is already reporting the syntax error.

E.g.:
1 introduce syntax error
2 compile
3 edit unrelated part of the file

Then you get a file that is unsaved, with syntax errors, but where the syntax error was reported already.

The client side does not merge diagnostics, so there are no duplicates. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_publishDiagnostics

analisys bin is ready. I will open a PR to update server.ts

[cristianoc]

Another related aspect, that also needs to wait, is about making sure there is no double reporting when the compiler is already reporting the syntax error.

E.g.:
1 introduce syntax error
2 compile
3 edit unrelated part of the file

Then you get a file that is unsaved, with syntax errors, but where the syntax error was reported already.

The client side does not merge diagnostics, so there are no duplicates. https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_publishDiagnostics

analisys bin is ready. I will open a PR to update server.ts

As long as the diagnostics are identical, true there's no problem.
So the only thing is checking that they are in fact identical.

[aspeddro]

I did some tests, but the assumption is that the parsing of .compiler.log is correct. Can I send the change of server.ts here? It is small and is related to this PR.

[zth]

I did some tests, but the assumption is that the parsing of .compiler.log is correct. Can I send the change of server.ts here? It is small and is related to this PR.

Go for it!

[cristianoc]

Noticed this when testing this PR:
#463

Now I don't think this change could have caused it.
Perhaps this is an area never tested much, when editing at the end of a file.
Or it could be that the upgrade preceding this PR leads to different behaviour.
In any case, it seems that a completion command can be issued for a non-existing location (2 past the last cursor position) in the file currently being edited.
Sounds like it could be just an effect of commands called in an asynchronous way, where the current file might have changed wile the command was being issued.
Or it could be there is now something wrong with the sync logic for the current text.

@zth any thoughts about this?

[cristianoc]

In general, it seems the reporting logic is very fast, and I was wondering whether this is indeed what we want.
It's kind of impossible to type any real code without getting syntax errors while typing, at least that's been my experience up until that crash.

[zth]

I'm going to do some thorough testing on TS and Rust, but I'm pretty sure we want to debounce the syntax diagnostics, so that it's only reported once the user "rests"/stops typing. Will get back here with the results.

[zth]

Ok, this was a bit surprising. My impression is both Rust and TS has same behavior as we have currently in this PR - everything is reported immediately. However, with TS it's sometimes hard to know whether it's a debounce involved, or if it's just slow. Here are two GIFs showing both experiences:

TypeScript

Rust

As you can see, both of them push the errors as soon as they can.

Two things come to mind knowing this:

1. It surprises me I've never thought about this. I guess that means that continuously reporting syntax errors at least isn't that annoying.
2. Is this really the best experience? Maybe we should experiment with debouncing ourselves? The "risk" of debouncing is the feeling of the language server being slow, but I'm unsure if that's such a big problem.

What do you think @aspeddro @cristianoc ?

[aspeddro]

I also don't find it annoying, as I remember most LSP's I've used report error when typing. Maybe a forum topic to see what users think.

[zth]

Other than deciding on the experience itself, what's left before this can be considered complete?

[cristianoc]

I also don't find it annoying, as I remember most LSP's I've used report error when typing. Maybe a forum topic to see what users think.

Given that the experience is the same in other languages, I think that settles it in terms of preferences/opinions.
The only thing one could try is to add some debouncing and try to figure out what the effects are.
But that's not going to be easy. The "It surprises me I've never thought about this" comment means you can't just go around asking people what they think. One would need to observe what happens when they edit, which is much harder to do.
I'd suggest doing nothing unless keen to run an interesting experiment on the side. And even before doing that, one would need to read up on what others have tried before and how it went.

[zth]

@cristianoc yeah it sounds like our time can be better spent on something else than an experiment like that right now.

I'm going to run this locally for a while to try things out.

[zth]

Ok, after testing some I've noticed a few things:

1. We need to detect whether the file we're operating on is .res or .resi, and call the parser appropriately depending on what extension it is. It currently runs the parser in res mode on resi files, which reports a bunch of invalid syntax errors. Running the parser in interface mode when resi will fix that.
2. Unsure whether this actually has to do with this PR, and haven't looked deeper, but saving (formatting) will occasionally "eat" the most recent thing I wrote, kind of as if the formatter isn't working on the freshest file contents anymore. Need to dig further here, but a heads up.

[cristianoc]

Let's investigate 2.
It aligns with my experience of sending a non existing location to the binary command.
I think something is wrong in what's believed to be the current file in memory. So Eg the formatted is sent the wrong text.
Something to investigate.
I've experienced that while typing. So some async interaction is involved for sure.

[cristianoc]

This looks great.
Left a few comments.

[cristianoc]

Approving as there are only minor suggestions.

[cristianoc]

Looking great. Let me know when you're done with the minor things, and we can test and merge.

[aspeddro]

Looking great. Let me know when you're done with the minor things, and we can test and merge.

Done