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

Fadeout based on roslyn tag 'unnecessary'. #2873

Merged
merged 60 commits into from
Jun 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
582e745
Initial prototype.
savpek Feb 24, 2019
d09f936
Merge from master.
savpek Feb 24, 2019
45630c2
Tweaks.
savpek Feb 24, 2019
816c178
Updates and tweaks.
savpek Feb 24, 2019
5c3b0b2
Rethinked how diagnostics should be handled a bit.
savpek Feb 24, 2019
d8e3266
Fixes.
savpek Feb 24, 2019
d655061
Refactoring.
savpek Feb 25, 2019
e01ce48
Removed hard coded CS diagnostics.
savpek Feb 26, 2019
624190d
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Mar 2, 2019
a3e0d5f
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Mar 14, 2019
8093eab
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Mar 17, 2019
80ae3fc
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Mar 19, 2019
dda8586
Merge upstream/master.
savpek Mar 20, 2019
3f9e955
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Mar 22, 2019
d8526c4
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Mar 30, 2019
6f8e1bd
Trigger build.
savpek Apr 5, 2019
358abbe
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 8, 2019
eea2114
Renamed badly named variables.
savpek Apr 8, 2019
c2f32b8
Merge branch 'feature/fadeout-with-roslyn-diag' of https://github.com…
savpek Apr 8, 2019
b2b2da0
Added dummy diagnostic integration test.
savpek Apr 8, 2019
bbf41ab
Test that actually returns diagnostics.
savpek Apr 8, 2019
f5ade23
Updates for diagnostic tests.
savpek Apr 10, 2019
8422d3d
Updates for diagnostic.cs
savpek Apr 10, 2019
c8e8dd2
Added test that fadeout diagnostics are returned.
savpek Apr 10, 2019
7a24182
Implemented version without 'suppressHiddenDiagnostics' and returned …
savpek Apr 10, 2019
3630b1d
Moved omnisharp config before host init.
savpek Apr 10, 2019
19ffdbc
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 11, 2019
8925593
Revert "Implemented version without 'suppressHiddenDiagnostics' and r…
savpek Apr 11, 2019
8a1b06b
Hardcoded non analyzer ones.
savpek Apr 11, 2019
9d04cc7
supress now supresses hint instead of original information level.
savpek Apr 11, 2019
789acb0
Merge branch 'feature/fadeout-with-roslyn-diag' of https://github.com…
savpek Apr 11, 2019
7dab7f0
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 12, 2019
8e9a2af
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 13, 2019
9f46187
Testing to set enableRoslynAnalyzers as default.
savpek Apr 13, 2019
dd960c6
Merge branch 'feature/fadeout-with-roslyn-diag' of https://github.com…
savpek Apr 13, 2019
c78770a
Few small review fixes.
savpek Apr 13, 2019
56e2514
Refactoring based on review.
savpek Apr 13, 2019
faf036b
Added enableRoslynAnalyzers as test default.
savpek Apr 13, 2019
4b0164d
Testing build.
savpek Apr 13, 2019
bd08a84
Attempt to fix tests.
savpek Apr 13, 2019
4329a12
Disabled analyzers and linked issue.
savpek Apr 14, 2019
1aa2f4c
Rebuild and restored subtle change.
savpek Apr 14, 2019
dc55430
Fixed invalid skip method.
savpek Apr 14, 2019
eeba8d3
Removed excess dot.
savpek Apr 14, 2019
7360718
Refactored rename test to use same routines as other tests.
savpek Apr 14, 2019
a036453
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 16, 2019
6afd672
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 26, 2019
2e54378
Test build.
savpek Apr 27, 2019
ba94620
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Apr 28, 2019
f592a9f
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek May 5, 2019
3641770
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek May 7, 2019
e7285f5
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek May 18, 2019
7cd8334
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek May 22, 2019
778191e
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek May 25, 2019
5b59092
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek May 30, 2019
1e85ed4
Merge branch 'master' into feature/fadeout-with-roslyn-diag
akshita31 Jun 7, 2019
f5f293c
Merge branch 'master' into feature/fadeout-with-roslyn-diag
akshita31 Jun 7, 2019
0537553
Merge branch 'master' into feature/fadeout-with-roslyn-diag
savpek Jun 8, 2019
a9920ec
Rebuild.
savpek Jun 9, 2019
cfad848
Rebuild.
savpek Jun 9, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 55 additions & 26 deletions src/features/diagnosticsProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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);
}
}
Expand All @@ -319,36 +325,59 @@ class DiagnosticsProvider extends AbstractSupport {
});
}

private static _shouldInclude(quickFix: protocol.QuickFix): boolean {
const config = vscode.workspace.getConfiguration('csharp');
akshita31 marked this conversation as resolved.
Show resolved Hide resolved
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);
}
Expand Down
2 changes: 2 additions & 0 deletions src/omnisharp/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ export interface QuickFix {
EndColumn: number;
Text: string;
Projects: string[];
Tags: string[];
Id: string;
}

export interface SymbolLocation extends QuickFix {
Expand Down
9 changes: 8 additions & 1 deletion test/integrationTests/codeActionRename.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
60 changes: 60 additions & 0 deletions test/integrationTests/diagnostics.integration.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
38 changes: 21 additions & 17 deletions test/integrationTests/poll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
1 change: 1 addition & 0 deletions test/integrationTests/testAssets/singleCsproj/A.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
class C {}
12 changes: 12 additions & 0 deletions test/integrationTests/testAssets/singleCsproj/diagnostics.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System.IO;

namespace Foo
{
public class FooBar
{
public void FooBarBar()
{
var notUsed = 3;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"omnisharp.defaultLaunchSolution": "b_SecondInOrder_SlnFile.sln",
"omnisharp.path": "latest",
"csharp.maxProjectFileCountForDiagnosticAnalysis": 1000,
"csharp.maxProjectFileCountForDiagnosticAnalysis": 1000
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
class C {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System.IO;

namespace Foo
{
public class FooBar
{
public void FooBarBar()
{
var notUsed = 3;
}
}
}