Skip to content

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Aug 25, 2025

Relates to test plan #75960

@333fred 333fred requested a review from a team as a code owner August 25, 2025 20:49
@333fred
Copy link
Member Author

333fred commented Aug 25, 2025

@jcouv @RikkiGibson for review

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (commit 1)

// Error if [MethodImpl(MethodImplOptions.Async)] is used directly on a method
// We give an exception to the AsyncHelpers special type, as it manually implements the pattern as part of the
// runtime's async support
if ((InternalSpecialType)appliedToSymbol.ExtendedSpecialType != InternalSpecialType.System_Runtime_CompilerServices_AsyncHelpers)
Copy link
Member

Choose a reason for hiding this comment

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

aside, and not part of your PR. but using a cast like this to go from a struct wrapper to an enum was absolutely not clear to me :D

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Signing off as my questions are not blocking and the code seems clear and in line with corresponding surrounding code.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (commit 4)

@333fred 333fred merged commit b8bf7a8 into dotnet:main Aug 29, 2025
24 checks passed
@333fred 333fred deleted the block-methodimplattribute branch August 29, 2025 23:21
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 29, 2025
@akhera99 akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants