Scope .editorconfig discovery to included files in dotnet format folder mode#55248
Scope .editorconfig discovery to included files in dotnet format folder mode#55248wellWINeo wants to merge 7 commits into
Conversation
|
@dotnet-policy-service agree |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Improves dotnet format whitespace --folder --include <file> performance by avoiding a full workspace .editorconfig scan when the include set is an explicit list of files under the workspace folder, and instead collecting only ancestor .editorconfig files that can actually apply.
Changes:
- Add
EditorConfigFinder.GetEditorConfigPathsForFiles(...)to discover.editorconfigfiles by walking ancestor directories for specific files. - Update folder-mode solution loading to use the file-based
.editorconfigdiscovery when--includeis a concrete file list (and--excludeis empty) fully contained under the workspace folder. - Add unit tests covering ancestor-only
.editorconfigdiscovery and duplicate prevention.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/dotnet-format.UnitTests/Utilities/EditorConfigFinderTests.cs | Adds unit coverage for the new file-based .editorconfig discovery behavior. |
| src/Dotnet.Format/dotnet-format/Workspaces/FolderWorkspace_FolderSolutionLoader.cs | Switches .editorconfig discovery to a fast ancestor-walk when includes are concrete files under the workspace. |
| src/Dotnet.Format/dotnet-format/Utilities/EditorConfigFinder.cs | Introduces GetEditorConfigPathsForFiles to collect applicable configs without scanning the full folder tree. |
| // Only configs in the files' ancestor directories can apply to them. Included | ||
| // files resolving outside the workspace folder keep the original scan so their | ||
| // behavior is unchanged. | ||
| var folderPrefix = absoluteFolderPath.EndsWith(Path.DirectorySeparatorChar) |
There was a problem hiding this comment.
Can you just make this part of calculating the absoluteFolderPath above?
if (!Path.EndsInDirectorySeparator(absoluteFolderPath))
{
absoluteFolderPath += Path.DirectorySeparatorChar;
}There was a problem hiding this comment.
Obsolete — folderPrefix only existed for the containment check, which is removed.
| return editorConfigPaths.ToImmutable(); | ||
| } | ||
|
|
||
| public static ImmutableArray<string> GetEditorConfigPathsForFiles(ImmutableArray<string> filePaths) |
There was a problem hiding this comment.
Can you have GetEditorConfigPaths(string) simply call this method? Update where you set directory name to first check if the path is a directory before calling GetDirectoryName
There was a problem hiding this comment.
Removed GetEditorConfigPaths(string) instead of unifying — the loader always calls GetEditorConfigPathsForFiles with the resolved file list, so there's a single discovery path.
| foreach (var filePath in filePaths) | ||
| { | ||
| var directoryName = Path.GetDirectoryName(filePath); | ||
| if (string.IsNullOrEmpty(directoryName) || !Directory.Exists(directoryName)) |
There was a problem hiding this comment.
Consider only doing the Directory.Exists() check after visitedDirectories check, so that way we don't pay it once we've already added the directory to the HashSet.
There was a problem hiding this comment.
Applied — dropped the guard entirely; File.Exists doesn't throw on a missing directory.
| var editorConfigPaths = hasConcreteIncludePaths | ||
| && filePaths.All(filePath => filePath.StartsWith(folderPrefix, StringComparison.OrdinalIgnoreCase)) | ||
| ? EditorConfigFinder.GetEditorConfigPathsForFiles(filePaths) | ||
| : EditorConfigFinder.GetEditorConfigPaths(folderPath); |
There was a problem hiding this comment.
So does this mean that as long as there's one glob then we'll still end up finding all the editor configs in the entire folder instead? What happens if we just always call GetEditorConfigPathsForFiles?
There was a problem hiding this comment.
Done — always file-based now; GetEditorConfigPaths(folderPath) removed.
Instead of gating the ancestor walk to concrete --include lists and falling back to a full-tree scan, always collect .editorconfig files by walking up from each formatted file. Remove the now-unused GetEditorConfigPaths(folderPath) full scan and the containment gating. In GetEditorConfigPathsForFiles, use File.Exists instead of DirectoryInfo.GetFiles and drop the redundant Directory.Exists guard.
|
Adopting @jasonmalinowski's suggestion to always use the ancestor walk (dropping the concrete-include gating) removes two things the previous version preserved. Both are now documented in the PR description Notes; flagging here since they differ from what you reviewed.
|
|
@wellWINeo I think both of those seem like reasonable behavior changes to me (unless @JoeRobich wants to overrule me.) The first behavior seems otherwise pretty broken (you'll format with the wrong settings), and in the second case, if there's multiple global configs in the same repo, you'll get "who knows" behavior (since we won't have the right logic for which one to consume.) |
JoeRobich
left a comment
There was a problem hiding this comment.
I like the direction this is headed.
|
|
||
| var filePaths = GetMatchingFilePaths(absoluteFolderPath, fileMatcher); | ||
| var editorConfigPaths = EditorConfigFinder.GetEditorConfigPaths(folderPath); | ||
| var hasConcreteIncludePaths = fileMatcher.Exclude.IsDefaultOrEmpty && AreAllFilePaths(fileMatcher.Include); |
There was a problem hiding this comment.
Lets revert pulling this out of the GetMatchingFilePaths method
| } | ||
| } | ||
|
|
||
| private static bool AreAllFilePaths(ImmutableArray<string> globs) |
There was a problem hiding this comment.
Lets revert pulling this out of the GetMatchingFilePaths method
…gFilePaths With the .editorconfig discovery ternary gone, the flag and helper no longer need to be hoisted to LoadSolutionInfo, so restore GetMatchingFilePaths to its original shape.
…derSolutionLoader.cs Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. |
Summary
dotnet format whitespace --folderscanned the entire workspace tree for.editorconfigfiles (EditorConfigFinder.GetEditorConfigPathsusedGetFiles(".editorconfig", SearchOption.AllDirectories)). In monorepos with large non-.NET subtrees (e.g.node_modules) that whole-tree scan dominates folder-mode runtime, even though the vast majority of those directories contain no file being formatted.This PR replaces the scan with an upward walk: for the set of files the workspace will actually format, it collects the
.editorconfigfiles in each file's ancestor directories (deduplicating shared ancestors), viaEditorConfigFinder.GetEditorConfigPathsForFiles. The full-treeGetEditorConfigPaths(folderPath)is removed.Correctness. A non-global
.editorconfigonly affects files in its own directory subtree, so the only configs that can change a file's effective options are those on the path from the workspace root down to the file's own directory. The folder workspace formats exactly the resolved file set, so the union of ancestor walks over that set yields identical analyzer-config resolution for every document; the configs the old scan collected from directories that are not an ancestor of any formatted file could never have matched them. Two edge cases where this differs from the old scan are in Notes.Measurements
On a large work monorepo (400+ C# projects plus a JS frontend with ~230
node_modulesdirectories), formatting one file. Numbers are the median of 3 timed runs after 1 warmup; "Before" and "After" are two Debug builds of this branch — the parent commit and this change — measured on the same machine:dotnet format whitespace . --folder --include src/App/Program.csReferences
--includeperformance) and dotnet-format is very slow on larger repos format#1378.--include.Notes
--excludeand MSBuild (project/solution) workspaces are unchanged; this only affects folder-mode.editorconfigdiscovery. Both concrete--includelists and glob includes now use the ancestor walk (globs resolve to files under the workspace folder, so their ancestor set is the same the scan would have found).--includepath resolving outside the workspace folder now has the.editorconfigfiles in its own ancestor directories applied. Previously folder mode discovered configs from the workspace folder's tree only, so such a file was formatted with the workspace folder's configs rather than its own — a behavior change (arguably more correct).is_globalconfigs: a file named.editorconfigcontainingis_global = trueis treated by Roslyn as a global analyzer config (its top-level options apply regardless of directory). The ancestor walk only visits directories on the path from a formatted file up to the root, so a global-marked.editorconfigin a directory that is not an ancestor of any formatted file is no longer discovered. Rare: the convention for repo-wide configuration is.globalconfig(not discovered by folder mode under old or new code), andis_globalis not part of the EditorConfig spec. Use.globalconfigfor genuinely global configuration.