Skip to content

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

Merged
carlossanlop merged 1 commit into
release/7.0from
backport/pr-78297-to-release/7.0
Nov 15, 2022
Merged

[release/7.0] Fix handling generic custom attributes#78304
carlossanlop merged 1 commit into
release/7.0from
backport/pr-78297-to-release/7.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Nov 14, 2022•
edited by MichalStrehovsky
Loading

Copy link
Copy Markdown
Contributor

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.

@ghost

ghost commented Nov 14, 2022

Copy link
Copy Markdown

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.

@build-analysisbuild-analysisBot mentioned this pull request Nov 14, 2022
Closed
@MichalStrehovskyMichalStrehovsky added the Servicing-consider Issue for next servicing release review label Nov 14, 2022
jeffschwMSFT
jeffschwMSFT approved these changes Nov 14, 2022
View reviewed changes

@jeffschwMSFTjeffschwMSFT left a comment

Copy link
Copy Markdown
Member

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

carlossanlop commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

@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

jeffschwMSFT commented Nov 14, 2022

Copy link
Copy Markdown
Member

adding @agocke

@ghost

ghost commented Nov 14, 2022

Copy link
Copy Markdown

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

carlossanlop commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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

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

carlossanlop commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

Adding the blocked label until the test question gets addressed.

@radical

radical commented Nov 15, 2022

Copy link
Copy Markdown
Member

/azp run runtime-wasm-libtests

@azure-pipelines

azure-pipelinesBot commented Nov 15, 2022

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

@MichalStrehovsky

MichalStrehovsky commented Nov 15, 2022

Copy link
Copy Markdown
Member

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

@radical

radical commented Nov 15, 2022

Copy link
Copy Markdown
Member

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

lol my mistake! 👍

@carlossanlop

carlossanlop commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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".

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

carlossanlop commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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

mmitche commented Nov 15, 2022

Copy link
Copy Markdown
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.

Sounds good, merge when ready.

@carlossanlop

carlossanlop commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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.0Nov 15, 2022
@carlossanlop
carlossanlop deleted the backport/pr-78297-to-release/7.0 branch November 15, 2022 16:20
@ghostghost locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
@jeffschwMSFTjeffschwMSFTjeffschwMSFT approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@MichalStrehovskyMichalStrehovsky

Labels

area-NativeAOT-coreclrServicing-approvedApproved for servicing release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@carlossanlop@jeffschwMSFT@radical@MichalStrehovsky@mmitche@jkotas@teo-tsirpanis