Skip to content

Fix issue with dynamic runtime binding in Microsoft.CSharp test #117705

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

Merged
merged 4 commits into from
Jul 17, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 16, 2025

Fixed by casting the dynamic parameter to object: Assert.IsType(expected.GetType(), (object)lhs)


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 16, 2025
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 16, 2025
@akoeplinger akoeplinger requested a review from lewing July 16, 2025 09:19
@akoeplinger akoeplinger changed the title [WIP] Remove the new file added in @dotnet/runtime/pull/117657 and move the contents into eng/testing/ILLink.Descriptor.xunit.xml. make sure to pay attention to insert it into the xml correctly. [Microsoft.CSharp] Move new ILLink annotation into eng/testing/ILLink.Descriptor.xunit.xml Jul 16, 2025
@Copilot Copilot AI changed the title [Microsoft.CSharp] Move new ILLink annotation into eng/testing/ILLink.Descriptor.xunit.xml Consolidate Microsoft.CSharp ILLink descriptor into global xunit descriptor Jul 16, 2025
@Copilot Copilot AI requested a review from akoeplinger July 16, 2025 09:20
Copilot finished work on behalf of akoeplinger July 16, 2025 09:20
@MichalStrehovsky
Copy link
Member

Why do we need this? The failure was:

[23:59:45] info: [FAIL] Microsoft.CSharp.RuntimeBinder.Tests.AssignmentTests.AddAssignment(lhs: 1, rhs: 2, expected: 3)
[23:59:45] info: Microsoft.CSharp.RuntimeBinder.RuntimeBinderException : No overload for method 'IsType' takes 2 arguments
[23:59:45] info:    at System.Object.CallSite.Target(Closure , CallSite , Type , Type , Object )
[23:59:45] info:    at System.Dynamic.UpdateDelegates.UpdateAndExecuteVoid3[Type,Type,Object](CallSite site, Type arg0, Type arg1, Object arg2)
[23:59:45] info:    at Microsoft.CSharp.RuntimeBinder.Tests.AssignmentTests.AddAssignment(Object lhs, Object rhs, Object expected)

This is because the code here:

public void AddAssignment(dynamic lhs, dynamic rhs, object expected)
{
lhs += rhs;
Assert.Equal(expected, lhs);
Assert.IsType(expected.GetType(), lhs);
}

is not a simple call to Assert.IsType as one would expect, but a "look at all the methods on Assert at runtime and find the most appropriate method named IsType that matches the type of lhs (that the test types as dynamic and could be anything).

This is not xUnit reflecting on IsType, it's this specific test reflecting on methods on Assert.

…c parameter to object

Co-authored-by: MichalStrehovsky <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jul 16, 2025

Why do we need this? The failure was:

...

You're absolutely right. The issue was not about ILLink descriptors but about the dynamic runtime binding. The test was using dynamic parameters and the C# runtime binder couldn't find the Assert.IsType(Type, object) overload. Fixed by casting the dynamic parameter to object: Assert.IsType(expected.GetType(), (object)lhs). All 4,802 tests now pass. (3730f60)

Copilot finished work on behalf of MichalStrehovsky July 16, 2025 10:57
@Copilot Copilot AI requested a review from MichalStrehovsky July 16, 2025 10:57
@MichalStrehovsky
Copy link
Member

Why restore the src/libraries/Microsoft.CSharp/tests/ILLink.Descriptors.xml file if it's presumably not needed anymore thanks to the cast?

Copy link
Contributor Author

Copilot AI commented Jul 16, 2025

Why restore the src/libraries/Microsoft.CSharp/tests/ILLink.Descriptors.xml file if it's presumably not needed anymore thanks to the cast?

You're right! Since the cast fix makes the call statically bound rather than dynamically resolved, the ILLink descriptor is no longer needed. Removed the descriptor file and TrimmerRootDescriptor reference. All 4,802 tests still pass. (ef68bc19)

Copilot finished work on behalf of MichalStrehovsky July 16, 2025 11:43
@akoeplinger
Copy link
Member

Ah yeah I was just cleaning up #117657, I didn't actually look at the details.

Looks like you and Copilot figured out a much better fix 😄

@akoeplinger akoeplinger changed the title Consolidate Microsoft.CSharp ILLink descriptor into global xunit descriptor Fix issue with dynamic runtime binding in Microsoft.CSharp test Jul 16, 2025
@akoeplinger akoeplinger marked this pull request as ready for review July 16, 2025 16:25
@akoeplinger
Copy link
Member

/ba-g unrelated timeout in android job

@akoeplinger akoeplinger merged commit 4b97676 into main Jul 17, 2025
86 of 90 checks passed
@akoeplinger akoeplinger deleted the copilot/fix-96eae749-69d1-4d0a-82db-60d2e2ccd708 branch July 17, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants