Skip to content
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

[release/7.0] Fix handling generic custom attributes #78304

Merged
merged 1 commit into from Nov 15, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 14, 2022

Backport of #78297 to release/7.0

/cc @MichalStrehovsky

Customer Impact

It's not possible to reflection-inspect generic custom attributes that reference their T in the constructor or properties. Attempting to inspect them throws an exception.

Generic attributes are new in .NET 7 and this bug constitutes a severely broken 7.0 scenario.

Testing

Targeted testing. CI.

Risk

Low. The fix is small.

@dotnet-issue-labeler
Copy link

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

@MichalStrehovsky MichalStrehovsky added the Servicing-consider Issue for next servicing release review label Nov 14, 2022
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@carlossanlop
Copy link
Member

@MichalStrehovsky / @jeffschwMSFT I don't see a Tactics approval email. Can one of you please send one? Today's the due date for merging December release servicing fixes.

@jeffschwMSFT
Copy link
Member

adding @agocke

@msftbot
Copy link
Contributor

msftbot bot commented Nov 14, 2022

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #78297 to release/7.0

/cc @MichalStrehovsky

Customer Impact

It's not possible to reflection-inspect generic custom attributes that reference their T in the constructor or properties. Attempting to inspect them throws an exception.

Generic attributes are new in .NET 7 and this bug constitutes a severely broken 7.0 scenario.

Testing

Targeted testing. CI.

Risk

Low. The fix is small.

Author: github-actions[bot]
Assignees: -
Labels:

Servicing-consider, area-NativeAOT-coreclr

Milestone: -

@carlossanlop
Copy link
Member

This was approved via email, but Tactics is also asking if this should include tests.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 15, 2022
@carlossanlop carlossanlop added the blocked Issue/PR is blocked on something - see comments label Nov 15, 2022
@carlossanlop
Copy link
Member

Adding the blocked label until the test question gets addressed.

@radical
Copy link
Member

radical commented Nov 15, 2022

/azp run runtime-wasm-libtests

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@MichalStrehovsky
Copy link
Member

@radical this file doesn't compile into Mono at all, let's save electricity and environment of unnecessary runs.

@radical
Copy link
Member

radical commented Nov 15, 2022

@radical this file doesn't compile into Mono at all, let's save electricity and environment of unnecessary runs.

lol my mistake! 👍

@carlossanlop
Copy link
Member

Adding the blocked label until the test question gets addressed.

The question was answered via email: "the product change was validated with all generic attributes testing we have". Also, "ran the specific CoreCLR test manually and verified it's okay".

@carlossanlop carlossanlop removed the blocked Issue/PR is blocked on something - see comments label Nov 15, 2022
@carlossanlop
Copy link
Member

But I'll wait for Tactics to reply on the email before merging.
@mmitche this is the only PR left to merge into 7.0.

@mmitche
Copy link
Member

mmitche commented Nov 15, 2022

But I'll wait for Tactics to reply on the email before merging. @mmitche this is the only PR left to merge into 7.0.

Sounds good, merge when ready.

@carlossanlop
Copy link
Member

Tactics confirmed we're good to merge. Signed off by area owners. No OOB package authoring changes needed. CI failure is a timeout cancelation.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 5d71b4c into release/7.0 Nov 15, 2022
@carlossanlop carlossanlop deleted the backport/pr-78297-to-release/7.0 branch November 15, 2022 16:20
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants