-
Notifications
You must be signed in to change notification settings - Fork 5k
Resolve ILLink warnings in System.Text.RegularExpressions #46687
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
Conversation
* Add DynamicallyAccessedMembers All to TypeBuilder and EnumBuilder CreateType methods. Contributes to dotnet#45623
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@@ -15,6 +15,9 @@ namespace System.Text.RegularExpressions | |||
/// RegexCompiler translates a block of RegexCode to MSIL, and creates a | |||
/// subclass of the RegexRunner type. | |||
/// </summary> | |||
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2060:MakeGenericMethod", | |||
Target = "M:System.Text.RegularExpressions.RegexCompiler.#cctor", | |||
Justification = "The referenced methods don't have any DynamicallyAccessedMembers annotations. See https://github.com/mono/linker/issues/1727")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, are all of the patterns employed below recognized by the linker? i.e. it's able to see it must preserve the IsBoundary method, the runtrack field, the char.IsDigit method, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ILLinker isn't warning for these methods:
private static FieldInfo RegexRunnerField(string fieldname) => typeof(RegexRunner).GetField(fieldname, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static)!;
private static MethodInfo RegexRunnerMethod(string methname) => typeof(RegexRunner).GetMethod(methname, BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static)!;
My understanding is because it sees the specified typeof(RegexRunner)
and thinks "I must preserve all Methods and Fields on the RegexRunner Type, since these methods try getting fields and methods on this Type".
But @vitek-karas and @MichalStrehovsky would know for sure if the above reasoning is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can validate by debugging the linker if needed, but @eerhardt is right - the call to GetField
will fallback to basically mark all fields which fulfill the binding flags, if it can't figure out the name of the field statically (like in this case). Same goes for GetMethod
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info.
@@ -15,6 +15,9 @@ namespace System.Text.RegularExpressions | |||
/// RegexCompiler translates a block of RegexCode to MSIL, and creates a | |||
/// subclass of the RegexRunner type. | |||
/// </summary> | |||
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2060:MakeGenericMethod", | |||
Target = "M:System.Text.RegularExpressions.RegexCompiler.#cctor", | |||
Justification = "The referenced methods don't have any DynamicallyAccessedMembers annotations. See https://github.com/mono/linker/issues/1727")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can validate by debugging the linker if needed, but @eerhardt is right - the call to GetField
will fallback to basically mark all fields which fulfill the binding flags, if it can't figure out the name of the field statically (like in this case). Same goes for GetMethod
.
Failures are #32030. Merging. |
Contributes to #45623