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

Replace DAMT.All with more restricted annotation on InvokeMember/FindMembers/DeclaredMembers #109801

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

MichalStrehovsky
Copy link
Member

I'm looking at places we use .All to see if the new annotations could help. Here we don't need the new annotations, this was always expressible. I can only assume the reason for .All was laziness. This was at a time when we believed .All "just keeps a bit more" and we didn't consider the impact of marking interface methods implemented by the class, or the impact of warnings due to .All capturing things that are not safe to reflection-call.

This is a breaking change in theory, should someone implement IReflect or derive from System.TypeInfo - they need to update the annotations.

Cc @dotnet/illink @eerhardt

@MichalStrehovsky MichalStrehovsky added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 13, 2024
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, 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.

1 similar comment
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, 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.

@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 13, 2024
@MichalStrehovsky MichalStrehovsky added the linkable-framework Issues associated with delivering a linker friendly framework label Nov 13, 2024
Copy link
Contributor

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky
Copy link
Member Author

@dotnet/illink @eerhardt anyone could have a look please?

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

LGTM. I think the breaking change is acceptable - should be rare for anyone to hit this, and if they do, fixing the annotations to match this would be an improvement.

@MichalStrehovsky
Copy link
Member Author

/ba-g deadlettered test

@MichalStrehovsky MichalStrehovsky merged commit ffb0bff into dotnet:main Nov 21, 2024
147 of 151 checks passed
@MichalStrehovsky MichalStrehovsky deleted the declared branch November 21, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. linkable-framework Issues associated with delivering a linker friendly framework needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants