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

Add RequiresDynamicCodeAttribute #61239

Closed
MichalStrehovsky opened this issue Nov 5, 2021 · 6 comments
Closed

Add RequiresDynamicCodeAttribute #61239

MichalStrehovsky opened this issue Nov 5, 2021 · 6 comments
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-NativeAOT-coreclr
Milestone

Comments

@MichalStrehovsky
Copy link
Member

Background and motivation

.NET comes with APIs that are fundamentally unsupportable without some sort of ability to run dynamic code. We already offer ability to not enable dynamic code support in some of our existing AOT form factors (Xamarin iOS without MtouchInterpreter enabled), and we have other runtime form factors on the horizon (#61231) without dynamic code support by default.

Customers currently don't have a way to tell whether a dynamic-code-less deployment option is a good fit for their app. They need to try and thoroughly test.

We need to introduce API annotations for AOT form factor. These should mirror the annotations introduced for single file and trimming in the past.

API Proposal

namespace System.Diagnostics.CodeAnalysis
{
    /// <summary>
    /// Indicates that the specified method requires the ability to generate new code at runtime,
    /// for example through <see cref="System.Reflection"/>.
    /// </summary>
    /// <remarks>
    /// This allows tools to understand which methods are unsafe to call when compiling ahead of time.
    /// </remarks>
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)]
    public sealed class RequiresDynamicCodeAttribute : Attribute
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="RequiresDynamicCodeAttribute"/> class
        /// with the specified message.
        /// </summary>
        /// <param name="message">
        /// A message that contains information about the usage of dynamic code.
        /// </param>
        public RequiresDynamicCodeAttribute(string message)
        {
            Message = message;
        }

        /// <summary>
        /// Gets a message that contains information about the usage of dynamic code.
        /// </summary>
        public string Message { get; }

        /// <summary>
        /// Gets or sets an optional URL that contains more information about the method,
        /// why it requries dynamic code, and what options a consumer has to deal with it.
        /// </summary>
        public string? Url { get; set; }
    }
}

API Usage

Assume DynamicMethod constructor was annotated with RequiresDynamicCode:

        DynamicMethod squareIt = new DynamicMethod(
            "SquareIt",
            typeof(long),
            methodArgs,
            typeof(Example).Module);

Appropriate tooling (Roslyn analyzer, AOT compiler) would generate a warnings that DynamicMethod requires dynamic code. The user would be able to either:

  • Mark the method containing the callsite as RequiresDynamicCode.
  • Or, place the reference under a RuntimeFeature.IsDynamicCodeSupported check
  • Or, suppress the warning

The expectation is to mostly mirror RequiresAssemblyFilesAttribute/RequiresUnreferencedCodeAttribute.

Alternative Designs

No response

Risks

No response

@MichalStrehovsky MichalStrehovsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation area-NativeAOT-coreclr labels Nov 5, 2021
@MichalStrehovsky MichalStrehovsky added this to the 7.0.0 milestone Nov 5, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 5, 2021
@MichalStrehovsky MichalStrehovsky removed the untriaged New issue has not been triaged by the area owner label Nov 5, 2021
@davidfowl
Copy link
Member

Very nice!

@agocke
Copy link
Member

agocke commented Nov 5, 2021

Nit: "dynamic code" is a little vague, maybe RequiresRunTimeCodeGen?

@MichalStrehovsky
Copy link
Member Author

Nit: "dynamic code" is a little vague, maybe RequiresRunTimeCodeGen?

I chose this name as a complement to the existing RuntimeFeature.IsDynamicCodeSupported API, but I'm not strongly attached to it.

@marek-safar
Copy link
Contributor

I think proposals like this one should come together with analyzer proposal.

@bartonjs
Copy link
Member

bartonjs commented Nov 16, 2021

Video

Looks good as proposed.

namespace System.Diagnostics.CodeAnalysis
{
    /// <summary>
    /// Indicates that the specified method requires the ability to generate new code at runtime,
    /// for example through <see cref="System.Reflection"/>.
    /// </summary>
    /// <remarks>
    /// This allows tools to understand which methods are unsafe to call when compiling ahead of time.
    /// </remarks>
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)]
    public sealed class RequiresDynamicCodeAttribute : Attribute
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="RequiresDynamicCodeAttribute"/> class
        /// with the specified message.
        /// </summary>
        /// <param name="message">
        /// A message that contains information about the usage of dynamic code.
        /// </param>
        public RequiresDynamicCodeAttribute(string message)
        {
            Message = message;
        }

        /// <summary>
        /// Gets a message that contains information about the usage of dynamic code.
        /// </summary>
        public string Message { get; }

        /// <summary>
        /// Gets or sets an optional URL that contains more information about the method,
        /// why it requries dynamic code, and what options a consumer has to deal with it.
        /// </summary>
        public string? Url { get; set; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 16, 2021
@tlakollo
Copy link
Contributor

Closing via #61956

@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-NativeAOT-coreclr
Projects
None yet
Development

No branches or pull requests

6 participants