Repository navigation
Check each referenced project once in dotnet format - #56621
jasonmalinowski merged 2 commits into
Conversation
|
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. |
ec14325 to
ccb3cc9
Compare
exp-ouroborous
changed the title
|
@dotnet-policy-service agree company="Epic Systems Corporation" |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the reference-loading semantics of a shipped user-facing tool (dotnet format), so despite the clean, well-tested implementation it warrants final human review before approval.
No open issues 🎉
What changed in this PR
This PR fixes a severe performance problem in dotnet format style / dotnet format analyzers (issue #56363). Before running analyzers, AnalyzerRunner verified that every transitively referenced project's compilation loaded by recursively walking ProjectReferences without memoizing visited projects, so shared dependencies in dense reference graphs were re-checked once per path — exponential in graph depth, causing runs to take minutes to hours. The fix replaces the recursive walk with a single linear pass over the solution's cached ProjectDependencyGraph, checking each project's compilation exactly once while preserving the existing "required references did not load" warning behavior.
Changes:
- Rewrote
AllReferencedProjectsLoadedAsyncto iterateGetProjectsThatThisProjectTransitivelyDependsOn(project.Id)plus the project itself, checking each compilation once instead of recursing. - Added
AnalyzerRunnerTestscovering prompt completion on a dense 40-project graph, and skip-with-warning behavior when a shared dependency and an indirectly-reachable dependency are missing corlib.
| File | Description |
|---|---|
src/Dotnet.Format/dotnet-format/Analyzers/AnalyzerRunner.cs |
Replaces the exponential recursive reference walk with a single pass over the cached project dependency graph, preserving the compilation/ObjectType checks and warning. |
test/dotnet-format.UnitTests/Analyzers/AnalyzerRunnerTests.cs |
New tests validating prompt completion on a dense graph and that missing shared/indirect references still skip the project with the expected warning. |
Review effort: Balanced
|
@dotnet-policy-service agree company="Epic Systems Corporation" |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
The code fix is fine, but we can simplify the tests by removing some stuff we don't need.
Before running analyzers, AnalyzerRunner recursively walks every project reference to confirm the compilations loaded. Shared dependencies are re-walked on every path that reaches them, so the cost grows exponentially with the depth of the reference graph. Dense graphs (e.g. solutions using DisableTransitiveProjectReferences) make `dotnet format style` and `dotnet format analyzers` take minutes to hours. Replace the recursion with a single pass over the project and its transitive dependencies from the solution's cached ProjectDependencyGraph, so each compilation is checked exactly once. Any dependency that fails to load still skips the project with the existing warning. Fixes dotnet#56363
Drop the custom timeout wrapper; the test harness already dumps hung tests, so a regression to the exponential walk trips that instead. Drop the missing-corlib tests: the old recursive walk already reported broken dependencies correctly, so they do not guard the regression this change fixes. With one caller left, inline the graph construction.
65e9609 to
9cf5dad
Compare
Backport of dotnet#56621 to release/10.0.1xx. Before running analyzers, AnalyzerRunner recursively walks every project reference to confirm the runtime references loaded. Shared dependencies are re-walked on every path that reaches them, so the cost grows exponentially with the depth of the reference graph. Dense graphs (e.g. solutions using DisableTransitiveProjectReferences) make `dotnet format style` and `dotnet format analyzers` take minutes to hours. Replace the recursion with a single pass over the project and its transitive dependencies from the solution's cached ProjectDependencyGraph, so each project is checked exactly once. This is a manual port: this branch keeps its existing synchronous mscorlib.dll check rather than main's compilation-based check, and the tests are translated from MSTest to xUnit. Fixes dotnet#56363
|
Hi @jasonmalinowski, thanks for the review! I've addressed both test simplification comments and rebased this PR onto the latest main. I've also rebased the .NET 10 backport (#56728) onto the latest release/10.0.1xx; it remains a draft pending this PR. When you have a chance, could you please take another look? Thanks! |
|
@ # #56499 |
|
/backport to release/11.0.1xx |
|
Started backporting to |
Fixes #56363
Problem
Before running analyzers,
AnalyzerRunner.RunCodeAnalysisAsyncchecks that every referenced project's compilation loaded by recursively walkingProjectReferences. The walk never remembers which projects it already verified, so a shared dependency is re-walked once per path that reaches it. The cost grows exponentially with the depth of the reference graph, which makesdotnet format style/dotnet format analyzerstake minutes to hours on solutions with dense reference graphs (for example, ones usingDisableTransitiveProjectReferences).Fix
Replace the recursive walk with a single pass over the project and its transitive dependencies, taken from the solution's cached
ProjectDependencyGraph(GetProjectsThatThisProjectTransitivelyDependsOn). Each project's compilation is checked exactly once, with no recursion. Any dependency that fails to load still skips the project with the existing "required references did not load" warning.Tests
Added
AnalyzerRunnerTests.RunCodeAnalysisAsync_DenseProjectReferenceGraph_CompletesPromptly: 40 projects, each referencing every earlier one. Without the fix the walk effectively never finishes and the test harness's hang detection trips; with the fix it completes in a few seconds.Locally, the rest of
dotnet-format.UnitTestsis unchanged by this PR. (Tests that need a builtredistlayout were not run locally, andTestUseVarCodeStyle_AppliesWhenNotUsingVarfails onmainwith or without this change.)Servicing
The same unmemoized recursion exists in the
release/10.0.*xxbranches, but in a different form: a synchronousAllReferencedProjectsLoadedthat checks for anmscorlib.dllmetadata reference, undersrc/BuiltInTools/dotnet-formatin 10.0.1xx/2xx. So this won't cherry-pick cleanly and needs a manual port. Would maintainers consider servicing this fix into .NET 10? I'm happy to prepare the backport PR(s).