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] Add missing DynamicallyAccessMembers attributes to System.Data.Common/ref #75980

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 21, 2022

Backport of #75961 to release/7.0

/cc @ViktorHofer

Customer Impact

Having these attributes on the contract when they are present in the implementation is important. To quote @eerhardt:

It is important because we have a Roslyn analyzer that runs when you set PublishTrimmed=true or PublishAot=true in your app (or EnableTrimAnalyzer=true in your library). So you get build-time analysis of your reflection usages to know if it is trim-compatible or not. See https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#enable-project-specific-trimming

Testing

Manually verified that the missing attributes are part of the contract assembly. Automation to prevent this divergence will be added with #73263.

Risk

Low. The attributes were missing before and are now part of the assembly. We'll now introduce the correct warnings for these APIs when projects opt-into the analyzer.

The existing APICompat tooling doesn't indicate a compatibility error when comparing the contract assembly against the implementation assembly and the `[return: ...]` attributes aren't in sync. The new tooling that is being bootstrapped in dotnet/runtime via #73263 does flag them.

In this case a few type members were missing the `DynamicallyAccessedMembers` return attribute in the contract.
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Sep 21, 2022

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

Issue Details

Backport of #75961 to release/7.0

/cc @ViktorHofer

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

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

area-System.Data, new-api-needs-documentation

Milestone: -

@ViktorHofer
Copy link
Member

@ericstj @danmoseley unsure if this an ask-mode or tell-mode change. Happy to send it to Tactics if necessary.

@ericstj
Copy link
Member

ericstj commented Sep 21, 2022

Since it touches the shipping reference assembly let's run it through ask mode.

@ericstj ericstj added the Servicing-consider Issue for next servicing release review label Sep 22, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 22, 2022
@rbhanda rbhanda added this to the 7.0.0 milestone Sep 22, 2022
@ViktorHofer ViktorHofer merged commit 264b675 into release/7.0 Sep 23, 2022
@ViktorHofer ViktorHofer deleted the backport/pr-75961-to-release/7.0 branch September 23, 2022 05:46
@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2022
Sign up for free to 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.

3 participants