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

Move to Roslyn's editorconfig support #590

Merged
merged 8 commits into from May 28, 2020
Merged

Move to Roslyn's editorconfig support #590

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
1c03fa2
Move to Roslyn's .editorconfig support.
JoeRobich May 22, 2020
0bd4346
Fix integration tests
JoeRobich May 22, 2020
53d4713
Fix launch.json args
JoeRobich May 22, 2020
d8a92a1
Merge branch 'master' into roslyn-editorconfig
JoeRobich May 26, 2020
8161648
Use Project.AnalyzerOptions directly to get options.
JoeRobich May 27, 2020
ed246cc
Add link to issue tracking .NET NuGetSDKResolver bug
JoeRobich May 27, 2020
9d7b838
Add comment explaining how formattable files are determined
JoeRobich May 27, 2020
3c3c344
Use the full path as the filename for editorconfig files
JoeRobich May 27, 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
6 changes: 3 additions & 3 deletions .vscode/launch.json
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@
"version": "0.2.0",
"configurations": [
{
"name": "dotnet-format -f --check",
"name": "format -f --check",
"type": "coreclr",
"request": "launch",
"preLaunchTask": "build",
Expand All @@ -15,15 +15,15 @@
"-f",
"-v",
"diag",
"--check",
"--check"
],
"cwd": "${workspaceFolder}/src",
// For more information about the 'console' field, see https://aka.ms/VSCode-CS-LaunchJson-Console
"console": "internalConsole",
"stopAtEntry": false
},
{
"name": "dotnet-format format.sln --check",
"name": "format format.sln --check",
"type": "coreclr",
"request": "launch",
"preLaunchTask": "build",
Expand Down
1 change: 0 additions & 1 deletion NuGet.config
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@
<add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" />
<add key="roslyn" value="https://dotnet.myget.org/F/roslyn/api/v3/index.json" />
<add key="roslyn-analyzers" value="https://dotnet.myget.org/F/roslyn-analyzers/api/v3/index.json" />
<add key="vs-editor" value="https://myget.org/F/vs-editor/api/v3/index.json" />
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
</packageSources>
</configuration>
7 changes: 6 additions & 1 deletion eng/Versions.props
View file
Open in desktop
Expand Up @@ -24,10 +24,15 @@
<MicrosoftExtensionsDependencyInjectionVersion>$(MicrosoftExtensionsVersion)</MicrosoftExtensionsDependencyInjectionVersion>
<MicrosoftExtensionsFileSystemGlobbingVersion>$(MicrosoftExtensionsVersion)</MicrosoftExtensionsFileSystemGlobbingVersion>
<MicrosoftExtensionsLoggingVersion>$(MicrosoftExtensionsVersion)</MicrosoftExtensionsLoggingVersion>
<MicrosoftVisualStudioCodingConventionsVersion>1.1.20180503.2</MicrosoftVisualStudioCodingConventionsVersion>
<MicrosoftCodeAnalysisAnalyzersVersion>3.0.0</MicrosoftCodeAnalysisAnalyzersVersion>
<MicrosoftCodeAnalysisVersion>$(MicrosoftNETCoreCompilersPackageVersion)</MicrosoftCodeAnalysisVersion>
<SystemTextJsonVersion>4.7.0</SystemTextJsonVersion>

<!--
Workaround for the inability to load the NugetSdkResolver from the .NET 5 SDK
https://github.com/microsoft/MSBuildLocator/issues/88
-->
<NewtonsoftJsonVersion>12.0.2</NewtonsoftJsonVersion>
</PropertyGroup>
<PropertyGroup>
<DiscoverEditorConfigFiles>true</DiscoverEditorConfigFiles>
Expand Down
6 changes: 3 additions & 3 deletions eng/format-verifier.ps1
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@ try {

if ($stage -eq "prepare") {
Write-Output "$(Get-Date) - Cloning $repoName."
git.exe clone $repo $repoPath
git.exe clone $repo $repoPath -b master --single-branch --no-tags
}

Set-Location $repoPath
Expand Down Expand Up @@ -56,7 +56,7 @@ try {

if ($stage -eq "format-workspace") {
Write-Output "$(Get-Date) - $solutionFile - Formatting Workspace"
$output = dotnet.exe run -p "$currentLocation\src\dotnet-format.csproj" -c Release -- -w $solution -v d --dry-run | Out-String
$output = dotnet.exe run -p "$currentLocation\src\dotnet-format.csproj" -c Release -- $solution -v diag --check | Out-String
Write-Output $output.TrimEnd()

# Ignore CheckFailedExitCode since we don't expect these repos to be properly formatted.
Expand All @@ -77,7 +77,7 @@ try {

if ($stage -eq "format-folder") {
Write-Output "$(Get-Date) - $folderName - Formatting Folder"
$output = dotnet.exe run -p "$currentLocation\src\dotnet-format.csproj" -c Release -- -f $repoPath -v d --dry-run | Out-String
$output = dotnet.exe run -p "$currentLocation\src\dotnet-format.csproj" -c Release -- -f $repoPath -v diag --check | Out-String
Write-Output $output.TrimEnd()

# Ignore CheckFailedExitCode since we don't expect these repos to be properly formatted.
Expand Down
223 changes: 90 additions & 133 deletions src/CodeFormatter.cs
View file
Open in desktop
Expand Up @@ -11,13 +11,11 @@
using System.Threading.Tasks;
using Microsoft.Build.Logging;
using Microsoft.CodeAnalysis.MSBuild;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Tools.Formatters;
using Microsoft.CodeAnalysis.Tools.Utilities;
using Microsoft.CodeAnalysis.Tools.Workspaces;
using Microsoft.Extensions.FileSystemGlobbing;
using Microsoft.Extensions.Logging;
using Microsoft.VisualStudio.CodingConventions;

namespace Microsoft.CodeAnalysis.Tools
{
Expand Down Expand Up @@ -46,8 +44,10 @@ internal static class CodeFormatter

var workspaceStopwatch = Stopwatch.StartNew();

using var workspace = await OpenWorkspaceAsync(
workspaceFilePath, workspaceType, fileMatcher, logWorkspaceWarnings, logger, cancellationToken, createBinaryLog).ConfigureAwait(false);
using var workspace = workspaceType == WorkspaceType.Folder
? await OpenFolderWorkspaceAsync(workspaceFilePath, fileMatcher, cancellationToken).ConfigureAwait(false)
: await OpenMSBuildWorkspaceAsync(workspaceFilePath, workspaceType, createBinaryLog, logWorkspaceWarnings, logger, cancellationToken).ConfigureAwait(false);

if (workspace is null)
{
return new WorkspaceFormatResult(filesFormatted: 0, fileCount: 0, exitCode: 1);
Expand All @@ -61,7 +61,7 @@ internal static class CodeFormatter

logger.LogTrace(Resources.Determining_formattable_files);

var (fileCount, formatableFiles) = await DetermineFormattableFiles(
var (fileCount, formatableFiles) = await DetermineFormattableFilesAsync(
solution, projectPath, fileMatcher, includeGeneratedFiles, logger, cancellationToken).ConfigureAwait(false);

var determineFilesMS = workspaceStopwatch.ElapsedMilliseconds - loadWorkspaceMS;
Expand Down Expand Up @@ -144,32 +144,20 @@ private static string GetReportFilePath(string reportPath)
}
}

private static async Task<Workspace?> OpenWorkspaceAsync(
string workspacePath,
WorkspaceType workspaceType,
Matcher fileMatcher,
bool logWorkspaceWarnings,
ILogger logger,
CancellationToken cancellationToken,
bool createBinaryLog = false)
private static async Task<Workspace> OpenFolderWorkspaceAsync(string workspacePath, Matcher fileMatcher, CancellationToken cancellationToken)
{
if (workspaceType == WorkspaceType.Folder)
{
var folderWorkspace = FolderWorkspace.Create();
await folderWorkspace.OpenFolder(workspacePath, fileMatcher, cancellationToken);
return folderWorkspace;
}

return await OpenMSBuildWorkspaceAsync(workspacePath, workspaceType, logWorkspaceWarnings, logger, cancellationToken, createBinaryLog);
var folderWorkspace = FolderWorkspace.Create();
await folderWorkspace.OpenFolder(workspacePath, fileMatcher, cancellationToken).ConfigureAwait(false);
return folderWorkspace;
}

private static async Task<Workspace?> OpenMSBuildWorkspaceAsync(
string solutionOrProjectPath,
WorkspaceType workspaceType,
bool createBinaryLog,
bool logWorkspaceWarnings,
ILogger logger,
CancellationToken cancellationToken,
bool createBinaryLog = false)
CancellationToken cancellationToken)
{
var properties = new Dictionary<string, string>(StringComparer.Ordinal)
{
Expand Down Expand Up @@ -215,36 +203,36 @@ private static string GetReportFilePath(string reportPath)
LogWorkspaceDiagnostics(logger, logWorkspaceWarnings, workspace.Diagnostics);

return workspace;
}

private static void LogWorkspaceDiagnostics(ILogger logger, bool logWorkspaceWarnings, ImmutableList<WorkspaceDiagnostic> diagnostics)
{
if (!logWorkspaceWarnings)
static void LogWorkspaceDiagnostics(ILogger logger, bool logWorkspaceWarnings, ImmutableList<WorkspaceDiagnostic> diagnostics)
{
if (diagnostics.Count > 0)
if (!logWorkspaceWarnings)
{
logger.LogWarning(Resources.Warnings_were_encountered_while_loading_the_workspace_Set_the_verbosity_option_to_the_diagnostic_level_to_log_warnings);
}

return;
}
if (diagnostics.Count > 0)
{
logger.LogWarning(Resources.Warnings_were_encountered_while_loading_the_workspace_Set_the_verbosity_option_to_the_diagnostic_level_to_log_warnings);
}

foreach (var diagnostic in diagnostics)
{
if (diagnostic.Kind == WorkspaceDiagnosticKind.Failure)
{
logger.LogError(diagnostic.Message);
return;
}
else

foreach (var diagnostic in diagnostics)
{
logger.LogWarning(diagnostic.Message);
if (diagnostic.Kind == WorkspaceDiagnosticKind.Failure)
{
logger.LogError(diagnostic.Message);
}
else
{
logger.LogWarning(diagnostic.Message);
}
}
}
}

private static async Task<Solution> RunCodeFormattersAsync(
Solution solution,
ImmutableArray<(DocumentId, OptionSet, ICodingConventionsSnapshot)> formattableDocuments,
ImmutableArray<DocumentWithOptions> formattableDocuments,
FormatOptions options,
ILogger logger,
List<FormattedFile> formattedFiles,
Expand All @@ -260,24 +248,28 @@ private static void LogWorkspaceDiagnostics(ILogger logger, bool logWorkspaceWar
return formattedSolution;
}

internal static async Task<(int, ImmutableArray<(DocumentId, OptionSet, ICodingConventionsSnapshot)>)> DetermineFormattableFiles(
internal static async Task<(int, ImmutableArray<DocumentWithOptions>)> DetermineFormattableFilesAsync(
Solution solution,
string projectPath,
Matcher fileMatcher,
bool includeGeneratedFiles,
ILogger logger,
CancellationToken cancellationToken)
{
var codingConventionsManager = CodingConventionsManagerFactory.CreateCodingConventionsManager();
var optionsApplier = new EditorConfigOptionsApplier();
var totalFileCount = solution.Projects.Sum(project => project.DocumentIds.Count);
int projectFileCount = 0;

var fileCount = 0;
var getDocumentsAndOptions = new List<Task<(Document?, OptionSet?, ICodingConventionsSnapshot?, bool)>>(solution.Projects.Sum(project => project.DocumentIds.Count));
var documentsCoveredByEditorConfig = ImmutableArray.CreateBuilder<DocumentWithOptions>(totalFileCount);
var documentsNotCoveredByEditorConfig = ImmutableArray.CreateBuilder<DocumentWithOptions>(totalFileCount);

var addedFilePaths = new HashSet<string>(totalFileCount);

foreach (var project in solution.Projects)
{
if (project.FilePath is null)
if (project?.FilePath is null)
{
continue;
}

// If a project is used as a workspace, then ignore other referenced projects.
if (!string.IsNullOrEmpty(projectPath) && !project.FilePath.Equals(projectPath, StringComparison.OrdinalIgnoreCase))
Expand All @@ -293,100 +285,65 @@ private static void LogWorkspaceDiagnostics(ILogger logger, bool logWorkspaceWar
continue;
}

fileCount += project.DocumentIds.Count;

// Get project documents and options with .editorconfig settings applied.
var getProjectDocuments = project.DocumentIds.Select(documentId => GetDocumentAndOptions(
project, documentId, fileMatcher, includeGeneratedFiles, codingConventionsManager, optionsApplier, cancellationToken));
getDocumentsAndOptions.AddRange(getProjectDocuments);
}

var documentsAndOptions = await Task.WhenAll(getDocumentsAndOptions).ConfigureAwait(false);
var foundEditorConfig = documentsAndOptions.Any(documentAndOptions => documentAndOptions.Item4);
projectFileCount += project.DocumentIds.Count;

var addedFilePaths = new HashSet<string>(documentsAndOptions.Length);
var formattableFiles = ImmutableArray.CreateBuilder<(DocumentId, OptionSet, ICodingConventionsSnapshot)>(documentsAndOptions.Length);
foreach (var (document, options, codingConventions, hasEditorConfig) in documentsAndOptions)
{
if (document?.FilePath is null)
{
continue;
}

// If any code file has an .editorconfig, then we should ignore files without an .editorconfig entry.
if (foundEditorConfig && !hasEditorConfig)
foreach (var document in project.Documents)
{
continue;
}

// If we've already added this document, either via a link or multi-targeted framework, then ignore.
if (addedFilePaths.Contains(document.FilePath))
{
continue;
}

addedFilePaths.Add(document.FilePath);
formattableFiles.Add((document.Id, options, codingConventions));
}

return (fileCount, formattableFiles.ToImmutableArray());
}

private static async Task<(Document?, OptionSet?, ICodingConventionsSnapshot?, bool)> GetDocumentAndOptions(
Project project,
DocumentId documentId,
Matcher fileMatcher,
bool includeGeneratedFiles,
ICodingConventionsManager codingConventionsManager,
EditorConfigOptionsApplier optionsApplier,
CancellationToken cancellationToken)
{
var document = project.Solution.GetDocument(documentId);
// If we've already added this document, either via a link or multi-targeted framework, then ignore.
if (document?.FilePath is null ||
addedFilePaths.Contains(document.FilePath))
{
continue;
}

if (document is null || await ShouldIgnoreDocument(document, fileMatcher, includeGeneratedFiles, cancellationToken))
{
return (null, null, null, false);
}
addedFilePaths.Add(document.FilePath);

var context = await codingConventionsManager.GetConventionContextAsync(
document.FilePath, cancellationToken).ConfigureAwait(false);
if (!fileMatcher.Match(document.FilePath).HasMatches ||
!document.SupportsSyntaxTree)
{
continue;
}

OptionSet options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
var syntaxTree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);
if (syntaxTree is null)
{
throw new Exception($"Unable to get a syntax tree for '{document.Name}'");
}

// Check whether an .editorconfig was found for this document.
if (context?.CurrentConventions is null)
{
return (document, options, null, false);
if (!includeGeneratedFiles &&
await GeneratedCodeUtilities.IsGeneratedCodeAsync(syntaxTree, cancellationToken).ConfigureAwait(false))
{
continue;
}

var analyzerConfigOptions = document.Project.AnalyzerOptions.AnalyzerConfigOptionsProvider.GetOptions(syntaxTree);
var optionSet = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);

var formattableDocument = new DocumentWithOptions(document, optionSet, analyzerConfigOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Why is DocumentWithOptions needed if we can fetch the OptionSet when you need it later or potentially the AnalyzerConfigOptions too? Is this just a perf optimization or needed for something else?

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 thought I was saving work, but you are right. Since nothing in the solution is changing and these are cached within the DocumentState and AnalyzerConfigSet respectively. This could just return document ids and we could fetch everything again later.

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'll take this suggestion as a follow up refactoring. #691


// Track files covered by an editorconfig separately from those not covered.
if (analyzerConfigOptions is object)
{
documentsCoveredByEditorConfig.Add(formattableDocument);
}
else
{
documentsNotCoveredByEditorConfig.Add(formattableDocument);
}
}
}

options = optionsApplier.ApplyConventions(options, context.CurrentConventions, project.Language);
return (document, options, context.CurrentConventions, true);
}

private static async Task<bool> ShouldIgnoreDocument(
Document document,
Matcher fileMatcher,
bool includeGeneratedFiles,
CancellationToken cancellationToken)
{
if (!fileMatcher.Match(document.FilePath).HasMatches)
{
// If a files list was passed in, then ignore files not present in the list.
return true;
}
else if (!document.SupportsSyntaxTree)
{
return true;
}
else if (!includeGeneratedFiles && await GeneratedCodeUtilities.IsGeneratedCodeAsync(document, cancellationToken).ConfigureAwait(false))
{
// Ignore generated code files.
return true;
}
else
{
return false;
}
// Initially we would format all documents in a workspace, even if some files weren't covered by an
// .editorconfig and would have defaults applied. This behavior was an early requested change since
// users were surprised to have files not specified by the .editorconfig modified. The assumption is
// that users without an .editorconfig still wanted formatting (they did run a formatter after all),
// so we run on all files with defaults.

// If no files are covered by an editorconfig, then return them all. Otherwise only return
// files that are covered by an editorconfig.
JoeRobich marked this conversation as resolved.
Show resolved Hide resolved
return documentsCoveredByEditorConfig.Count == 0
? (projectFileCount, documentsNotCoveredByEditorConfig.ToImmutableArray())
: (projectFileCount, documentsCoveredByEditorConfig.ToImmutableArray());
}
}
}