Skip to content

[mono] Fix support for generic custom attributes. - #78091

Merged
vargaz merged 2 commits into
dotnet:mainfrom
vargaz:generic-cattr
Nov 17, 2022
Merged

[mono] Fix support for generic custom attributes.#78091
vargaz merged 2 commits into
dotnet:mainfrom
vargaz:generic-cattr

Conversation

@vargaz

Copy link
Copy Markdown
Contributor

Fixes#77047.

@MichalStrehovsky

Copy link
Copy Markdown
Member

Do we need a regression test? It would contribute to #73735.

@vargaz

Copy link
Copy Markdown
ContributorAuthor

The test seems to fail on nativeaot.

@MichalStrehovsky

Copy link
Copy Markdown
Member

The test seems to fail on nativeaot.

Not completely surprised. This used to be a compiler crash until very late in RC1 when System.Reflection.Metadata finally fixed a bug around this leaving no runway in 7.0 to look at this. I added an ActiveIssue.

@am11

am11 commented Nov 12, 2022

Copy link
Copy Markdown
Member

Can we delete

<ExcludeList Include = "$(XunitTestBinBase)/reflection/GenericAttribute/**">
as part of this PR?

@vargaz
vargazforce-pushed the generic-cattr branch 2 times, most recently from 134b49a to ddfc910CompareNovember 13, 2022 19:08
@vargaz

Copy link
Copy Markdown
ContributorAuthor

Removed the added test and reenabled the existing ones.

@vargaz

Copy link
Copy Markdown
ContributorAuthor

Failure is
#78290

@am11

am11 commented Nov 15, 2022

Copy link
Copy Markdown
Member

.targets change got lost after the rebase.

@vargaz
vargazforce-pushed the generic-cattr branch 2 times, most recently from a0cd7e1 to 12eff9aCompareNovember 15, 2022 16:09
@runfoapprunfoappBot mentioned this pull request Nov 15, 2022

@am11am11 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.

Thanks. It also fixes #56887. 👍

@lewing

Copy link
Copy Markdown
Member

should we backport this?

@vargaz

Copy link
Copy Markdown
ContributorAuthor

Its low risk.

@marek-safar

Copy link
Copy Markdown
Contributor

If it's low risk then we should.

@vargaz

Copy link
Copy Markdown
ContributorAuthor

It did seems to cause a small number of perf regressions:
#78821

@ghostghost locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blazor: Generic attributes do not work in Mono

5 participants

@vargaz@MichalStrehovsky@am11@lewing@marek-safar