Fadeout based on roslyn tag 'unnecessary'. (#2873)

* Initial prototype.

* Tweaks.

* Updates and tweaks.

* Rethinked how diagnostics should be handled a bit.

* Fixes.

* Refactoring.

* Removed hard coded CS diagnostics.

* Trigger build.

* Renamed badly named variables.

* Added dummy diagnostic integration test.

* Test that actually returns diagnostics.

* Updates for diagnostic tests.

* Updates for diagnostic.cs

* Added test that fadeout diagnostics are returned.

* Implemented version without 'suppressHiddenDiagnostics' and returned hard codings for CSxxxx fadings.

* Moved omnisharp config before host init.

* Revert "Implemented version without 'suppressHiddenDiagnostics' and returned hard codings for CSxxxx fadings."

This reverts commit 7a24182.

* Hardcoded non analyzer ones.

* supress now supresses hint instead of original information level.

* Testing to set enableRoslynAnalyzers as default.

* Few small review fixes.

* Refactoring based on review.

* Added enableRoslynAnalyzers as test default.

* Testing build.

* Attempt to fix tests.

* Disabled analyzers and linked issue.

* Rebuild and restored subtle change.

* Fixed invalid skip method.

* Removed excess dot.

* Refactored rename test to use same routines as other tests.

* Test build.

* Rebuild.

* Rebuild.

src/features/diagnosticsProvider.ts

@@ -127,12 +127,14 @@ class DiagnosticsProvider extends AbstractSupport {
     private _documentValidations: { [uri: string]: vscode.CancellationTokenSource } = Object.create(null);
     private _projectValidation: vscode.CancellationTokenSource;
     private _diagnostics: vscode.DiagnosticCollection;
+    private _suppressHiddenDiagnostics: boolean;
 
     constructor(server: OmniSharpServer, validationAdvisor: Advisor) {
         super(server);
 
         this._validationAdvisor = validationAdvisor;
         this._diagnostics = vscode.languages.createDiagnosticCollection('csharp');
+        this._suppressHiddenDiagnostics = vscode.workspace.getConfiguration('csharp').get('suppressHiddenDiagnostics', true);
 
         let d1 = this._server.onPackageRestore(this._validateProject, this);
         let d2 = this._server.onProjectChange(this._validateProject, this);
@@ -236,7 +238,7 @@ class DiagnosticsProvider extends AbstractSupport {
         let handle = setTimeout(async () => {
             try {
                 let value = await serverUtils.codeCheck(this._server, { FileName: document.fileName }, source.token);
-                let quickFixes = value.QuickFixes.filter(DiagnosticsProvider._shouldInclude);
+                let quickFixes = value.QuickFixes;
                 // Easy case: If there are no diagnostics in the file, we can clear it quickly.
                 if (quickFixes.length === 0) {
                     if (this._diagnostics.has(document.uri)) {
@@ -247,8 +249,9 @@ class DiagnosticsProvider extends AbstractSupport {
                 }
 
                 // (re)set new diagnostics for this document
-                let diagnostics = quickFixes.map(DiagnosticsProvider._asDiagnostic);
-                this._diagnostics.set(document.uri, diagnostics);
+                let diagnosticsInFile = this._mapQuickFixesAsDiagnosticsInFile(quickFixes);
+
+                this._diagnostics.set(document.uri, diagnosticsInFile.map(x => x.diagnostic));
             }
             catch (error) {
                 return;
@@ -259,6 +262,12 @@ class DiagnosticsProvider extends AbstractSupport {
         this._documentValidations[key] = source;
     }
 
+    private _mapQuickFixesAsDiagnosticsInFile(quickFixes: protocol.QuickFix[]): { diagnostic: vscode.Diagnostic, fileName: string }[] {
+        return quickFixes
+            .map(quickFix => this._asDiagnosticInFileIfAny(quickFix))
+            .filter(diagnosticInFile => diagnosticInFile !== undefined);
+    }
+
     private _validateProject(): void {
         // If we've already started computing for this project, cancel that work.
         if (this._projectValidation) {
@@ -275,25 +284,22 @@ class DiagnosticsProvider extends AbstractSupport {
                 let value = await serverUtils.codeCheck(this._server, { FileName: null }, this._projectValidation.token);
 
                 let quickFixes = value.QuickFixes
-                    .filter(DiagnosticsProvider._shouldInclude)
                     .sort((a, b) => a.FileName.localeCompare(b.FileName));
 
                 let entries: [vscode.Uri, vscode.Diagnostic[]][] = [];
                 let lastEntry: [vscode.Uri, vscode.Diagnostic[]];
 
-                for (let quickFix of quickFixes) {
-
-                    let diag = DiagnosticsProvider._asDiagnostic(quickFix);
-                    let uri = vscode.Uri.file(quickFix.FileName);
+                for (let diagnosticInFile of this._mapQuickFixesAsDiagnosticsInFile(quickFixes)) {
+                    let uri = vscode.Uri.file(diagnosticInFile.fileName);
 
                     if (lastEntry && lastEntry[0].toString() === uri.toString()) {
-                        lastEntry[1].push(diag);
+                        lastEntry[1].push(diagnosticInFile.diagnostic);
                     } else {
                         // We're replacing all diagnostics in this file. Pushing an entry with undefined for
                         // the diagnostics first ensures that the previous diagnostics for this file are
                         // cleared. Otherwise, new entries will be merged with the old ones.
                         entries.push([uri, undefined]);
-                        lastEntry = [uri, [diag]];
+                        lastEntry = [uri, [diagnosticInFile.diagnostic]];
                         entries.push(lastEntry);
                     }
                 }
@@ -319,36 +325,59 @@ class DiagnosticsProvider extends AbstractSupport {
         });
     }
 
-    private static _shouldInclude(quickFix: protocol.QuickFix): boolean {
-        const config = vscode.workspace.getConfiguration('csharp');
-        if (config.get('suppressHiddenDiagnostics', true)) {
-            return quickFix.LogLevel.toLowerCase() !== 'hidden';
-        } else {
-            return true;
+    private _asDiagnosticInFileIfAny(quickFix: protocol.QuickFix): { diagnostic: vscode.Diagnostic, fileName: string } {
+        let display = this._getDiagnosticDisplay(quickFix, this._asDiagnosticSeverity(quickFix));
+
+        if (display.severity === "hidden") {
+            return undefined;
         }
+
+        let message = `${quickFix.Text} [${quickFix.Projects.map(n => this._asProjectLabel(n)).join(', ')}]`;
+
+        let diagnostic = new vscode.Diagnostic(toRange(quickFix), message, display.severity);
+
+        if (display.isFadeout) {
+            diagnostic.tags = [vscode.DiagnosticTag.Unnecessary];
+        }
+
+        return { diagnostic: diagnostic, fileName: quickFix.FileName };
     }
 
-    // --- data converter
+    private _getDiagnosticDisplay(quickFix: protocol.QuickFix, severity: vscode.DiagnosticSeverity | "hidden"): { severity: vscode.DiagnosticSeverity | "hidden", isFadeout: boolean }
+    {
+        // CS0162 & CS8019 => Unnused using and unreachable code.
+        // These hard coded values bring some goodnes of fading even when analyzers are disabled.
+        let isFadeout = (quickFix.Tags && !!quickFix.Tags.find(x => x.toLowerCase() == 'unnecessary')) || quickFix.Id == "CS0162" || quickFix.Id == "CS8019";
+
+        if (isFadeout && quickFix.LogLevel.toLowerCase() === 'hidden' || quickFix.LogLevel.toLowerCase() === 'none') {
+            // Theres no such thing as hidden severity in VSCode,
+            // however roslyn uses commonly analyzer with hidden to fade out things.
+            // Without this any of those doesn't fade anything in vscode.
+            return { severity: vscode.DiagnosticSeverity.Hint , isFadeout };
+        }
 
-    private static _asDiagnostic(quickFix: protocol.QuickFix): vscode.Diagnostic {
-        let severity = DiagnosticsProvider._asDiagnosticSeverity(quickFix.LogLevel);
-        let message = `${quickFix.Text} [${quickFix.Projects.map(n => DiagnosticsProvider._asProjectLabel(n)).join(', ')}]`;
-        return new vscode.Diagnostic(toRange(quickFix), message, severity);
+        return { severity: severity, isFadeout };
     }
 
-    private static _asDiagnosticSeverity(logLevel: string): vscode.DiagnosticSeverity {
-        switch (logLevel.toLowerCase()) {
+    private _asDiagnosticSeverity(quickFix: protocol.QuickFix): vscode.DiagnosticSeverity | "hidden" {
+        switch (quickFix.LogLevel.toLowerCase()) {
             case 'error':
                 return vscode.DiagnosticSeverity.Error;
             case 'warning':
                 return vscode.DiagnosticSeverity.Warning;
-            // info and hidden
-            default:
+            case 'info':
                 return vscode.DiagnosticSeverity.Information;
+            case 'hidden':
+                if (this._suppressHiddenDiagnostics) {
+                    return "hidden";
+                }
+                return vscode.DiagnosticSeverity.Hint;
+            default:
+                return "hidden";
         }
     }
 
-    private static _asProjectLabel(projectName: string): string {
+    private _asProjectLabel(projectName: string): string {
         const idx = projectName.indexOf('+');
         return projectName.substr(idx + 1);
     }

src/omnisharp/protocol.ts

@@ -201,6 +201,8 @@ export interface QuickFix {
     EndColumn: number;
     Text: string;
     Projects: string[];
+    Tags: string[];
+    Id: string;
 }
 
 export interface SymbolLocation extends QuickFix {

test/integrationTests/codeActionRename.integration.test.ts

@@ -8,24 +8,31 @@ import * as vscode from 'vscode';
 import { should, expect } from 'chai';
 import { activateCSharpExtension } from './integrationHelpers';
 import testAssetWorkspace from './testAssets/testAssetWorkspace';
+import * as path from 'path';
 
 const chai = require('chai');
 chai.use(require('chai-arrays'));
 chai.use(require('chai-fs'));
 
 suite(`Code Action Rename ${testAssetWorkspace.description}`, function () {
+    let fileUri: vscode.Uri;
+
     suiteSetup(async function () {
         should();
         await testAssetWorkspace.restore();
         await activateCSharpExtension();
+
+        let fileName = 'A.cs';
+        let projectDirectory = testAssetWorkspace.projects[0].projectDirectoryPath;
+        let filePath = path.join(projectDirectory, fileName);
+        fileUri = vscode.Uri.file(filePath)
     });
 
     suiteTeardown(async () => {
         await testAssetWorkspace.cleanupWorkspace();
     });
 
     test("Code actions can rename and open files", async () => {
-        let fileUri = await testAssetWorkspace.projects[0].addFileWithContents("test.cs", "class C {}");
         await vscode.commands.executeCommand("vscode.open", fileUri);
         let c = await vscode.commands.executeCommand("vscode.executeCodeActionProvider", fileUri, new vscode.Range(0, 7, 0, 7)) as { command: string, title: string, arguments: string[] }[];
         let command = c.find(

test/integrationTests/diagnostics.integration.test.ts

@@ -0,0 +1,60 @@
+/*--------------------------------------------------------------------------------------------- 
+*  Copyright (c) Microsoft Corporation. All rights reserved. 
+*  Licensed under the MIT License. See License.txt in the project root for license information. 
+*--------------------------------------------------------------------------------------------*/
+
+import * as vscode from 'vscode';
+import * as path from 'path';
+
+import { should, expect } from 'chai';
+import { activateCSharpExtension } from './integrationHelpers';
+import testAssetWorkspace from './testAssets/testAssetWorkspace';
+import poll from './poll';
+
+const chai = require('chai');
+chai.use(require('chai-arrays'));
+chai.use(require('chai-fs'));
+
+suite(`DiagnosticProvider: ${testAssetWorkspace.description}`, function () {
+    let fileUri: vscode.Uri;
+
+    suiteSetup(async function () {
+        should();
+
+        await testAssetWorkspace.restore();
+        await activateCSharpExtension();
+
+        let fileName = 'diagnostics.cs';
+        let projectDirectory = testAssetWorkspace.projects[0].projectDirectoryPath;
+        let filePath = path.join(projectDirectory, fileName);
+        fileUri = vscode.Uri.file(filePath);
+
+        await vscode.commands.executeCommand("vscode.open", fileUri);
+    });
+
+    suiteTeardown(async () => {
+        await testAssetWorkspace.cleanupWorkspace();
+    });
+
+    test("Returns any diagnostics from file", async function () {
+        let result = await poll(() => vscode.languages.getDiagnostics(fileUri), 10*1000, 500);
+        expect(result.length).to.be.greaterThan(0);
+    });
+
+    test("Return unnecessary tag in case of unnesessary using", async function () {
+        let result = await poll(() => vscode.languages.getDiagnostics(fileUri), 15*1000, 500);
+
+        let cs8019 = result.find(x => x.message.includes("CS8019"));
+        expect(cs8019).to.not.be.undefined;
+        expect(cs8019.tags).to.include(vscode.DiagnosticTag.Unnecessary);
+    });
+
+    test("Return fadeout diagnostics like unused usings based on roslyn analyzers", async function () {
+        this.skip(); // Remove this once https://github.com/OmniSharp/omnisharp-roslyn/issues/1458 is resolved.
+        let result = await poll(() => vscode.languages.getDiagnostics(fileUri), 15*1000, 500);
+
+        let ide0005 = result.find(x => x.message.includes("IDE0005"));
+        expect(ide0005).to.not.be(undefined);
+        expect(ide0005.tags).to.include(vscode.DiagnosticTag.Unnecessary);
+    });
+});

test/integrationTests/poll.ts

@@ -2,23 +2,27 @@
  *  Copyright (c) Microsoft Corporation. All rights reserved. 
  *  Licensed under the MIT License. See License.txt in the project root for license information. 
  *--------------------------------------------------------------------------------------------*/ 
- 
-export default async function poll<T>(getValue: () => T, duration: number, step: number): Promise<T> { 
-    while (duration > 0) { 
+
+export default async function poll<T>(getValue: () => T, duration: number, step: number): Promise<T> {
+    while (duration > 0) {
         let value = await getValue();
-
-        if (value) { 
-            return value; 
-        } 
-
-        await sleep(step); 
-
-        duration -= step; 
-    } 
-
-    throw new Error("Polling did not succeed within the alotted duration."); 
-} 
-
+
+        if(Array.isArray(value) && value.length > 0) {
+            return value;
+        }
+
+        if (!Array.isArray(value) && value) {
+            return value;
+        }
+
+        await sleep(step);
+
+        duration -= step;
+    }
+
+    throw new Error("Polling did not succeed within the alotted duration.");
+}
+
 async function sleep(ms = 0) {
-    return new Promise(r => setTimeout(r, ms)); 
+    return new Promise(r => setTimeout(r, ms));
 }

test/integrationTests/testAssets/singleCsproj/A.cs

@@ -0,0 +1 @@
+class C {}

test/integrationTests/testAssets/singleCsproj/diagnostics.cs

@@ -0,0 +1,12 @@
+using System.IO;
+
+namespace Foo
+{
+    public class FooBar
+    {
+        public void FooBarBar()
+        {
+            var notUsed = 3;
+        }
+    }
+}

test/integrationTests/testAssets/slnWithCsproj/.vscode/settings.json

@@ -1,5 +1,5 @@
 {
     "omnisharp.defaultLaunchSolution": "b_SecondInOrder_SlnFile.sln",
     "omnisharp.path": "latest",
-    "csharp.maxProjectFileCountForDiagnosticAnalysis": 1000,
+    "csharp.maxProjectFileCountForDiagnosticAnalysis": 1000
 }

test/integrationTests/testAssets/slnWithCsproj/src/app/A.cs

@@ -0,0 +1 @@
+class C {}

test/integrationTests/testAssets/slnWithCsproj/src/app/diagnostics.cs

@@ -0,0 +1,12 @@
+using System.IO;
+
+namespace Foo
+{
+    public class FooBar
+    {
+        public void FooBarBar()
+        {
+            var notUsed = 3;
+        }
+    }
+}