Recent change to Attribute.GetCustomAttributes is breaking in ASP.NET Core

[SteveSandersonMS]

Description

I know it's possible that this was an intentional breaking change and perhaps the correct resolution is to change things in ASP.NET Core. However it's worth checking.

We're getting a test failure in MapEndpoint_GeneratedDelegateWorks when it runs this logic:

        // Add delegate attributes as metadata
        var attributes = requestDelegate.Method.GetCustomAttributes();


        // This can be null if the delegate is a dynamic method or compiled from an expression tree
        if (attributes != null)

The exception is System.InvalidCastException : Unable to cast object of type 'System.Object[]' to type 'System.Attribute[]'. An example failing build is here.

Note that the exception is coming from runtime code, not our test code, so this may be a real regression that would affect customers.

It looks very likely that the cause is the changes in #65237 from last week. I see that previously, if the delegate was a dynamic method or compiled from an expression tree, then GetCustomAttributes would have returned null, whereas in the newer runtime code, it throws this InvalidCastException.

Is that an intentional change? It seems unlikely because:

1. It forces user code to use try/catch since there's no TryGetCustomAttributes
2. Surfacing it as an InvalidCastException really seems to be an internal implementation detail of the runtime. I'd expect a more meaningful exception with a message saying why it's illegal to ask about custom attributes for this specific method.

If we have to change ASP.NET Core to try/catch around this call then technically we can do so, but perhaps other customer code would also be broken by this change, and the use of try/catch for control flow here might be problematic for perf.

Reproduction Steps

See the failing build at https://dev.azure.com/dnceng/public/_build/results?buildId=1657756&view=ms.vss-test-web.build-test-results-tab&runId=45670040&resultId=102702&paneView=debug

Expected behavior

Retain existing behavior of returning null. Or if you don't want to return null, then return an empty array.

Actual behavior

Throws InvalidCastException

Regression?

Yes, this was not happening in the last runtime build ingested by ASP.NET Core.

Known Workarounds

No response

Configuration

No response

Other information

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

I know it's possible that this was an intentional breaking change and perhaps the correct resolution is to change things in ASP.NET Core. However it's worth checking.

We're getting a test failure in MapEndpoint_GeneratedDelegateWorks when it runs this logic:

        // Add delegate attributes as metadata
        var attributes = requestDelegate.Method.GetCustomAttributes();


        // This can be null if the delegate is a dynamic method or compiled from an expression tree
        if (attributes != null)

The exception is System.InvalidCastException : Unable to cast object of type 'System.Object[]' to type 'System.Attribute[]'. An example failing build is here.

Note that the exception is coming from runtime code, not our test code, so this may be a real regression that would affect customers.

It looks very likely that the cause is the changes in #65237 from last week. I see that previously, if the delegate was a dynamic method or compiled from an expression tree, then GetCustomAttributes would have returned null, whereas in the newer runtime code, it throws this InvalidCastException.

Is that an intentional change? It seems unlikely because:

1. It forces user code to use try/catch since there's no TryGetCustomAttributes
2. Surfacing it as an InvalidCastException really seems to be an internal implementation detail of the runtime. I'd expect a more meaningful exception with a message saying why it's illegal to ask about custom attributes for this specific method.

If we have to change ASP.NET Core to try/catch around this call then technically we can do so, but perhaps other customer code would also be broken by this change, and the use of try/catch for control flow here might be problematic for perf.

Reproduction Steps

See the failing build at https://dev.azure.com/dnceng/public/_build/results?buildId=1657756&view=ms.vss-test-web.build-test-results-tab&runId=45670040&resultId=102702&paneView=debug

Expected behavior

Retain existing behavior of returning null. Or if you don't want to return null, then return an empty array.

Actual behavior

Throws InvalidCastException

Regression?

Yes, this was not happening in the last runtime build ingested by ASP.NET Core.

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	SteveSandersonMS
Assignees:	-
Labels:	area-System.Reflection, untriaged
Milestone:	-

[stephentoub]

cc: @jkotas

[madelson]

Probably DynamicMethod needs similar updates to the ones made in my PR (and test coverage for this).

[jkotas]

We should revert the original change until this is understood and fixed: #66508

[buyaa-n]

// This can be null if the delegate is a dynamic method or compiled from an expression tree
...
Retain existing behavior of returning null. Or if you don't want to return null, then return an empty array.

The API is not supposed to return null, i believe we should return and empty array in this case

[buyaa-n]

@SteveSandersonMS this is fixed in preview 5, could you check, you might need to update your code as it is now return empty array instead of null

[ghost]

This issue has been marked needs-author-action and may be missing some important information.

[ghost]

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[ghost]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.