Skip to content

Commit 11b6d7c

Browse files
authored
.Net: fix: Fix AOT Compatibility - Remove Expression.Compile() from Brave/Tavily TextSearch (#13541)
# PR: Fix AOT Compatibility - Remove Expression.Compile() from Brave/Tavily TextSearch ## Motivation and Context This PR addresses **Critical Issue #1** from Copilot's code review feedback on PR #13384 (feature-text-search-linq → main). **Problem:** The `ExtractValue()` method in both `BraveTextSearch` and `TavilyTextSearch` uses `Expression.Lambda(expression).Compile().DynamicInvoke()` as a fallback case, which breaks NativeAOT compatibility. This directly contradicts the ADR-0065 architectural decision that states: > "Both interfaces are AOT-compatible with no `[RequiresDynamicCode]` attributes" **Impact:** - Breaks NativeAOT compilation scenarios - Violates Semantic Kernel's AOT compatibility requirements - Contradicts documented architectural decisions - Blocks deployment scenarios requiring AOT **Root Cause:** The issue was introduced in PR #13191 (Tavily/Brave connector modernization) without AOT consideration. ## Description Replaces the AOT-incompatible `Expression.Lambda(expression).Compile().DynamicInvoke()` fallback with a `NotSupportedException` that: - Maintains AOT compatibility (no dynamic code generation) - Provides clear error message explaining supported expression types - Includes expression type and value for debugging - Guides users to use only constant expressions and simple member access ## Changes Made ### Files Modified - `dotnet/src/Plugins/Plugins.Web/Brave/BraveTextSearch.cs` - `dotnet/src/Plugins/Plugins.Web/Tavily/TavilyTextSearch.cs` ### Before (AOT-incompatible): ```csharp private static object? ExtractValue(Expression expression) { return expression switch { ConstantExpression constant => constant.Value, MemberExpression member when member.Expression is ConstantExpression constantExpr => member.Member switch { System.Reflection.FieldInfo field => field.GetValue(constantExpr.Value), System.Reflection.PropertyInfo property => property.GetValue(constantExpr.Value), _ => null }, _ => Expression.Lambda(expression).Compile().DynamicInvoke() // ❌ BREAKS AOT }; } ``` ### After (AOT-compatible): ```csharp private static object? ExtractValue(Expression expression) { return expression switch { ConstantExpression constant => constant.Value, MemberExpression member when member.Expression is ConstantExpression constantExpr => member.Member switch { System.Reflection.FieldInfo field => field.GetValue(constantExpr.Value), System.Reflection.PropertyInfo property => property.GetValue(constantExpr.Value), _ => null }, _ => throw new NotSupportedException( $"Unable to extract value from expression of type '{expression.GetType().Name}'. " + $"Only constant expressions and simple member access are supported for AOT compatibility. " + $"Expression: {expression}") }; } ``` ## Supported Expression Patterns ✅ **Works (AOT-compatible):** - Constant values: `page.Language == "en"` - Simple variables: `var lang = "en"; page.Language == lang` - Member access: `config.Language` (where `config` is a captured variable) ❌ **Not Supported (would require dynamic compilation):** - Computed values: `page.Language == someVariable` - Method calls: `page.Language == GetLanguage()` - Complex expressions: `page.Language == (isEnglish ? "en" : "fr")` Users encountering the exception should extract the value to a variable before using it in the filter expression. ## Testing - [x] Verified changes compile successfully - [x] Confirmed AOT compatibility (no `Expression.Compile()` usage) - [x] Exception message provides clear guidance - [ ] Unit tests pass (pending CI/CD validation) ## Related Issues/PRs - **Parent PR:** #13384 (feature-text-search-linq → main) - **Copilot Review:** Issue #1 - AOT Compatibility (Critical) - **Root Cause:** PR #13191 (Tavily/Brave modernization) - **ADR Reference:** docs/decisions/0065-linq-based-text-search-filtering.md (PR #13335) ## Contribution Checklist - [ ] The code builds clean without any errors or warnings - [x] Appropriate tests have been added (existing tests remain) - [x] Changes are documented as needed - [x] AOT compatibility verified ## Reviewer Notes @markwallace-microsoft, @westey-m This is the first of two critical fixes for PR #13384. The fix: 1. Eliminates all `Expression.Compile()` usage for AOT compatibility 2. Provides clear error messages for unsupported expression patterns 3. Aligns implementation with ADR-0065's AOT compatibility guarantee The exception should rarely be hit in practice since most filters use simple constants or variables (which are handled by the first two cases). Users with complex expressions will get immediate, clear feedback on how to fix their code. **Next Steps:** - Issue #2 (Breaking Change - Legacy Result Type) requires design decision - Issues #3-4 (Moderate bugs) can be addressed in follow-up PR --------- Co-authored-by: Alexander Zarei <alzarei@users.noreply.github.com>
1 parent c47eedd commit 11b6d7c

2 files changed

Lines changed: 50 additions & 16 deletions

File tree

dotnet/src/Plugins/Plugins.Web/Brave/BraveTextSearch.cs

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -496,14 +496,31 @@ private static void ExtractMethodCallClause(MethodCallExpression methodCall, Lis
496496
return expression switch
497497
{
498498
ConstantExpression constant => constant.Value,
499-
MemberExpression member when member.Expression is ConstantExpression constantExpr =>
500-
member.Member switch
501-
{
502-
System.Reflection.FieldInfo field => field.GetValue(constantExpr.Value),
503-
System.Reflection.PropertyInfo property => property.GetValue(constantExpr.Value),
504-
_ => null
505-
},
506-
_ => Expression.Lambda(expression).Compile().DynamicInvoke()
499+
MemberExpression member => ExtractMemberValue(member),
500+
_ => throw new NotSupportedException(
501+
$"Unable to extract value from expression of node type '{expression.NodeType}'. " +
502+
"Only constant expressions and member access are supported for AOT compatibility. " +
503+
"Expression: " + expression)
504+
};
505+
}
506+
507+
/// <summary>
508+
/// Extracts a value from a member expression by walking the member access chain.
509+
/// </summary>
510+
/// <param name="memberExpression">The member expression to evaluate.</param>
511+
/// <returns>The extracted value, or null if extraction failed.</returns>
512+
private static object? ExtractMemberValue(MemberExpression memberExpression)
513+
{
514+
// Recursively evaluate the member's expression (handles nested member access)
515+
var target = memberExpression.Expression is not null
516+
? ExtractValue(memberExpression.Expression)
517+
: null;
518+
519+
return memberExpression.Member switch
520+
{
521+
System.Reflection.FieldInfo field => field.GetValue(target),
522+
System.Reflection.PropertyInfo property => property.GetValue(target),
523+
_ => null
507524
};
508525
}
509526

dotnet/src/Plugins/Plugins.Web/Tavily/TavilyTextSearch.cs

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -489,14 +489,31 @@ private static void ExtractMethodCallClause(MethodCallExpression methodCall, Lis
489489
return expression switch
490490
{
491491
ConstantExpression constant => constant.Value,
492-
MemberExpression member when member.Expression is ConstantExpression constantExpr =>
493-
member.Member switch
494-
{
495-
System.Reflection.FieldInfo field => field.GetValue(constantExpr.Value),
496-
System.Reflection.PropertyInfo property => property.GetValue(constantExpr.Value),
497-
_ => null
498-
},
499-
_ => Expression.Lambda(expression).Compile().DynamicInvoke()
492+
MemberExpression member => ExtractMemberValue(member),
493+
_ => throw new NotSupportedException(
494+
$"Unable to extract value from expression of node type '{expression.NodeType}'. " +
495+
"Only constant expressions and member access are supported for AOT compatibility. " +
496+
"Expression: " + expression)
497+
};
498+
}
499+
500+
/// <summary>
501+
/// Extracts a value from a member expression by walking the member access chain.
502+
/// </summary>
503+
/// <param name="memberExpression">The member expression to evaluate.</param>
504+
/// <returns>The extracted value, or null if extraction failed.</returns>
505+
private static object? ExtractMemberValue(MemberExpression memberExpression)
506+
{
507+
// Recursively evaluate the member's expression (handles nested member access)
508+
var target = memberExpression.Expression is not null
509+
? ExtractValue(memberExpression.Expression)
510+
: null;
511+
512+
return memberExpression.Member switch
513+
{
514+
System.Reflection.FieldInfo field => field.GetValue(target),
515+
System.Reflection.PropertyInfo property => property.GetValue(target),
516+
_ => null
500517
};
501518
}
502519

0 commit comments

Comments
 (0)