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

Diagnostics doesn't get removed #132

Open
ncannasse opened this issue Jun 22, 2017 · 22 comments
Open

Diagnostics doesn't get removed #132

ncannasse opened this issue Jun 22, 2017 · 22 comments

Comments

@ncannasse
Copy link
Member

If I delete (or rename?) a file, the diagnostics errors are kept in the window and doesn't get removed.

@Gama11 Gama11 added the bug label Jun 24, 2017
@Gama11
Copy link
Member

Gama11 commented Jun 27, 2017

Hm... this is actually a bit problematic. We can deal with it for files in the workspace by using VSCode's file watcher API (which we already use for other things).

However, sometimes we also display diagnostics for files outside of the current workspace (Haxelibs, Classpaths...), and the file watcher API is limited to the current workspace. Need to watch for progress on microsoft/vscode#3025.

Gama11 added a commit to vshaxe/haxe-language-server that referenced this issue Jun 27, 2017
@Gama11
Copy link
Member

Gama11 commented Jun 27, 2017

Ok, I've dealt with the in-workspace case, leaving this open as a reminder for the rest.

@ncannasse
Copy link
Member Author

The diagnostics don't also get removed when compiling until you switch to the file again.
So if in A.hx you have an error related to B.hx, and you modify B.hx and recompile, the diagnostic should leave. Actually ideally you would know that changing B might need to reevaluate A's diagnostics.

@Gama11
Copy link
Member

Gama11 commented Jun 27, 2017

The language server doesn't have any control over problems generated from problem matchers / running tasks (those aren't technically "diagnostics" btw).

@ncannasse
Copy link
Member Author

Wow, that seems very problematic that errors keep showing even after they have been dealt with. But I'm not sure I understand : the compiler can actually add "problems", is it not possible to clear the list when compilation starts ?

@ncannasse
Copy link
Member Author

One additional option would be to redo diagnostics for all files having reported problems each time you change one other file

@Gama11
Copy link
Member

Gama11 commented Jun 27, 2017

That seems extremely expensive... You'd probably just need to have Nape as a dependency (unused import diagnostics in every file :D), enable diagnostics for Haxelibs and run the "global diagnostics check" command (checks all classpaths for diagnostics) once to make vshaxe unusable.

@ncannasse
Copy link
Member Author

I correct then: having reported "errors" (no warnings)

@Gama11
Copy link
Member

Gama11 commented Jun 27, 2017

I'm not sure that's much better, you might have errors in a lot of files if a method that's used in a lot of places has been renamed, or similar.. even if it's just a few, that would generate quite a few additional display requests (diagnostics are updated on save, and some people have auto-save enabled, so it really needs to be fast).

@ncannasse
Copy link
Member Author

ncannasse commented Jun 28, 2017

I have a different approach: to me, it is fine if you eventually miss some diagnostics - if the file has never been opened for instance, or in other cases were you might not have a real up to date view. But it is absolutely not good to have diagnostics that are no longer relevant, because you might want to fix these, click on it, look at the code, only to figure out later it was a false positive.

I think given the nature of Haxe it's quite often that you might have this situation, for instance you have a too many arguments diagnostics, but what you meant is to add a new optional argument to the method (in another file), etc.

So while I'm not sure what's the best solution here to get rid of false positive, either clearing the diagnostics upon compilation or rebuilding all diagnostics for all files that have errors when another file change (we could batch them in a single haxe compiler service call) seems both good solution, maybe there are other I have overlooked (detecting depending on the error if the diagnostic is related to another file seems quite hard).

@Simn what's your take on this?

@Gama11
Copy link
Member

Gama11 commented Jun 28, 2017

clearing the diagnostics upon compilation

We can't do that, vshaxe / the language server has no idea that a task is being run by the user - I don't think the extension API has a callback for that. Even if there was one, it would be pretty hard to detect that it's a task that does compilation (haxe might be called through Lime or other build tools).

@ncannasse
Copy link
Member Author

Looks like the tasks limitations are overwhelming. Any way to avoid them entirely and have our own way to run compilation by using haxe settings + setting a shortcut on a command? (this way we would do much more)

@Gama11
Copy link
Member

Gama11 commented Jun 28, 2017

I mean, theoretically yes, but that doesn't seem like a good approach to take. It'd mean the user experience is quite different for using Haxe in VSCode compared other languages. VSCode is also making the Tasks system more prominent - in the current Insider's build, there's now a new top-level task menu:

If you're a Haxe user new to VSCode, you'd expect "Run Build Task" to just work for compilation, not vshaxe rolling its own build command hidden somewhere in the command palette...

The upcoming API for generating Tasks is also alleviating one of the major issues I've had with them. It'll allow us to have out-of-the-box support for build tools like Lime, like FD has (currently setup for that is a bit painful), with a target picker etc.

@ncannasse
Copy link
Member Author

I see, I guess we have a deal with it for now the best we can. Would Tasks generation allows us to do things such as manipulating diagnostics before/after compilation ? A base of the problem seems to be to have both being entirely separated while errors should be shared between the two.

@nadako
Copy link
Member

nadako commented Jun 28, 2017

Let's not introduce any custom build systems here. If anything, this should be handled on language-server level. We could maybe look into having custom build script run by Tasks that communicates with the language server and requests diagnostics cleanup and whatnot.

@Gama11
Copy link
Member

Gama11 commented Jun 28, 2017

It seems the VSCode guys are at least aware of this issue, this describes the problem we have with problems being generated from both tasks and diagnostics:

microsoft/vscode#6973

@Gama11
Copy link
Member

Gama11 commented Jun 28, 2017

To summarize (because it's gotten a bit confusing), there are at least three different issues here:

  1. The language server doesn't clear diagnostics for deleted files outside of the workspace (VSCode File Watcher API limitation -> API: Allow to use the file watcher for aribitrary folders microsoft/vscode#3025).

  2. Entries in the problems panel have two sources:

    • the Haxe problem matcher, updated when running a task
    • the language server, updated when saving a file

    Because the former are only updated on running tasks, it can happen that an error that has already been fixed is still sticking around. This is confusing to the user, because he's used to problems updating as he types (-> Improve diagnostic story for build and reconcile diagnostics microsoft/vscode#6973).

  3. The language server only requests diagnostics for the current file. Say you have errors in File B caused by something in File A, and fix them, the error will stick around until you switch back to File B. This one is fully the language server's fault / VSCode can't help us here.

@Gama11
Copy link
Member

Gama11 commented Jul 8, 2017

TypeScript has an interesting approach to this - I believe they only apply their problem matcher to closed documents, and use diagnostics for the ones that are opened:

https://github.com/Microsoft/vscode/blob/1.13.1/extensions/typescript/package.json#L466

That avoids the issue of duplicated error messages in the problems panel. They also remove diagnostics when a file is closed (which doesn't play well with our "Run Global Diagnostics Check"...).

@Gama11
Copy link
Member

Gama11 commented Mar 26, 2018

There's a proposed VSCode API with callbacks for task / start end:

https://github.com/Microsoft/vscode/blob/33aeed33c17d0db73c363f5424eb6b5e1215d547/src/vs/vscode.proposed.d.ts#L717

We could probably use that to clear diagnostics on compilation in the future.

Gama11 added a commit to vshaxe/haxe-language-server that referenced this issue Apr 8, 2018
Gama11 added a commit that referenced this issue Apr 8, 2018
@Gama11
Copy link
Member

Gama11 commented Apr 8, 2018

I've added [tasks] and [diagnostics] labels to different kinds of problems, which should at least help clarify the situation a bit for now:

[diagnostics] problems are updated on file save, [tasks] problems only on recompilation.

@ncannasse
Copy link
Member Author

Is it still not possible to clear the [tasks] Problems when task is launched ? Currently Shift+Ctrl+B seems very unresponsive since it just changes a very small label in the bottom bar.

@Simn
Copy link
Member

Simn commented Apr 26, 2018

This bothers me as well, but I think it's a general problem. The OCaml extension is even worse in that regard because it doesn't indicate active builds at all.

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

No branches or pull requests

4 participants