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

Dedupe Build and Live Diagnostics #6543

Merged
merged 17 commits into from
Nov 6, 2023

Conversation

beccamc
Copy link
Contributor

@beccamc beccamc commented Oct 12, 2023

Currently build and live diagnostics are reported independently, causing duplicate errors and errors that don't go away until a re-build. The root cause of this problem is that DevKit extension reports build diagnostics and C# extension reports live diagnostics.

image

This change creates a separate build diagnostics collection, that filters the diagnostics reported by the build and only shows the ones that are not being shown by live.

  1. On build complete, receive build diagnostics from DevKit extension via a brokered service
  2. Check if any of the files with build diagnostics have live diagnostics. If they have live diagnostics, display "build-only" diagnostics (e.g. errors we know that will not be caught by live). If they have no diagnostics, shown, they display all of the diagnostics reported by the build.
  3. When a file is opened, check to see if the build reported anything about it. If it did, display the build-only diagnostics. All other diagnostics should be shown by live.

dedupe

Issue: #5728

@beccamc beccamc requested a review from a team as a code owner October 12, 2023 19:58
@beccamc
Copy link
Contributor Author

beccamc commented Oct 12, 2023

If anyone has testing thoughts, please share! I'm sure there are corner cases I am not aware of.

@dibarbet
Copy link
Member

If anyone has testing thoughts, please share! I'm sure there are corner cases I am not aware of.

  1. Making sure behavior makes sense with FSA on/off (seems like it does from the description)
  2. Making sure the experience works if you turn FSA on or off (does that require a restart?)
  3. Is it possible to somehow indicate which errors are build only? Perhaps we could use the diagnostic source? To potentially alleviate confusion on why some errors go away later.

@beccamc
Copy link
Contributor Author

beccamc commented Oct 12, 2023

The source is set to "csharp-build"

image

src/lsptoolshost/roslynLanguageServer.ts Outdated Show resolved Hide resolved
src/lsptoolshost/roslynLanguageServer.ts Outdated Show resolved Hide resolved
src/lsptoolshost/roslynLanguageServer.ts Outdated Show resolved Hide resolved
@beccamc
Copy link
Contributor Author

beccamc commented Oct 27, 2023

The experience with FSA and very large solutions isn't very good. With this change, FSA solutions don't show build errors that would be duplicates. This means, as David pointed out, you could build the solution but not see errors until FSA finishes. Manish is gathering telemetry on FSA solutions so we can understand how common this is. For now, waiting for feedback and tracking in this issue - #6606

src/lsptoolshost/buildDiagnosticsService.ts Outdated Show resolved Hide resolved
src/lsptoolshost/buildDiagnosticsService.ts Outdated Show resolved Hide resolved
src/lsptoolshost/buildDiagnosticsService.ts Outdated Show resolved Hide resolved
src/lsptoolshost/buildDiagnosticsService.ts Outdated Show resolved Hide resolved
src/lsptoolshost/buildDiagnosticsService.ts Outdated Show resolved Hide resolved
src/lsptoolshost/services/buildResultReporterService.ts Outdated Show resolved Hide resolved
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this change is safe to merge even if the devkit change is not in?

src/lsptoolshost/buildDiagnosticsService.ts Show resolved Hide resolved
src/lsptoolshost/buildDiagnosticsService.ts Outdated Show resolved Hide resolved
@beccamc
Copy link
Contributor Author

beccamc commented Nov 6, 2023

Yes, this is safe without the DevKit change! CSharp extension will listen for any of the IBuildResultDiagnostics events, but it is not a problem that it doesn't receive any. On every file open it will check to see if there are build diagnostics in it's cache. There will be none and it'll skip doing any processing.

@beccamc beccamc merged commit 3802c7c into dotnet:main Nov 6, 2023
13 checks passed
Comment on lines +115 to +117
// Check the compiler diagnostic setting. If the setting is "none" or if the file is closed,
// then no live compiler diagnostics are being shown and bulid compiler diagnostics should be added.
// If FSA is on, then this is a no-op as FSA will report all compiler diagnostics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach sounds good to me as a starting point. I think we should dogfood the experience to confirm if the delay between build completion and background analysis reporting live diagnostics for open files (FSA off) or closed files (FSA on) is acceptable for real world solutions with large number of open files across the solution. Especially if there are compiler errors in the files which we are deferring to background analysis to report diagnostics. Otherwise, we may have to introduce some handshake between this diagnostic source and the existing live diagnostic source such that we report all build reported diagnostics for a file here until live background analysis has analyzed that file and reported live diagnostics for it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants