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.

Sign up for GitHub

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

Jump to bottom

Run all analyzers even when a fixer isn't present #723

Merged
merged 8 commits into from Jul 6, 2020
Merged

Run all analyzers even when a fixer isn't present #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
57e8aa5
Copy NuGet packages to build directory.
JoeRobich Jul 4, 2020
42d4fee
Run NamingStyle analyzers
JoeRobich Jul 4, 2020
c4c3ca0
Fix variable name
JoeRobich Jul 4, 2020
c986271
Run all analyzers regardless of whether a fixer is present.
JoeRobich Jul 6, 2020
0db01ca
Run dotnet-format to fix code style warnings.
JoeRobich Jul 6, 2020
e89105c
Remove method used for analyzer testing
JoeRobich Jul 6, 2020
38fdc7d
Remove redundant tests
JoeRobich Jul 6, 2020
efd4767
Updated formatting based on comments
JoeRobich Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 33 additions & 0 deletions .editorconfig
View file
Open in desktop
Expand Up @@ -131,6 +131,7 @@ csharp_indent_switch_labels = true
csharp_indent_labels = flush_left

# Prefer "var" everywhere
dotnet_diagnostic.IDE0007.severity = error
csharp_style_var_for_built_in_types = true:error
csharp_style_var_when_type_is_apparent = true:error
csharp_style_var_elsewhere = true:error
Expand Down Expand Up @@ -178,3 +179,35 @@ csharp_space_between_parentheses = false
csharp_prefer_braces = true:silent
csharp_preserve_single_line_blocks = true
csharp_preserve_single_line_statements = true

# IDE0073: File header
dotnet_diagnostic.IDE0073.severity = warning
file_header_template = Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.
Copy link
Member Author

Choose a reason for hiding this comment

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

Added some of the goodness from Roslyn's .editorconfig


# IDE0035: Remove unreachable code
dotnet_diagnostic.IDE0035.severity = warning

# IDE0036: Order modifiers
dotnet_diagnostic.IDE0036.severity = warning

# IDE0040: Add accessibility modifiers
dotnet_diagnostic.IDE0040.severity = warning

# IDE0043: Format string contains invalid placeholder
dotnet_diagnostic.IDE0043.severity = warning

# IDE0044: Make field readonly
dotnet_diagnostic.IDE0044.severity = warning

# CONSIDER: Are IDE0051 and IDE0052 too noisy to be warnings for IDE editing scenarios? Should they be made build-only warnings?
# IDE0051: Remove unused private member
dotnet_diagnostic.IDE0051.severity = warning

# IDE0052: Remove unread private member
dotnet_diagnostic.IDE0052.severity = warning

# IDE0059: Unnecessary assignment to a value
dotnet_diagnostic.IDE0059.severity = warning

# IDE0060: Remove unused parameter
dotnet_diagnostic.IDE0060.severity = warning
11 changes: 6 additions & 5 deletions .vscode/launch.json
View file
Open in desktop
Expand Up @@ -41,15 +41,16 @@
"stopAtEntry": false
},
{
"name": "format format.sln --fix-style --check",
"name": "format format.sln --fix-style warn --check",
"type": "coreclr",
"request": "launch",
"preLaunchTask": "publish",
"preLaunchTask": "build",
// If you have changed target frameworks, make sure to update the program path.
"program": "${workspaceFolder}/artifacts/bin/dotnet-format/Debug/netcoreapp2.1/publish/dotnet-format.dll",
"program": "${workspaceFolder}/artifacts/bin/dotnet-format/Debug/netcoreapp2.1/dotnet-format.dll",
"args": [
"format.sln",
"--fix-style",
"warn",
"-v",
"diag",
"--check"
Expand All @@ -63,9 +64,9 @@
"name": "format format.sln --fix-analyzers warn --check",
"type": "coreclr",
"request": "launch",
"preLaunchTask": "publish",
"preLaunchTask": "build",
// If you have changed target frameworks, make sure to update the program path.
"program": "${workspaceFolder}/artifacts/bin/dotnet-format/Debug/netcoreapp2.1/publish/dotnet-format.dll",
"program": "${workspaceFolder}/artifacts/bin/dotnet-format/Debug/netcoreapp2.1/dotnet-format.dll",
"args": [
"format.sln",
"--fix-analyzers",
Expand Down
47 changes: 2 additions & 45 deletions src/Analyzers/AnalyzerFinderHelpers.cs
View file
Open in desktop
Expand Up @@ -4,8 +4,6 @@
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Diagnostics;
Expand All @@ -14,7 +12,7 @@ namespace Microsoft.CodeAnalysis.Tools.Analyzers
{
internal static class AnalyzerFinderHelpers
{
public static ImmutableArray<(DiagnosticAnalyzer Analyzer, CodeFixProvider? Fixer)> LoadAnalyzersAndFixers(IEnumerable<Assembly> assemblies)
public static (ImmutableArray<DiagnosticAnalyzer> Analyzers, ImmutableArray<CodeFixProvider> Fixers) LoadAnalyzersAndFixers(IEnumerable<Assembly> assemblies)
{
var types = assemblies
.SelectMany(assembly => assembly.GetTypes()
Expand All @@ -34,48 +32,7 @@ public static ImmutableArray<(DiagnosticAnalyzer Analyzer, CodeFixProvider? Fixe
.OfType<DiagnosticAnalyzer>()
.ToImmutableArray();

var builder = ImmutableArray.CreateBuilder<(DiagnosticAnalyzer Analyzer, CodeFixProvider? Fixer)>();
foreach (var diagnosticAnalyzer in diagnosticAnalyzers)
{
var diagnosticIds = diagnosticAnalyzer.SupportedDiagnostics.Select(diagnostic => diagnostic.Id).ToImmutableHashSet();
var codeFixProvider = codeFixProviders.FirstOrDefault(codeFixProvider => codeFixProvider.FixableDiagnosticIds.Any(id => diagnosticIds.Contains(id)));

if (codeFixProvider is null)
{
continue;
}

builder.Add((diagnosticAnalyzer, codeFixProvider));
}

return builder.ToImmutableArray();
}

public static async Task<ImmutableDictionary<Project, ImmutableArray<DiagnosticAnalyzer>>> FilterBySeverityAsync(
IEnumerable<Project> projects,
ImmutableArray<DiagnosticAnalyzer> allAnalyzers,
ImmutableHashSet<string> formattablePaths,
DiagnosticSeverity minimumSeverity,
CancellationToken cancellationToken)
{
var projectAnalyzers = ImmutableDictionary.CreateBuilder<Project, ImmutableArray<DiagnosticAnalyzer>>();
foreach (var project in projects)
{
var analyzers = ImmutableArray.CreateBuilder<DiagnosticAnalyzer>();

foreach (var analyzer in allAnalyzers)
{
var severity = await analyzer.GetSeverityAsync(project, formattablePaths, cancellationToken).ConfigureAwait(false);
if (severity >= minimumSeverity)
{
analyzers.Add(analyzer);
}
}

projectAnalyzers.Add(project, analyzers.ToImmutableArray());
}

return projectAnalyzers.ToImmutableDictionary();
return (diagnosticAnalyzers, codeFixProviders);
}
}
}
133 changes: 108 additions & 25 deletions src/Analyzers/AnalyzerFormatter.cs
View file
Open in desktop
Expand Up @@ -17,18 +17,18 @@ namespace Microsoft.CodeAnalysis.Tools.Analyzers
internal class AnalyzerFormatter : ICodeFormatter
{
private readonly string _name;
private readonly IAnalyzerFinder _finder;
private readonly IAnalyzerInformationProvider _informationProvider;
private readonly IAnalyzerRunner _runner;
private readonly ICodeFixApplier _applier;

public AnalyzerFormatter(
string name,
IAnalyzerFinder finder,
IAnalyzerInformationProvider informationProvider,
IAnalyzerRunner runner,
ICodeFixApplier applier)
{
_name = name;
_finder = finder;
_informationProvider = informationProvider;
_runner = runner;
_applier = applier;
}
Expand All @@ -41,11 +41,7 @@ internal class AnalyzerFormatter : ICodeFormatter
List<FormattedFile> formattedFiles,
CancellationToken cancellationToken)
{
var analyzersAndFixers = _finder.GetAnalyzersAndFixers(solution, formatOptions, logger);
if (analyzersAndFixers.Length == 0)
{
return solution;
}
var (analyzers, fixers) = _informationProvider.GetAnalyzersAndFixers(solution, formatOptions, logger);

var analysisStopwatch = Stopwatch.StartNew();
logger.LogTrace(Resources.Running_0_analysis, _name);
Expand All @@ -55,17 +51,21 @@ internal class AnalyzerFormatter : ICodeFormatter

logger.LogTrace(Resources.Determining_diagnostics);

var allAnalyzers = analyzersAndFixers.Select(pair => pair.Analyzer).ToImmutableArray();
var projectAnalyzers = await _finder.FilterBySeverityAsync(solution.Projects, allAnalyzers, formattablePaths, formatOptions, cancellationToken).ConfigureAwait(false);
var severity = _informationProvider.GetSeverity(formatOptions);

var projectDiagnostics = await GetProjectDiagnosticsAsync(solution, projectAnalyzers, formattablePaths, formatOptions, logger, formattedFiles, cancellationToken).ConfigureAwait(false);
// Filter to analyzers that report diagnostics with equal or greater severity.
var projectAnalyzers = await FilterBySeverityAsync(solution.Projects, analyzers, formattablePaths, severity, cancellationToken).ConfigureAwait(false);

// Determine which diagnostics are being reported for each project.
var projectDiagnostics = await GetProjectDiagnosticsAsync(solution, projectAnalyzers, formattablePaths, formatOptions, severity, logger, formattedFiles, cancellationToken).ConfigureAwait(false);

var projectDiagnosticsMS = analysisStopwatch.ElapsedMilliseconds;
logger.LogTrace(Resources.Complete_in_0_ms, projectDiagnosticsMS);

logger.LogTrace(Resources.Fixing_diagnostics);

solution = await FixDiagnosticsAsync(solution, analyzersAndFixers, projectDiagnostics, formattablePaths, logger, cancellationToken).ConfigureAwait(false);
// Run each analyzer individually and apply fixes if possible.
solution = await FixDiagnosticsAsync(solution, analyzers, fixers, projectDiagnostics, formattablePaths, severity, logger, cancellationToken).ConfigureAwait(false);

var fixDiagnosticsMS = analysisStopwatch.ElapsedMilliseconds - projectDiagnosticsMS;
logger.LogTrace(Resources.Complete_in_0_ms, fixDiagnosticsMS);
Expand All @@ -80,6 +80,7 @@ internal class AnalyzerFormatter : ICodeFormatter
ImmutableDictionary<Project, ImmutableArray<DiagnosticAnalyzer>> projectAnalyzers,
ImmutableHashSet<string> formattablePaths,
FormatOptions options,
DiagnosticSeverity severity,
ILogger logger,
List<FormattedFile> formattedFiles,
CancellationToken cancellationToken)
Expand All @@ -93,7 +94,8 @@ internal class AnalyzerFormatter : ICodeFormatter
continue;
}

await _runner.RunCodeAnalysisAsync(result, analyzers, project, formattablePaths, logger, cancellationToken).ConfigureAwait(false);
// Run all the filtered analyzers to determine which are reporting diagnostic.
await _runner.RunCodeAnalysisAsync(result, analyzers, project, formattablePaths, severity, logger, cancellationToken).ConfigureAwait(false);
}

LogDiagnosticLocations(solution, result.Diagnostics.SelectMany(kvp => kvp.Value), options.WorkspaceFilePath, options.ChangesAreErrors, logger, formattedFiles);
Expand All @@ -106,7 +108,7 @@ static void LogDiagnosticLocations(Solution solution, IEnumerable<Diagnostic> di

foreach (var diagnostic in diagnostics)
{
var message = diagnostic.GetMessage();
var message = $"{diagnostic.GetMessage()} ({diagnostic.Id})";
var filePath = diagnostic.Location.SourceTree?.FilePath;
var document = solution.GetDocument(diagnostic.Location.SourceTree);

Expand All @@ -130,40 +132,121 @@ static void LogDiagnosticLocations(Solution solution, IEnumerable<Diagnostic> di

private async Task<Solution> FixDiagnosticsAsync(
Solution solution,
ImmutableArray<(DiagnosticAnalyzer Analyzer, CodeFixProvider? Fixer)> analyzersAndFixers,
ImmutableArray<DiagnosticAnalyzer> allAnalyzers,
ImmutableArray<CodeFixProvider> allCodefixes,
ImmutableDictionary<ProjectId, ImmutableHashSet<string>> projectDiagnostics,
ImmutableHashSet<string> formattablePaths,
DiagnosticSeverity severity,
ILogger logger,
CancellationToken cancellationToken)
{
// we need to run each codefix iteratively so ensure that all diagnostics are found and fixed
foreach (var (analyzer, codefix) in analyzersAndFixers)
// Determine the reported diagnostic ids
var reportedDiagnostics = projectDiagnostics.SelectMany(kvp => kvp.Value).Distinct().ToImmutableArray();
if (reportedDiagnostics.IsEmpty)
{
return solution;
}

// Build maps between diagnostic id and the associated analyzers and codefixes
var analyzersById = CreateAnalyzerMap(reportedDiagnostics, allAnalyzers);
var fixersById = CreateFixerMap(reportedDiagnostics, allCodefixes);

// We need to run each codefix iteratively so ensure that all diagnostics are found and fixed.
foreach (var diagnosticId in reportedDiagnostics)
{
var analyzers = analyzersById[diagnosticId];
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should try to get the value from the dictionary? Isn't it expected that we would not find entries for this?

Copy link
Contributor

@jmarolf jmarolf Jul 6, 2020 •
edited

Choose a reason for hiding this comment

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

Ah I see, the ToImmutableArray on an empty enumerable will mean that we should always have an empty immutable array

var codefixes = fixersById[diagnosticId];

// If there is no codefix, there is no reason to run analysis again.
if (codefixes.IsEmpty)
{
logger.LogWarning(Resources.Unable_to_fix_0_No_associated_code_fix_found, diagnosticId);
continue;
}

var result = new CodeAnalysisResult();
foreach (var project in solution.Projects)
{
if (!projectDiagnostics.TryGetValue(project.Id, out var diagnosticIds) ||
!analyzer.SupportedDiagnostics.Any(diagnostic => diagnosticIds.Contains(diagnostic.Id)))
// Only run analysis on projects that had previously reported the diagnostic
if (!projectDiagnostics.TryGetValue(project.Id, out var diagnosticIds))
{
continue;
}

await _runner.RunCodeAnalysisAsync(result, analyzer, project, formattablePaths, logger, cancellationToken).ConfigureAwait(false);
await _runner.RunCodeAnalysisAsync(result, analyzers, project, formattablePaths, severity, logger, cancellationToken).ConfigureAwait(false);
}

var hasDiagnostics = result.Diagnostics.Any(kvp => kvp.Value.Count > 0);
if (hasDiagnostics && codefix != null)
if (hasDiagnostics)
{
solution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken).ConfigureAwait(false);
var changedSolution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken).ConfigureAwait(false);
if (changedSolution.GetChanges(solution).Any())
foreach (var codefix in codefixes)
{
solution = changedSolution;
var changedSolution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken).ConfigureAwait(false);
if (changedSolution.GetChanges(solution).Any())
{
solution = changedSolution;
break;
}
}
}
}

return solution;

static ImmutableDictionary<string, ImmutableArray<DiagnosticAnalyzer>> CreateAnalyzerMap(
ImmutableArray<string> diagnosticIds,
ImmutableArray<DiagnosticAnalyzer> analyzers)
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we use an inconsistent wrapping style. Which one do we prefer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this one because it doesn't change when things are renamed/refactored.

{
return diagnosticIds.ToImmutableDictionary(
id => id,
id => analyzers.Where(analyzer => analyzer.SupportedDiagnostics.Any(diagnostic => diagnostic.Id == id)).ToImmutableArray());
}

static ImmutableDictionary<string, ImmutableArray<CodeFixProvider>> CreateFixerMap(
ImmutableArray<string> diagnosticIds,
ImmutableArray<CodeFixProvider> fixers)
{
return diagnosticIds.ToImmutableDictionary(
id => id,
id => fixers.Where(fixer => fixer.FixableDiagnosticIds.Contains(id)).ToImmutableArray());
}
}

internal static async Task<ImmutableDictionary<Project, ImmutableArray<DiagnosticAnalyzer>>> FilterBySeverityAsync(
IEnumerable<Project> projects,
ImmutableArray<DiagnosticAnalyzer> allAnalyzers,
ImmutableHashSet<string> formattablePaths,
DiagnosticSeverity minimumSeverity,
CancellationToken cancellationToken)
{
// We only want to run analyzers for each project that have the potential for reporting a diagnostic with
// a severity equal to or greater than specified.
var projectAnalyzers = ImmutableDictionary.CreateBuilder<Project, ImmutableArray<DiagnosticAnalyzer>>();
foreach (var project in projects)
{
var analyzers = ImmutableArray.CreateBuilder<DiagnosticAnalyzer>();

foreach (var analyzer in allAnalyzers)
{
// Always run naming style analyzers because we cannot determine potential severity.
// The reported diagnostics will be filtered by severity when they are run.
if (analyzer.GetType().FullName.EndsWith("NamingStyleDiagnosticAnalyzer"))
{
analyzers.Add(analyzer);
continue;
}

var severity = await analyzer.GetSeverityAsync(project, formattablePaths, cancellationToken).ConfigureAwait(false);
if (severity >= minimumSeverity)
{
analyzers.Add(analyzer);
}
}

projectAnalyzers.Add(project, analyzers.ToImmutableArray());
}

return projectAnalyzers.ToImmutableDictionary();
}
}
}
19 changes: 10 additions & 9 deletions src/Analyzers/AnalyzerOptionExtensions.cs
View file
Open in desktop
@@ -1,6 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Linq;
Expand Down Expand Up @@ -44,14 +42,17 @@ private static string GetCategoryBasedDotnetAnalyzerDiagnosticSeverityKey(string
return false;
}

// If user has explicitly configured severity for this diagnostic ID, that should be respected and
// bulk configuration should not be applied.
// If user has explicitly configured severity for this diagnostic ID, that should be respected.
if (compilation.Options.SpecificDiagnosticOptions.TryGetValue(descriptor.Id, out severity))
{
return true;
}

// If user has explicitly configured severity for this diagnostic ID, that should be respected.
// For example, 'dotnet_diagnostic.CA1000.severity = error'
if (compilation.Options.SpecificDiagnosticOptions.ContainsKey(descriptor.Id) ||
tree.DiagnosticOptions.ContainsKey(descriptor.Id))
if (tree.DiagnosticOptions.TryGetValue(descriptor.Id, out severity))
Copy link
Member Author

Choose a reason for hiding this comment

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

We were not returning the severity from DiagnosticOptions causing some analyzers to not be run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is why we need a test for this :(

{
severity = default;
return false;
return true;
}

var analyzerConfigOptions = analyzerOptions.AnalyzerConfigOptionsProvider.GetOptions(tree);
Expand Down