Skip to content

Simplify PatternMatchingUtility by removing expression trees#1766

Merged
grvillic merged 3 commits intomainfrom
refactor/simplify-pattern-matching-utility
Apr 7, 2026
Merged

Simplify PatternMatchingUtility by removing expression trees#1766
grvillic merged 3 commits intomainfrom
refactor/simplify-pattern-matching-utility

Conversation

@JamieMagee
Copy link
Copy Markdown
Member

Replaces the expression tree and reflection-based implementation in PatternMatchingUtility with plain closures.

The old code used Expression.Lambda, Expression.Call, and four reflection GetMethod lookups to build a compiled delegate at runtime. This was unnecessary given the small, static pattern lists from detector SearchPatterns (typically 1-3 entries). The reflection calls had no null checks and would throw cryptic errors if the API surface changed. Calling Aggregate on an empty pattern list would throw InvalidOperationException.

The new version calls the same ReadOnlySpan<char> extension methods directly from closures. It also handles *foo* (contains) patterns, which the old code silently treated as exact matches.

Copilot AI review requested due to automatic review settings April 6, 2026 19:38
@JamieMagee JamieMagee requested a review from a team as a code owner April 6, 2026 19:38
@JamieMagee JamieMagee requested a review from VXianOng April 6, 2026 19:38

This comment was marked as outdated.

@JamieMagee JamieMagee force-pushed the refactor/simplify-pattern-matching-utility branch from 1bfb55b to 8547c2c Compare April 6, 2026 19:51
@JamieMagee JamieMagee requested a review from Copilot April 6, 2026 19:52

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR simplifies PatternMatchingUtility by replacing the prior expression-tree/reflection approach with straightforward, prebuilt closures for pattern matching. This reduces runtime complexity and removes failure modes from reflection lookups, while also making empty pattern lists safe to handle.

Changes:

  • Replaced expression-tree compiled predicate generation with a list of simple per-pattern match delegates (closures).
  • Added support for *foo* “contains” patterns, and ensured empty pattern lists return a matcher that never matches (instead of throwing).
  • Consolidated and expanded unit test coverage to cover more pattern variants, multiple-pattern OR behavior, and empty pattern lists.
Show a summary per file
File Description
src/Microsoft.ComponentDetection.Common/PatternMatchingUtility.cs Removes expression trees/reflection and implements matching via direct span-based closures (including *foo* contains and empty pattern handling).
test/Microsoft.ComponentDetection.Common.Tests/PatternMatchingUtilityTests.cs Updates tests to data-drive single-pattern cases, and adds coverage for multi-pattern and empty-pattern behavior.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new

@grvillic grvillic merged commit f40d70c into main Apr 7, 2026
31 of 32 checks passed
@grvillic grvillic deleted the refactor/simplify-pattern-matching-utility branch April 7, 2026 16:56
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.0%. Comparing base (97f3d73) to head (e7b0e8e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #1766   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

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.

4 participants