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

vscode.languages.getDiagnostics appears to return old diagnostics in some circumstances #54359

Closed
lostintangent opened this issue Jul 15, 2018 · 14 comments
Assignees
Labels
api bug Issue identified by VS Code Team member as probable bug languages-diagnostics Source problems reporting verified Verification succeeded
Milestone

Comments

@lostintangent
Copy link
Member

lostintangent commented Jul 15, 2018

A user reported an issue with VS Live Share, about diagnostics not being synchronized properly between participants in a session. After looking into it a bit, it appears like the issue may be with the vscode.languages.getDiagnostics method returning "stale" Diagnostics, depending on how the lifetime of the DiagnosticCollection is managed by extensions.

For example, take the following code (which is representative of what the Elm and Flow extensions are currently doing):

vscode.workspace.onDidSaveTextDocument((d) => {
    let errors = vscode.languages.createDiagnosticCollection("foo");
    errors.clear(); 
    errors.set(d.uri, [new vscode.Diagnostic(new vscode.Range(0, 0, 0, 0), "Failure")]);

    const diagnostics = vscode.languages.getDiagnostics(d.uri);
    console.log(diagnostics);
}));

If you debug that, and edit/save a document a couple times, you'll see the following in the Problems pane, which is the expected behavior:

screen shot 2018-07-15 at 3 58 55 pm

However, if you look in the Debug Console, you'll see the following printed, which is the output of the languages.getDiagnostics method:

screen shot 2018-07-15 at 3 59 06 pm

So for some reason, the Problems pane is displaying the expected diagnostics. However, the getDiagnostics method is returning stale diagnostics. Ultimately, this results in errors never going away, since the result of calling getDiagnostics becomes purely additive. This is problematic for Live Share, since we use the results of the getDiagnostics method to sync diagnostics with guests.

If you modify the above code, to create a single DiagnosticCollection for the lifetime of the extension (as opposed to re-creating it over and over again), then things behave as expected:

let errors = vscode.languages.createDiagnosticCollection("foo");
vscode.workspace.onDidSaveTextDocument((d) => {    
    errors.clear();
    errors.set(d.uri, [new vscode.Diagnostic(new vscode.Range(0, 0, 0, 0), "Failure")]);

    const diagnostics = vscode.languages.getDiagnostics(d.uri);
    console.log(diagnostics);
}));

Overall, the above code seems to make sense, and I submitted a PR to the Elm extension to refactor their code as such. However, it would be ideal if the vscode.languages.getDiagnostics method was resilient to this situation, and returned the same Diagnostics that are being displayed in the Problems pane.

@jrieken
Copy link
Member

jrieken commented Jul 16, 2018

For example, take the following code (which is representative of what the Elm and Flow extensions are currently doing):

So, if these extension always create a new collection, with disposing the old one it's likely that. The UI is likely deduping makers but the model won't. So, with the sample above, you are always adding new diagnostics but never removing them. Calling clear on the collection is local to that instance and those instances aren't shared because they share the name.

So, please check with your extension(s).

@jrieken jrieken added the info-needed Issue requires more information from poster label Jul 16, 2018
@jrieken
Copy link
Member

jrieken commented Jul 16, 2018

to create a single DiagnosticCollection for the lifetime of the extension (as opposed to re-creating it over and over again), then things behave as expected:

Sorry, only saw that now. Yeah, we don't dedupe in the model and I don't think it's the correct thing to do.

@jrieken jrieken added *dev-question VS Code Extension Development Question and removed info-needed Issue requires more information from poster labels Jul 16, 2018
@vscodebot
Copy link

vscodebot bot commented Jul 16, 2018

We have a great developer community over on slack where extension authors help each other. This is a great place for you to ask questions and find support.

Happy Coding!

@vscodebot vscodebot bot closed this as completed Jul 16, 2018
@lostintangent
Copy link
Member Author

lostintangent commented Jul 16, 2018

@jrieken Apologies. My original repro was probably too simple. If I modify the diagnostic message like the following:

let num = 0;

...

// Notice that I'm changing the error message each time to prevent de-duplication
errors.set(d.uri, [new vscode.Diagnostic(new vscode.Range(0, 0, 0, 0), `Failure${num++}`)]);

