Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 15, 2025

Addressed review feedback to improve analyzer performance and modernize test code.

Performance optimization

  • Added syntax-based checks before semantic model lookups to avoid expensive symbol resolution for every invocation in the compilation
  • Filter by method identifier name ("EnableFeature", "Defaults") and class declaration presence before querying semantic model
  • Reduces unnecessary semantic analysis for non-matching invocations

Test modernization

  • Converted test code blocks from verbatim strings (@"") to raw string literals (""") for better readability and consistency with C# 11+ conventions

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copy link
Contributor Author

Copilot AI commented Nov 15, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • f.feedz.io
    • Triggering command: dotnet build src/NServiceBus.Core.Analyzer/NServiceBus.Core.Analyzer.csproj (dns block)
    • Triggering command: dotnet build src/NServiceBus.Core.Analyzer/NServiceBus.Core.Analyzer.csproj --no-restore (dns block)
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.23.3/x64/codeql/csharp/tools/linux64/Semmle.Autobuild.CSharp (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Add analyzer for feature defaults enforcement Optimize analyzer performance with syntax-based filtering Nov 15, 2025
Copilot AI requested a review from danielmarbach November 15, 2025 11:44
@danielmarbach
Copy link
Contributor

@copilot I'm wondering if we could make it more performant by checking the inside defaults method first before we check the lambda part or is that not a good idea?

Copy link
Contributor Author

Copilot AI commented Nov 15, 2025

@copilot I'm wondering if we could make it more performant by checking the inside defaults method first before we check the lambda part or is that not a good idea?

The current order is actually optimal. We can't check for Defaults before checking the lambda because:

  1. We need to traverse up to find a lambda first (line 61)
  2. Only then can we access defaultsInvocation through the lambda's parent structure (lines 63-72)
  3. Then we can check if that invocation is named "Defaults" (lines 75-85)

The lambda check is a prerequisite for accessing the defaultsInvocation variable, so we can't reorder them. The current flow already filters efficiently:

  • EnableFeature name check (cheapest, filters most invocations)
  • Lambda existence check (cheap tree traversal)
  • Defaults name check (also cheap, using the invocation we found via the lambda)
  • Class declaration check
  • Then semantic model lookups (most expensive)

@danielmarbach
Copy link
Contributor

@copilot Are the additional name checks on the semantic model now redundant or best practice?

Copy link
Contributor Author

Copilot AI commented Nov 15, 2025

@copilot Are the additional name checks on the semantic model now redundant or best practice?

Best practice. The syntax-based name checks are optimizations to bail early, while the semantic name checks verify we have the exact correct methods:

Syntax checks (lines 55, 82): Filter out obviously wrong invocations quickly

  • Check identifier text only
  • Could match any EnableFeature/Defaults method

Semantic checks (lines 103, 116): Authoritative verification

  • Verify the method is from the correct type (SettingsExtensions.EnableFeature and Feature.Defaults)
  • Essential to avoid false positives (e.g., user-defined EnableFeature methods)

This pattern (fast syntax filter → precise semantic verification) is the recommended approach for Roslyn analyzers.

@danielmarbach danielmarbach marked this pull request as ready for review November 15, 2025 11:54
@danielmarbach danielmarbach merged commit c30667b into defaults-fixer Nov 15, 2025
9 checks passed
@danielmarbach danielmarbach deleted the copilot/sub-pr-7468 branch November 15, 2025 11:54
danielmarbach added a commit that referenced this pull request Nov 19, 2025
…es (#7468)

* Error on Feature Defaults enabling a feature

* CSharp14

* Modernize

* Optimize analyzer performance with syntax-based filtering (#7470)

* Initial plan

* Optimize analyzer performance and modernize test code blocks

Co-authored-by: danielmarbach <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: danielmarbach <[email protected]>

* Simplify

---------

Co-authored-by: Daniel Marbach <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: danielmarbach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants