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.

Sign up for GitHub

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

Jump to bottom

Suppressing RequiresDynamicCodeAttribute for Compiled Regex #67299

Closed
wants to merge 3 commits into from
Closed

Suppressing RequiresDynamicCodeAttribute for Compiled Regex #67299

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a26195e
Suppressing RequiresDynamicCodeAttribute for Compiled Regex
LakshanF Mar 29, 2022
6037814
Incorporating FB
LakshanF Mar 29, 2022
32a53dd
Squashed commit of the following:
LakshanF Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ystem.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresDynamicCodeAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace System.Diagnostics.CodeAnalysis
/// <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)]
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)]
public sealed class RequiresDynamicCodeAttribute : Attribute
{
/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7458,7 +7458,7 @@ public RequiresAssemblyFilesAttribute(string message) { }
public string? Message { get { throw null; } }
public string? Url { get { throw null; } set { } }
}
[System.AttributeUsageAttribute(System.AttributeTargets.Constructor | System.AttributeTargets.Method, Inherited=false)]
[System.AttributeUsageAttribute(System.AttributeTargets.Constructor | System.AttributeTargets.Method | AttributeTargets.Class, Inherited=false)]
public sealed partial class RequiresDynamicCodeAttribute : System.Attribute
{
public RequiresDynamicCodeAttribute(string message) { }
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ internal Regex(string pattern, CultureInfo? culture)
// if no options are ever used.
}

[UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode",
Justification = "Native AOT does not use compiled Regex")]
internal Regex(string pattern, RegexOptions options, TimeSpan matchTimeout, CultureInfo? culture)
{
// Validate arguments.
Expand Down Expand Up @@ -201,6 +203,7 @@ protected IDictionary? CapNames
/// Regex constructor, we don't load RegexCompiler and its reflection classes when
/// instantiating a non-compiled regex.
/// </summary>
[RequiresDynamicCode("The native code for Regex compilation might not be available at runtime.")]
[MethodImpl(MethodImplOptions.NoInlining)]
private static RegexRunnerFactory? Compile(string pattern, RegexTree regexTree, RegexOptions options, bool hasTimeout) =>
RegexCompiler.Compile(pattern, regexTree, options, hasTimeout);
Expand Down
1 change: 1 addition & 0 deletions ...raries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace System.Text.RegularExpressions
/// <summary>
/// RegexCompiler translates a block of RegexCode to MSIL, and creates a subclass of the RegexRunner type.
/// </summary>
[RequiresDynamicCode("The native code for Regex compilation might not be available at runtime.")]
Copy link
Member

Choose a reason for hiding this comment

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

Before we can place it on a class we need to:

  • Get the updated attribute through an API review
  • Make sure the Roslyn analyzer can actually support this. E.g. RequiresAssemblyFiles cannot be placed on a class, but it can be placed on a property. We're currently not very consistent on the applicability of these. Maybe we don't need to be? (Allowing RequiresUnreferencedCode/RequiresDynamicCode on properties is going to be bunch of work, for example)
  • Make sure the NativeAOT compiler supports this.

If this pull request takes a dependency on this, it will be blocked until all of the above is validated/happens.

Copy link
Member

Choose a reason for hiding this comment

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

I think the main value of this PR is that we are dogfooding the system we are putting in place and finding all the holes before customers will find them.

no public APIs are going to get these annotations. So really this is just for our internal code.

The alternatives to waiting would be:

  1. Add attributes to most (all?) the methods in this class until we can support the class level attribute.

2.Suppress the warnings at the low level places like the original suggestion.

I could live with 1 for a while as long as we ensured we supported class level in .NET 7. We would also use that in JSON and DependencyInjection, since both of those libraries have whole classes just for generating IL.

Copy link
Member

Choose a reason for hiding this comment

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

I like 1 - unless @LakshanF has a different opinion, let's create an issue tracking the 3 things above (API review, validate/implement Roslyn analyzer/add a test for it, validate/implement in NativeAOT and add a test for it, and undo workaround from this pull request). For this pull request, add RDC on the individual methods instead of on the class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Option 1 as mentioned above would require some churn - the warnings come from static read only fields that would require a longer workarounds that will need to be spilled into methods in the class - that we expect to be temporary anyway.

I like @eerhardt thinking of this PR as flushing out initial issues with the annotation process and created an API request for enhancing the attribute. I would like to explore that path (including compiler changes ) in more detail first since its the solution we eventually likely go with before going for the temporary long workaround with option 1.

internal abstract class RegexCompiler
{
private static readonly FieldInfo s_runtextstartField = RegexRunnerField("runtextstart");
Expand Down
3 changes: 3 additions & 0 deletions ...es/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexLWCGCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Threading;
using System.Reflection;
using System.Reflection.Emit;
using System.Diagnostics.CodeAnalysis;

namespace System.Text.RegularExpressions
{
Expand All @@ -30,6 +31,7 @@ internal sealed class RegexLWCGCompiler : RegexCompiler
private static int s_regexCount;

/// <summary>The top-level driver. Initializes everything then calls the Generate* methods.</summary>
[RequiresDynamicCode("The native code for Regex compilation might not be available at runtime.")]
public RegexRunnerFactory? FactoryInstanceFromCode(string pattern, RegexTree regexTree, RegexOptions options, bool hasTimeout)
{
if (!regexTree.Root.SupportsCompilation(out _))
Expand Down Expand Up @@ -65,6 +67,7 @@ internal sealed class RegexLWCGCompiler : RegexCompiler
}

/// <summary>Begins the definition of a new method (no args) with a specified return value.</summary>
[RequiresDynamicCode("The native code for Regex compilation might not be available at runtime.")]
private DynamicMethod DefineDynamicMethod(string methname, Type? returntype, Type hostType, Type[] paramTypes)
{
// We're claiming that these are static methods, but really they are instance methods.
Expand Down