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

Remove diagnostics from error list on document close or reset for tha… #39892

Merged
merged 8 commits into from Dec 3, 2019

Conversation

@mavasani
Copy link
Contributor

mavasani commented Nov 19, 2019

…t document

This fixes a recent regression where we stopped clearing out documents diagnostics on document close for TypeScript.

Fixes #39902


More Context

  1. Prior behavior (when closed file diagnostics/FSA is turned off):
    1. We only compute diagnostics for opened files and populate error list
    2. When user closes a file, we explicitly remove the diagnostics from the error list for that file.
  2. Recently, we added new functionality to re-purpose “Run Code Analysis” command for C# and VB projects, which regressed the above behavior.
    1. With this command, users can force compute live diagnostics for any project or current solution on demand. Computed diagnostics include both open file and closed file diagnostics and they are automatically populated in the error list.
    2. We want these diagnostics to stay in the error list even for closed files. I had to make a change to diagnostic service to not force remove all closed file diagnostics, otherwise this command was populating the correct diagnostics in error list only for them to be immediately removed for all closed files.
  3. New behavior:
    1. We only compute diagnostics for opened files and populate error list by default
    2. Users can force complete diagnostics for closed files by using explicit commands, at which point error list will also contain closed file diagnostics
    3. When user closes the file, we do not remove the diagnostics from the error list. User can switch the error list scope combo box to “Open documents” instead of the default “Entire solution” to only view diagnostics for opened documents.

We feel the new behavior is more logical – error list contents are purely what diagnostics have been computed and is not affected by actions such as closing a file. Users should be taught to use the error list scope combo box if they desire to get differing views over data, such as Current Document, Open Documents, and so on, instead of us trying to proactively do so for users by modifying the actual error list contents.

However, TypeScript would like to retain the old behavior. So we have decided to add a hook to allow a per-language setting to decide between the old and new behavior.

…t document

This fixes a recent regression where we stopped clearing out documents diagnostics on document close
@mavasani mavasani added the Area-IDE label Nov 19, 2019
@mavasani mavasani added this to the 16.5 milestone Nov 19, 2019
@mavasani mavasani requested review from genlu, sharwell, amcasey and dotnet/roslyn-analysis Nov 19, 2019
@mavasani mavasani requested a review from dotnet/roslyn-ide as a code owner Nov 19, 2019
@amcasey

This comment has been minimized.

Copy link
Member

amcasey commented Nov 19, 2019

Thanks! FYI @mrcrane

@mavasani mavasani closed this Nov 20, 2019
@mavasani mavasani reopened this Nov 20, 2019
@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Nov 20, 2019

FYI: I am working on an integration test for this behavior to prevent future regressions. Added with 479dbde

@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Nov 20, 2019

Filed #39902 to discuss #39892 (comment) in more detail at the IDE design meeting.

…a document. Verified that the test fails prior to the fix, and passes after the fix.
Copy link
Member

sharwell left a comment

The new behavior appears to be the correct behavior. I would need more information about specific failing scenarios before we knowingly revert this bug fix.

mavasani added 4 commits Nov 26, 2019
… and default this to false for other languages.
@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Nov 26, 2019

@sharwell - I have pushed the changes so the new behavior is retained for C# and VB, and old behavior is restored for TypeScript. I have temporarily hardcoded TypeScript for the old behavior, but that can be removed once they move to using the new option to enable the old behavior.

Tagging @jinujoseph

@mavasani mavasani requested a review from sharwell Nov 26, 2019
@mavasani mavasani dismissed sharwell’s stale review Nov 26, 2019

Stale review

@minestarks

This comment has been minimized.

Copy link

minestarks commented Nov 28, 2019

@mavasani I reran our tests after installing the VSIX from this CI, but they still fail with the same behavior. I haven't had a chance to dig into why, but will follow up after the holidays.

@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Nov 28, 2019

@minestarks I verified that by replacing the hard coded TypeScript string with C# ensures that closed file diagnostics for C# go away. I am wondering if the TS tests are failing due to some other reason. Would it be possible to manually validate your scenario?

@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Nov 28, 2019

@minestarks I manually validated the TS scenario (as per my understanding). I opened a new TS file, which creates a virtual TS project with bunch of library files. I opened one of those library files, then introduced syntax errors and closed the file. The errors remain in error list after closing the library file on latest dogfood build, but they are removed from error list after applying changes on this branch.

@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Dec 3, 2019

@minestarks Let me know if you were able to manually validate this.

@minestarks

This comment has been minimized.

Copy link

minestarks commented Dec 3, 2019

@mavasani thanks for following up. It's entirely possible there was an issue with the test set up. For what it's worth, the test opens an asp.net project and adds a .ts file to it (so not just a loose .ts file). I will run the steps manually once I have access to a machine again (tomorrow)

https://devdiv.visualstudio.com/DevDiv/_git/TypeScript-VS?path=%2FVS%2FLanguageService%2FTests%2FIntegration%2FClosedFileDiagnostics%2FClosedFileDiagnosticsTests.cs&version=GBmaster&line=13&lineEnd=15&lineStartColumn=1&lineEndColumn=113&lineStyle=plain

@minestarks

This comment has been minimized.

Copy link

minestarks commented Dec 3, 2019

@mavasani had a chance to try the test manually, it worked. :shipit: Hopefully it was something in my set up and the next Roslyn insertion will turn the integration test green as well. Thanks!

@mavasani

This comment has been minimized.

Copy link
Contributor Author

mavasani commented Dec 3, 2019

Thanks @minestarks!

@mavasani mavasani merged commit c239147 into dotnet:master Dec 3, 2019
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI #20191128.5 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI #20191128.5 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@mavasani mavasani deleted the mavasani:ClosedFileDiagnostics branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.