And save the document a few times, I see the following in the Problems pane:

screen shot 2018-07-16 at 9 00 04 am

And I see the following output from calling getDiagnostics(uri):

screen shot 2018-07-16 at 8 59 57 am

Are you saying that the Problems pane is de-duplicating diagnostics based on some kind of message pattern? In the above case, these diagnostics have different messages, and so I'm not sure why they would be removed from the UI if they're still in the model.

Even further, if I call the getDiagnostics() overload, instead of getDiagnostics(uri), then the old diagnostics are gone! So this appears to be a bug with the getDiagnostics(uri) method specifically. Is it possible that the Problems pane is consuming the getDiagnostics() method (workspace-wide retrieval vs. document-specific) or equivalent, and that's why it's immune to this issue?

@jrieken
Copy link
Member

jrieken commented Jul 16, 2018

Even further, if I call the getDiagnostics() method, instead of getDiagnostics(uri), then the old diagnostics are gone!

Sure about that? Both methods loop over the same collection of DiagnosticsCollections... Be aware of the different return types.

So this appears to be a bug with the getDiagnostics(uri) method specifically.

As long as you create more and more collections without disposing old ones, diagnostics pile up and the getDiagnostics-function returns them all. That's how it should be.

Is it possible that the Problems pane is consuming the getDiagnostics() method (workspace-wide retrieval vs. document-specific), and that's why it's immune to this issue?

Fair question. Taking another look and turns out that the name you provide when creating a collection is actually used as some kind of identifier. That makes the old markers disappear (be overwritten). I'd say that's actually a bug and I will make a code accordingly, e.g. when using a name that's already being used, we will bring a warning and use a different internal identifier.

@jrieken jrieken reopened this Jul 16, 2018
@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug api languages-diagnostics Source problems reporting and removed *dev-question VS Code Extension Development Question labels Jul 16, 2018
@lostintangent
Copy link
Member Author

lostintangent commented Jul 16, 2018

@jrieken Ah yeah, you're right. Both overloads of getDiagnostics have this same behavior. Apologies for saying otherwise!

@jrieken jrieken added this to the July 2018 milestone Jul 16, 2018
@jrieken jrieken reopened this Jul 18, 2018
@jrieken
Copy link
Member

jrieken commented Jul 18, 2018

I have now added a warning when a diagnostic collection with the same name is created (and it will use a different owner). Interestingly, TypeScript triggers this warning because for JS and TS the same name/owner is being used. According to @mjbvz this is because of tasks and problem matching. @dbaeumer can you provide insights.

@dbaeumer
Copy link
Member

Using a different owner for TypeScript and JavaScript works as long as users don't use a TSC task to check JS code. If they do the best we can do is to have different problem matchers with different IDs. I would suggest we simply make the change in the TS extension and then see if users get hit by tasks on pure JS projects (which I think is a very limited number of users in total).

@mjbvz mjbvz self-assigned this Jul 19, 2018
@jrieken
Copy link
Member

jrieken commented Jul 19, 2018

Ok. Alternatively, TypeScript can use one diagnostics collection (named typescript) for both languages

@jrieken jrieken removed their assignment Jul 19, 2018
@jrieken
Copy link
Member

jrieken commented Jul 19, 2018

I leave my change as it and leaving up to you two to fix this

@dbaeumer
Copy link
Member

+1 for @jrieken suggestion since it is the right fix to do since JS and TS will not overlap.

@mjbvz
Copy link
Contributor

mjbvz commented Jul 19, 2018

Here's the change that unified the diagnostics in the first place b945973 which was made to fix #48709. It was done for exactly the reason you detailed: using tsc to check javascript code.

Using a single collection will require work since we make some assumptions about having separate ones for JS and TS (it lets us handle the typescript.validate and javascript.validate settings more easily)

@mjbvz mjbvz closed this as completed in 9fb3229 Jul 20, 2018
@dbaeumer
Copy link
Member

@mjbvz thanks!

@octref octref added the verified Verification succeeded label Aug 2, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Sep 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api bug Issue identified by VS Code Team member as probable bug languages-diagnostics Source problems reporting verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants