perf(analyzers): trim remaining analyzer hot-path symbol lookups and binds - #6928
Conversation
GitHub Actions
Error
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe changes centralize TUnit analyzer symbol lookup, conditionally register WebApplicationFactory analysis, and refine generic invocation filtering in mock analyzers. A new analyzer test checks an unqualified ChangesTUnit symbol lookup
Web application factory analysis
Mock analyzer invocation filtering
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains in the reviewed analyzer changes. Merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the symbols bright, Comment |
Review: perf(analyzers) hot-path trimmingI found no blocking issues. The change matches its description and diagnostics stay the same. What I checked
Non-blocking suggestions
Good, focused change with a test for the edge case. Approving in spirit. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 257e5686e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| private static bool ReferencesWebApplicationTest(Compilation compilation) | ||
| { | ||
| foreach (var tunitNamespace in compilation.GlobalNamespace.GetNamespaceMembers()) |
There was a problem hiding this comment.
When TUnit.AspNetCore.Core is referenced only through an extern alias (a reference with no global alias), Compilation.GlobalNamespace omits that assembly, so this scan returns false and the compilation-start callback never registers either operation action. Such code can still bind and use alias::TUnit.AspNetCore.WebApplicationTest, meaning diagnostics previously emitted by this analyzer silently disappear; the repository's MigrationNamespaceHelper handles the same Roslyn limitation by explicitly scanning SourceModule.ReferencedAssemblySymbols (src/TUnit.Analyzers/Migrators/Base/MigrationNamespaceHelper.cs:47-68). Please include referenced assemblies in this gate as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed regression (main had no gate). Fixed in 4d907f2: the gate now uses Compilation.GetTypesByMetadataName for TUnit.AspNetCore.WebApplicationTest and WebApplicationTest2, which searches every reference including extern-aliased ones and doesn't null out on ambiguity. Added Error_When_WebApplicationTest_Is_Only_Referenced_Through_Extern_Alias`, which fails against the previous namespace walk.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…references The compilation-start gate walked Compilation.GlobalNamespace, which omits references only reachable through an extern alias, so TUnit0062/0063 were silently dropped for such projects. Use GetTypesByMetadataName (all references, no null-on-ambiguity) for both arities and add a regression test.
ReviewI found no blocking issues. The changes match the PR description. Checked
Minor suggestions (non-blocking)
LGTM. |
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.71.0 to 1.72.4. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.72.4 <!-- Release notes generated using configuration in .github/release.yml at v1.72.4 --> ## What's Changed ### Other Changes * fix(source-gen): stop parameter resolver keeping every non-public test-class method (IL2111) by @thomhurst in thomhurst/TUnit#6937 ### Dependencies * chore(deps): update tunit to 1.72.0 by @thomhurst in thomhurst/TUnit#6934 **Full Changelog**: thomhurst/TUnit@v1.72.0...v1.72.4 ## 1.72.0 <!-- Release notes generated using configuration in .github/release.yml at v1.72.0 --> ## What's Changed ### Other Changes * perf(source-gen): resolve parameter reflection info through a shared runtime helper by @thomhurst in thomhurst/TUnit#6923 * perf(analyzers): trim remaining analyzer hot-path symbol lookups and binds by @thomhurst in thomhurst/TUnit#6928 * perf(mocks): move shared MockCall wrapper plumbing into runtime base classes by @thomhurst in thomhurst/TUnit#6929 * perf(source-gen): close incremental caching gaps in static property and property injection generators by @thomhurst in thomhurst/TUnit#6925 * perf(source-gen): stop InfrastructureGenerator pinning an old Compilation by @thomhurst in thomhurst/TUnit#6926 * perf(assertions-analyzers): cache assertion symbols and cut per-call work by @thomhurst in thomhurst/TUnit#6927 * perf(source-gen): emit hooks per class with direct, non-async bodies by @thomhurst in thomhurst/TUnit#6924 * test: fix flaky ObjectInitializer continuation-thread test by @thomhurst in thomhurst/TUnit#6932 * fix: CI flakes from leaked hook contexts, ActivityCollector race and Repro5700 rendezvous by @thomhurst in thomhurst/TUnit#6933 * fix(aspnetcore): honor WebApplicationFactoryClientOptions in CreateClient by @thomhurst in thomhurst/TUnit#6931 ### Dependencies * chore(deps): update tunit to 1.71.0 by @thomhurst in thomhurst/TUnit#6920 **Full Changelog**: thomhurst/TUnit@v1.71.0...v1.72.0 Commits viewable in [compare view](thomhurst/TUnit@v1.71.0...v1.72.4). </details> Updated [TUnit.AspNetCore](https://github.com/thomhurst/TUnit) from 1.71.0 to 1.72.4. <details> <summary>Release notes</summary> _Sourced from [TUnit.AspNetCore's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.72.4 <!-- Release notes generated using configuration in .github/release.yml at v1.72.4 --> ## What's Changed ### Other Changes * fix(source-gen): stop parameter resolver keeping every non-public test-class method (IL2111) by @thomhurst in thomhurst/TUnit#6937 ### Dependencies * chore(deps): update tunit to 1.72.0 by @thomhurst in thomhurst/TUnit#6934 **Full Changelog**: thomhurst/TUnit@v1.72.0...v1.72.4 ## 1.72.0 <!-- Release notes generated using configuration in .github/release.yml at v1.72.0 --> ## What's Changed ### Other Changes * perf(source-gen): resolve parameter reflection info through a shared runtime helper by @thomhurst in thomhurst/TUnit#6923 * perf(analyzers): trim remaining analyzer hot-path symbol lookups and binds by @thomhurst in thomhurst/TUnit#6928 * perf(mocks): move shared MockCall wrapper plumbing into runtime base classes by @thomhurst in thomhurst/TUnit#6929 * perf(source-gen): close incremental caching gaps in static property and property injection generators by @thomhurst in thomhurst/TUnit#6925 * perf(source-gen): stop InfrastructureGenerator pinning an old Compilation by @thomhurst in thomhurst/TUnit#6926 * perf(assertions-analyzers): cache assertion symbols and cut per-call work by @thomhurst in thomhurst/TUnit#6927 * perf(source-gen): emit hooks per class with direct, non-async bodies by @thomhurst in thomhurst/TUnit#6924 * test: fix flaky ObjectInitializer continuation-thread test by @thomhurst in thomhurst/TUnit#6932 * fix: CI flakes from leaked hook contexts, ActivityCollector race and Repro5700 rendezvous by @thomhurst in thomhurst/TUnit#6933 * fix(aspnetcore): honor WebApplicationFactoryClientOptions in CreateClient by @thomhurst in thomhurst/TUnit#6931 ### Dependencies * chore(deps): update tunit to 1.71.0 by @thomhurst in thomhurst/TUnit#6920 **Full Changelog**: thomhurst/TUnit@v1.71.0...v1.72.0 Commits viewable in [compare view](thomhurst/TUnit@v1.71.0...v1.72.4). </details> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
Small follow-up to #6917 for analyzer hot paths it didn't cover (TUnit.Assertions.Analyzers is left alone; another PR owns it). Diagnostics are unchanged. The gains are small: the only one that clearly shows up in measurements is in TUnit.Mocks.Analyzers. The rest mainly simplify code and stop repeating per-attribute lookups.
What changed
TUnit.Mocks.Analyzers: the pre-filter requires a generic name where T can't be inferred
Mock.Of<...>,MockRepository.Of<T>,Mock.OfDelegate<T>,Arg.IsNull<T>()andArg.IsNotNull<T>()have no parameter that mentions their type parameters. I checked every overload inMock.cs,MockRepository.csandArg.cs, and none is non-generic. So every call to them is writtenName<...>(...).InvocationNameFilter.MayInvokeGenericalso rejects call sites that aren't aGenericNameSyntax.ArgIsNullNonNullableAnalyzerno longer binds every TUnit assertion.IsNotNull()/.IsNull()in projects that use both packages. TheOf/OfDelegateanalyzers skip other non-generic.Of(...)calls too.Wrap<T>(instance)(T is inferable) andT.Mock()keep the loose name match.using static Arg; IsNull<int>()(a bareGenericNameSyntax) still reports TM005.TUnit.Analyzers: more symbols in the
TUnitSymbolsper-compilation cacheBaseTestAttribute,MatrixAttribute,MatrixDataSourceAttribute,CombinedDataSourceAttributeandIDataSourceAttribute.MethodExtensions.HasTestAttributenow callsTUnitSymbols.HasTestAttribute. That is a plain loop over the base types, stopping atobjectasGetSelfAndBaseTypesdid, without aGetTypeByMetadataNamecall or a LINQ iterator per call.IsMatrixAttribute,IsMatrixDataSourceAttribute,IsCombinedDataSourceAttributeandIsDataSourceAttributeread the cached symbols instead of doing a type lookup for each attribute. Their comparison semantics are unchanged, including the existing null handling.TUnit.AspNetCore.Analyzers:
WebApplicationFactoryAccessAnalyzerImmutableHashSet<string>.Containsname checks are nowname is "A" or "B"patterns.TUnit.AspNetCore.WebApplicationTesttype of any arity. It walks namespaces rather than callingGetTypeByMetadataName, which returns null on ambiguity, so the gate matches the existing name-basedIsWebApplicationTestTypecheck exactly.Measurements
Method:
-p:ReportAnalyzer=true -v:d, net10.0 Release, warm compiler server, incremental rebuild after touching one.csfile with--no-dependencies. I ran 3 interleaved rounds of origin/main and this branch, 2 builds per project per round, and report medians (n=6). The machine was loaded by other builds, so assembly totals are noisy.TUnit.AspNetCore.Testsdoesn't loadTUnit.AspNetCore.Analyzersin-repo, because the analyzer is referenced only fromTUnit.AspNetCore.Core. So there's no in-repo number for that change; it's a simplification plus a cheap gate.Testing done
tests/TUnit.Analyzers.Tests: 833 passed, 1 skipped (unchanged)tests/TUnit.Mocks.Analyzers.Tests: 64 passed (63 existing + 1 new)tests/TUnit.AspNetCore.Analyzers.Tests: 29 passedSummary by CodeRabbit