From 73ce6c1071cf1c5c00ee884ab31ace7860f1a48e Mon Sep 17 00:00:00 2001 From: Joey Robichaud Date: Thu, 1 Aug 2019 17:18:44 -0700 Subject: [PATCH] Code cleanup and refactoring --- src/Analyzers/AnalyzerFormatter.cs | 112 ++++++++++-------- src/Analyzers/AnalyzerRunner.cs | 18 +-- src/Analyzers/CodeAnalysisResult.cs | 17 +-- .../RoslynCodeStyleAnalyzerFinder.cs | 4 +- src/Analyzers/SolutionCodeFixApplier.cs | 2 +- tests/Extensions/ExportProviderExtensions.cs | 25 +++- tests/Formatters/AbstractFormatterTests.cs | 7 +- tests/dotnet-format.UnitTests.csproj | 10 ++ 8 files changed, 117 insertions(+), 78 deletions(-) diff --git a/src/Analyzers/AnalyzerFormatter.cs b/src/Analyzers/AnalyzerFormatter.cs index 1a0e787baa..8b1fb8d504 100644 --- a/src/Analyzers/AnalyzerFormatter.cs +++ b/src/Analyzers/AnalyzerFormatter.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics; using System.IO; using System.Linq; using System.Threading; @@ -40,46 +41,13 @@ internal class AnalyzerFormatter : ICodeFormatter var analysisStopwatch = Stopwatch.StartNew(); logger.LogTrace($"Analyzing code style."); - var paths = formattableDocuments.Select(id => solution.GetDocument(id)?.FilePath) - .OfType().ToImmutableArray(); - - var pairs = _finder.GetAnalyzersAndFixers(); - if (!options.SaveFormattedFiles) { - // no need to run codefixes as we won't persist the changes - var analyzers = pairs.Select(x => x.Analyzer).ToImmutableArray(); - var result = new CodeAnalysisResult(); - await solution.Projects.ForEachAsync(async (project, token) => - { - await _runner.RunCodeAnalysisAsync(result, analyzers, project, paths, logger, token); - }, cancellationToken); - - LogDiagnosticLocations(result.Diagnostics.SelectMany(kvp => kvp.Value), options.WorkspaceFilePath, options.ChangesAreErrors, logger); + await LogDiagnosticsAsync(solution, formattableDocuments, options, logger, cancellationToken); } else { - // we need to run each codefix iteratively so ensure that all diagnostics are found and fixed - foreach (var (analyzer, codefix) in pairs) - { - var result = new CodeAnalysisResult(); - await solution.Projects.ForEachAsync(async (project, token) => - { - await _runner.RunCodeAnalysisAsync(result, analyzer, project, paths, logger, token); - }, cancellationToken); - - var hasDiagnostics = result.Diagnostics.Any(kvp => kvp.Value.Length > 0); - if (hasDiagnostics && codefix is object) - { - logger.LogTrace($"Applying fixes for {codefix.GetType().Name}"); - solution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken); - var changedSolution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken); - if (changedSolution.GetChanges(solution).Any()) - { - solution = changedSolution; - } - } - } + solution = await FixDiagnosticsAsync(solution, formattableDocuments, logger, cancellationToken); } logger.LogTrace("Analysis complete in {0}ms.", analysisStopwatch.ElapsedMilliseconds); @@ -87,29 +55,79 @@ internal class AnalyzerFormatter : ICodeFormatter return solution; } - private void LogDiagnosticLocations(IEnumerable diagnostics, string workspacePath, bool changesAreErrors, ILogger logger) + private async Task LogDiagnosticsAsync(Solution solution, ImmutableArray formattableDocuments, FormatOptions options, ILogger logger, CancellationToken cancellationToken) { - var workspaceFolder = Path.GetDirectoryName(workspacePath); + var pairs = _finder.GetAnalyzersAndFixers(); + var paths = formattableDocuments.Select(id => solution.GetDocument(id)?.FilePath) + .OfType().ToImmutableArray(); - foreach (var diagnostic in diagnostics) + // no need to run codefixes as we won't persist the changes + var analyzers = pairs.Select(x => x.Analyzer).ToImmutableArray(); + var result = new CodeAnalysisResult(); + await solution.Projects.ForEachAsync(async (project, token) => { - var message = diagnostic.GetMessage(); - var filePath = diagnostic.Location.SourceTree?.FilePath; + await _runner.RunCodeAnalysisAsync(result, analyzers, project, paths, logger, token); + }, cancellationToken); + + LogDiagnosticLocations(result.Diagnostics.SelectMany(kvp => kvp.Value), options.WorkspaceFilePath, options.ChangesAreErrors, logger); - var mappedLineSpan = diagnostic.Location.GetMappedLineSpan(); - var changePosition = mappedLineSpan.StartLinePosition; + return; - var formatMessage = $"{Path.GetRelativePath(workspaceFolder, filePath)}({changePosition.Line + 1},{changePosition.Character + 1}): {message}"; + static void LogDiagnosticLocations(IEnumerable diagnostics, string workspacePath, bool changesAreErrors, ILogger logger) + { + var workspaceFolder = Path.GetDirectoryName(workspacePath); - if (changesAreErrors) + foreach (var diagnostic in diagnostics) { - logger.LogError(formatMessage); + var message = diagnostic.GetMessage(); + var filePath = diagnostic.Location.SourceTree?.FilePath; + + var mappedLineSpan = diagnostic.Location.GetMappedLineSpan(); + var changePosition = mappedLineSpan.StartLinePosition; + + var formatMessage = $"{Path.GetRelativePath(workspaceFolder, filePath)}({changePosition.Line + 1},{changePosition.Character + 1}): {message}"; + + if (changesAreErrors) + { + logger.LogError(formatMessage); + } + else + { + logger.LogWarning(formatMessage); + } } - else + } + } + + private async Task FixDiagnosticsAsync(Solution solution, ImmutableArray formattableDocuments, ILogger logger, CancellationToken cancellationToken) + { + var pairs = _finder.GetAnalyzersAndFixers(); + var paths = formattableDocuments.Select(id => solution.GetDocument(id)?.FilePath) + .OfType().ToImmutableArray(); + + // we need to run each codefix iteratively so ensure that all diagnostics are found and fixed + foreach (var (analyzer, codefix) in pairs) + { + var result = new CodeAnalysisResult(); + await solution.Projects.ForEachAsync(async (project, token) => { - logger.LogWarning(formatMessage); + await _runner.RunCodeAnalysisAsync(result, analyzer, project, paths, logger, token); + }, cancellationToken); + + var hasDiagnostics = result.Diagnostics.Any(kvp => kvp.Value.Count > 0); + if (hasDiagnostics && codefix is object) + { + logger.LogTrace($"Applying fixes for {codefix.GetType().Name}"); + solution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken); + var changedSolution = await _applier.ApplyCodeFixesAsync(solution, result, codefix, logger, cancellationToken); + if (changedSolution.GetChanges(solution).Any()) + { + solution = changedSolution; + } } } + + return solution; } } } diff --git a/src/Analyzers/AnalyzerRunner.cs b/src/Analyzers/AnalyzerRunner.cs index b0eac7fc90..c3f3230829 100644 --- a/src/Analyzers/AnalyzerRunner.cs +++ b/src/Analyzers/AnalyzerRunner.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license 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.Collections.Immutable; @@ -41,12 +41,16 @@ internal partial class AnalyzerRunner : IAnalyzerRunner cancellationToken); var diagnostics = await analyzerCompilation.GetAnalyzerDiagnosticsAsync(cancellationToken); // filter diagnostics - var filteredDiagnostics = diagnostics.Where( - x => !x.IsSuppressed && - x.Severity >= DiagnosticSeverity.Warning && - x.Location.IsInSource && - formattableDocumentPaths.Contains(x.Location.SourceTree?.FilePath, StringComparer.OrdinalIgnoreCase)); - result.AddDiagnostic(project, filteredDiagnostics); + foreach (var diagnostic in diagnostics) + { + if (!diagnostic.IsSuppressed && + diagnostic.Severity >= DiagnosticSeverity.Warning && + diagnostic.Location.IsInSource && + formattableDocumentPaths.Contains(diagnostic.Location.SourceTree?.FilePath, StringComparer.OrdinalIgnoreCase)) + { + result.AddDiagnostic(project, diagnostic); + } + } } } } diff --git a/src/Analyzers/CodeAnalysisResult.cs b/src/Analyzers/CodeAnalysisResult.cs index 0551ebb294..5090f5a649 100644 --- a/src/Analyzers/CodeAnalysisResult.cs +++ b/src/Analyzers/CodeAnalysisResult.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. using System.Collections.Generic; -using System.Collections.Immutable; -using System.Linq; using NonBlocking; @@ -13,23 +11,18 @@ internal class CodeAnalysisResult private readonly ConcurrentDictionary> _dictionary = new ConcurrentDictionary>(); - internal void AddDiagnostic(Project project, IEnumerable diagnostics) + internal void AddDiagnostic(Project project, Diagnostic diagnostic) { _ = _dictionary.AddOrUpdate(project, - addValueFactory: (key) => diagnostics.ToList(), + addValueFactory: (key) => new List() { diagnostic }, updateValueFactory: (key, list) => { - list.AddRange(diagnostics); + list.Add(diagnostic); return list; }); } - public IReadOnlyDictionary> Diagnostics - => new Dictionary>( - _dictionary.Select( - x => KeyValuePair.Create( - x.Key, - x.Value.ToImmutableArray()))); - + public IReadOnlyDictionary> Diagnostics + => _dictionary; } } diff --git a/src/Analyzers/RoslynCodeStyleAnalyzerFinder.cs b/src/Analyzers/RoslynCodeStyleAnalyzerFinder.cs index 4b030f3a16..2ebf30efc0 100644 --- a/src/Analyzers/RoslynCodeStyleAnalyzerFinder.cs +++ b/src/Analyzers/RoslynCodeStyleAnalyzerFinder.cs @@ -14,12 +14,13 @@ internal class RoslynCodeStyleAnalyzerFinder : IAnalyzerFinder private readonly static string s_executingPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); private readonly string _featuresCSharpPath = Path.Combine(s_executingPath, "Microsoft.CodeAnalysis.CSharp.Features.dll"); - private readonly string _featuresVisualBasicPath = Path.Combine(s_executingPath, "Microsoft.CodeAnalysis.CSharp.VisualBasic.dll"); + private readonly string _featuresVisualBasicPath = Path.Combine(s_executingPath, "Microsoft.CodeAnalysis.VisualBasic.Features.dll"); public ImmutableArray<(DiagnosticAnalyzer Analyzer, CodeFixProvider? Fixer)> GetAnalyzersAndFixers() { var analyzers = FindAllAnalyzers(); + // TODO: Match CodeFixes to the analyzers that produce the diagnostic ids they fix. return analyzers.Select(analyzer => (analyzer, (CodeFixProvider?)null)).ToImmutableArray(); } @@ -37,6 +38,7 @@ private ImmutableArray FindAllAnalyzers() private ImmutableArray FindAllCodeFixesAsync() { + // TODO: Discover CodeFixes return ImmutableArray.Empty; } diff --git a/src/Analyzers/SolutionCodeFixApplier.cs b/src/Analyzers/SolutionCodeFixApplier.cs index afdad9a042..55652d0685 100644 --- a/src/Analyzers/SolutionCodeFixApplier.cs +++ b/src/Analyzers/SolutionCodeFixApplier.cs @@ -51,7 +51,7 @@ internal class SolutionCodeFixApplier : ICodeFixApplier private class DiagnosticProvider : FixAllContext.DiagnosticProvider { private static readonly Task> EmptyDignosticResult = Task.FromResult(Enumerable.Empty()); - private readonly IReadOnlyDictionary> _diagnosticsByProject; + private readonly IReadOnlyDictionary> _diagnosticsByProject; internal DiagnosticProvider(CodeAnalysisResult analysisResult) { diff --git a/tests/Extensions/ExportProviderExtensions.cs b/tests/Extensions/ExportProviderExtensions.cs index a2ec704d2a..1158612168 100644 --- a/tests/Extensions/ExportProviderExtensions.cs +++ b/tests/Extensions/ExportProviderExtensions.cs @@ -29,7 +29,7 @@ public CompositionContextShim(ExportProvider exportProvider) public override bool TryGetExport(CompositionContract contract, out object export) { var importMany = contract.MetadataConstraints.Contains(new KeyValuePair("IsImportMany", true)); - var (contractType, metadataType) = GetContractType(contract.ContractType, importMany); + var (contractType, metadataType, isArray) = GetContractType(contract.ContractType, importMany); if (metadataType != null) { @@ -41,7 +41,7 @@ where method.GetParameters().Length == 1 && method.GetParameters()[0].ParameterT var parameterizedMethod = methodInfo.MakeGenericMethod(contractType, metadataType); export = parameterizedMethod.Invoke(_exportProvider, new[] { contract.ContractName }); } - else + else if (!isArray) { var methodInfo = (from method in _exportProvider.GetType().GetTypeInfo().GetMethods() where method.Name == nameof(ExportProvider.GetExports) @@ -51,12 +51,27 @@ where method.GetParameters().Length == 1 && method.GetParameters()[0].ParameterT var parameterizedMethod = methodInfo.MakeGenericMethod(contractType); export = parameterizedMethod.Invoke(_exportProvider, new[] { contract.ContractName }); } + else + { + var methodInfo = (from method in _exportProvider.GetType().GetTypeInfo().GetMethods() + where method.Name == nameof(ExportProvider.GetExportedValues) + where method.IsGenericMethod && method.GetGenericArguments().Length == 1 + where method.GetParameters().Length == 0 + select method).Single(); + var parameterizedMethod = methodInfo.MakeGenericMethod(contractType); + export = parameterizedMethod.Invoke(_exportProvider, null); + } return true; } - private (Type exportType, Type metadataType) GetContractType(Type contractType, bool importMany) + private (Type exportType, Type metadataType, bool isArray) GetContractType(Type contractType, bool importMany) { + if (importMany && contractType.BaseType == typeof(Array)) + { + return (contractType.GetElementType(), null, true); + } + if (importMany && contractType.IsConstructedGenericType) { if (contractType.GetGenericTypeDefinition() == typeof(IList<>) @@ -71,11 +86,11 @@ where method.GetParameters().Length == 1 && method.GetParameters()[0].ParameterT { if (contractType.GetGenericTypeDefinition() == typeof(Lazy<>)) { - return (contractType.GenericTypeArguments[0], null); + return (contractType.GenericTypeArguments[0], null, false); } else if (contractType.GetGenericTypeDefinition() == typeof(Lazy<,>)) { - return (contractType.GenericTypeArguments[0], contractType.GenericTypeArguments[1]); + return (contractType.GenericTypeArguments[0], contractType.GenericTypeArguments[1], false); } else { diff --git a/tests/Formatters/AbstractFormatterTests.cs b/tests/Formatters/AbstractFormatterTests.cs index ac5efd91fb..69b7d8255e 100644 --- a/tests/Formatters/AbstractFormatterTests.cs +++ b/tests/Formatters/AbstractFormatterTests.cs @@ -69,15 +69,12 @@ static AbstractFormatterTest() protected AbstractFormatterTest() { - TestState = new SolutionState(DefaultFolderPath + DefaultFilePathPrefix, DefaultFileExt); + TestState = new SolutionState(DefaultFilePathPrefix, DefaultFileExt); } /// /// Gets the language name used for the test. /// - /// - /// The language name used for the test. - /// public abstract string Language { get; } private static ILogger Logger => new TestLogger(); @@ -238,7 +235,7 @@ protected virtual Project CreateProjectImpl((string filename, SourceText content { (var newFileName, var source) = sources[i]; var documentId = DocumentId.CreateNewId(projectId, debugName: newFileName); - solution = solution.AddDocument(documentId, newFileName, source, filePath: newFileName); + solution = solution.AddDocument(documentId, newFileName, source, filePath: Path.Combine(DefaultTestProjectPath, newFileName)); } for (var i = 0; i < additionalFiles.Length; i++) diff --git a/tests/dotnet-format.UnitTests.csproj b/tests/dotnet-format.UnitTests.csproj index 75cf04bc3c..d002e8609c 100644 --- a/tests/dotnet-format.UnitTests.csproj +++ b/tests/dotnet-format.UnitTests.csproj @@ -17,6 +17,16 @@ + + + + + + + + + +