Type hierarchy merge #2176
Closed
Type hierarchy merge #2176
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Any access to DAM annotated method must be validated by the trimmer. Normal calls and such are handled already, but direct reflection access or other indirect accesses can lead to trimmer allowing potential invocation of DAM annotated method without validating the annotations. This change will warn whenever such potential "leak" happens. This applies to method parameter, method return value and field annotations. Uses the same infra as RUC already just adds additional checks. Tests for the new cases.
Annotated return value is mostly not problematic (the caller only "Reads" from it, which is always safe). The only case where it's problematic is if something would override the method at runtime (ref emit) in which case the trimmer can't validate the new code that it fulfills the return value requirements. But that can only happen if the method is virtual.
Messages for these warnings can not be suppressed at the member level due to issues described in dotnet#2163. Also fix expected warnings for properties, revert ResultChecker changes, and remove redundant test.
- Include "Attribute" in attribute type names - Add comments about why we want to report warnings - Add comment about the interaction with RUC on types
- Clean up some test attributes - Add test with DAM field on annotated PublicMethods type - Add link to issue about duplicate warnings Also use replace DAMT with DAM in tests.
- Don't warn on non-virtual methods that don't have return annotations - Use the helpers introduced in dotnet#2145
vitek-karas
reviewed
Jul 28, 2021
test/Mono.Linker.Tests.Cases/Reflection/TypeHierarchyReflectionWarnings.cs
Outdated
Show resolved
Hide resolved
Clarify that getter without annotated return value doesn't warn because it's non-virtual
|
@vitek-karas here are the updated runtime suppressions: https://github.com/dotnet/runtime/compare/main...sbomer:updateSuppressionsForReflectionAccess?expand=1. I took a few of them straight from your changes in https://github.com/dotnet/runtime/compare/main...vitek-karas:FixReflectionOnDAMImpact?expand=1 with updated warning codes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This contains the changes from #2145 and #2162. PTAL at the last commit @vitek-karas. Once we have the runtime changes ready I would suggest we merge your change in, then I will rebase my original PR with this change included and we can check it in immediately (that way the repo commit history will be cleaner since the changes are logically separate).