-
Notifications
You must be signed in to change notification settings - Fork 841
Clean up local function names in AIFunctionFactory #6909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: stephentoub <[email protected]>
test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionFactoryTest.cs
Outdated
Show resolved
Hide resolved
|
@copilot, there are also build failures in the tests from analyzers: Please make sure build is successful and all tests successfully complete before declaring success. |
Co-authored-by: stephentoub <[email protected]>
There was a problem hiding this 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 enhances the SanitizeMemberName method in AIFunctionFactory to provide cleaner names for local functions and lambdas used as AI tools. The changes ensure that compiler-generated names are transformed into more readable formats while maintaining uniqueness for multiple functions in the same method.
- Enhanced
SanitizeMemberNameto handle local functions and lambdas with specific regex patterns - Added comprehensive tests covering various scenarios including multiple functions in the same method
- Ensured lambda names retain complete ordinal suffixes for uniqueness
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.AI.Abstractions/Functions/AIFunctionFactory.cs | Enhanced SanitizeMemberName method with regex patterns for local functions and lambdas, added new regex methods |
| test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionFactoryTest.cs | Added comprehensive test coverage for local functions, lambdas, and edge cases with a helper static method |
Co-authored-by: stephentoub <[email protected]>
Fixed in commit 3d7e842. Added explanatory comments to all empty test method bodies (local functions and lambdas) to satisfy analyzer requirements S1186 and S108. Build now succeeds with 0 warnings and 0 errors, and all 346 tests pass. |
|
@jaredpar, I don't love taking a dependency here on the compiler's implementation detail around naming. Am I right to be nervous, or is what this PR is doing reasonable? |
What's the worst thing that could happen if the naming convention changes? I take it it would just revert to using the original name. |
Probably... though if the compiler's scheme were to change in specific ways, in theory I think we could end up with some duplicated names in corner cases, which could lead to ambiguity in the names provided to the LLM. In practice, though, I don't think that would happen, and really a developer should be providing a better name explicitly. |
test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionFactoryTest.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionFactoryTest.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Functions/AIFunctionFactory.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
src/Libraries/Microsoft.Extensions.AI.Abstractions/Functions/AIFunctionFactory.cs
Outdated
Show resolved
Hide resolved
|
Also, @copilot, there are build failures for netstandard2.0 / net462: |
Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
Fixed in commit 8b4c7e3. Used conditional compilation to handle the string operations differently for older frameworks. For netstandard2.0/net462, using |
* Initial plan * Implement local function name cleanup for AIFunctionFactory Co-authored-by: stephentoub <[email protected]> * Fix lambda name uniqueness by capturing full ordinal suffix Co-authored-by: stephentoub <[email protected]> * Fix analyzer errors by adding comments to empty test methods Co-authored-by: stephentoub <[email protected]> * Change Assert.Contains to Assert.StartsWith for lambda name tests Co-authored-by: stephentoub <[email protected]> * Consolidate regexes and improve test parameter names Co-authored-by: stephentoub <[email protected]> * Keep ordinal suffix for local functions to ensure uniqueness Co-authored-by: stephentoub <[email protected]> * Fix build errors for netstandard2.0 and net462 targets Co-authored-by: stephentoub <[email protected]> * Cleanup --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: stephentoub <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
* Initial plan * Implement local function name cleanup for AIFunctionFactory Co-authored-by: stephentoub <[email protected]> * Fix lambda name uniqueness by capturing full ordinal suffix Co-authored-by: stephentoub <[email protected]> * Fix analyzer errors by adding comments to empty test methods Co-authored-by: stephentoub <[email protected]> * Change Assert.Contains to Assert.StartsWith for lambda name tests Co-authored-by: stephentoub <[email protected]> * Consolidate regexes and improve test parameter names Co-authored-by: stephentoub <[email protected]> * Keep ordinal suffix for local functions to ensure uniqueness Co-authored-by: stephentoub <[email protected]> * Fix build errors for netstandard2.0 and net462 targets Co-authored-by: stephentoub <[email protected]> * Cleanup --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: stephentoub <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
* Initial plan * Implement local function name cleanup for AIFunctionFactory Co-authored-by: stephentoub <[email protected]> * Fix lambda name uniqueness by capturing full ordinal suffix Co-authored-by: stephentoub <[email protected]> * Fix analyzer errors by adding comments to empty test methods Co-authored-by: stephentoub <[email protected]> * Change Assert.Contains to Assert.StartsWith for lambda name tests Co-authored-by: stephentoub <[email protected]> * Consolidate regexes and improve test parameter names Co-authored-by: stephentoub <[email protected]> * Keep ordinal suffix for local functions to ensure uniqueness Co-authored-by: stephentoub <[email protected]> * Fix build errors for netstandard2.0 and net462 targets Co-authored-by: stephentoub <[email protected]> * Cleanup --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: stephentoub <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
Summary
Fixed local function name cleanup for AIFunctionFactory, ensuring both lambdas and local functions have unique names.
Changes Made
1. Enhanced
SanitizeMemberNamein AIFunctionFactory.cs<Method>g__LocalFunc|x_y→Method_LocalFunc_x_y(keeps ordinal for uniqueness)<Method>b__x_y→Method_x_y(captures full ordinal suffix for uniqueness)\w__for future-proofing2. Updated Lambda and Local Function Name Handling
^<([^>]+)>\w__(.+)matches both local functions and lambdas\winstead of hardcoding specific lettersFetchDataAsync_0_0→FetchData_0_0)3. Updated Tests
Assert.StartsWithsince ordinals varya,btofirstNumber,secondNumberto avoid false positives in schema validationAssert.NotEqualValidation Results
✅ All 346 tests passed
✅ Multiple lambdas in same method now get unique names
✅ Multiple local functions with same name in nested scopes now get unique names
✅ No regressions in existing functionality
✅ Build succeeded with 0 warnings and 0 errors (including analyzer checks)
✅ Compatible with all target frameworks including netstandard2.0 and net462
Addresses Review Feedback
Assert.StartsWithfor more robust testing with compiler-generated ordinalsFixes #6677
Original prompt
Fixes #6677
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Microsoft Reviewers: Open in CodeFlow