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

Support Requires on type in NativeAot #68978

Merged
merged 7 commits into from
May 13, 2022

Conversation

tlakollo
Copy link
Contributor

@tlakollo tlakollo commented May 6, 2022

Support Requires on type in NativeAot

  • Adds new overload for MessageOrigin creation that uses MethodIL and an offset as parameters. Eventually, this will be used to produce warnings
  • Share methods used in linker/analyzer for asking questions about members being in RequiresScope, members Requiring and Requires warning generation. These methods include handling of Requires on a type.
  • The ReflectionMethodBodyScanner on-field access now verifies for requires on type, since implicitly can call the static constructor
  • Updated the mismatch attribute rules to take into account Requires on type

- Adds new overload for MessageOrigin creation that uses MethodIL and an offset as parameters, eventually this will be used to produce warnings
- Share methods used in linker/analyzer for asking questions about members being in RequiresScope, members Requiring and Requires warning generation. These methods include handling of Requires on a type.
- The ReflectionMethodBodyScanner on field access now verifies for requires on type, since implicitly can call the static constructor
- Updated the mismatch attribute rules to take into account Requires on type
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Looks good

/// <remarks>Unlike <see cref="DoesMethodRequires(MethodDesc, string, out CustomAttributeValue?)"/>
/// if a declaring type has Requires, all methods in that type are considered "in scope" of that Requires. So this includes also
/// instance methods (not just statics and .ctors).</remarks>
internal static bool IsMethodInRequiresScope(this MethodDesc method, string requiresAttribute)
Copy link
Member

Choose a reason for hiding this comment

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

I would call this IsInRequiresScope - that's the name we use in the analyzer - so it will be easier to lookup the matching functionality there. (The body does match the behavior, so the name might as well)

Copy link
Member

Choose a reason for hiding this comment

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

I see the linker calls this IsMethodInRequiresUnreferencedCodeScope...
In that case I don't mind the current name, but we should then rename the one in the analyzer to match.
And please add a comment which methods this is a counterpart for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened dotnet/linker#2788 to track the changes between linker/analyzer

@tlakollo
Copy link
Contributor Author

tlakollo commented May 9, 2022

In an offline discussion, we discussed that his PR should also take into account that PublishAot implies that the app will be trimmed and single file. Meaning that NativeAOT compiler should produce warnings about single file too. Also, if PublishAot is set that also means that EnableAotAnalyzer, EnableTrimAnalyzer and EnableSingleFileAnalyzer should be set too. See dotnet/sdk#25301

Added support for RequiresAssemblyFilesAttribute
 - Now the code checks for RAF and warns about it
 - Added support to check and retrieve attributes in a property
 - Added overloads to IsInRequiresScope that verify associated properties
Added new constant values for the different requires attributes
@tlakollo tlakollo merged commit 88fb966 into dotnet:main May 13, 2022
@tlakollo tlakollo deleted the SupportForRequiresOnClass branch May 13, 2022 05:25
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 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.

2 participants