Annotating Regex library for Native Aot apps #67841
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsThe
|
| @@ -201,6 +203,7 @@ protected internal static void ValidateMatchTimeout(TimeSpan matchTimeout) | |||
| /// 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.")] | |||
There was a problem hiding this comment.
Why is this one needed? It's only called from the above internal Regex constructor that's attributed as unconditional suppression. The analyzer doesn't then just ignore everything below that point?
(Same question for all the annotations after this one)
There was a problem hiding this comment.
The analyzer doesn't then just ignore everything below that point?
No, the analyzer doesn't do "cross-method" analysis. It just analyzes the current method.
In a perfect world, the UnconditionalSuppressMessage attribute on the internal Regex constructor shouldn't even be needed because it only calls this method behind a check for RuntimeFeature.IsDynamicCodeCompiled. The analyzer should be able to tell that the method that "RequiresDynamicCode" doesn't ever get invoked when dynamic code isn't available.
There was a problem hiding this comment.
In a perfect world, the UnconditionalSuppressMessage attribute on the internal Regex constructor shouldn't even be needed because it only calls this method behind a check for RuntimeFeature.IsDynamicCodeCompiled. The analyzer should be able to tell that the method that "RequiresDynamicCode" doesn't ever get invoked when dynamic code isn't available.
Right, we talked about that on the other PRs/issues. I was asking here about, short of recognizing IsDynamicCodeCompiled checks in that manner, it could do the same for UnconditionalSuppressMessage. Both would be valuable, and presumably both require the same level of cross-method analysis (recursively suppressing diagnostics on methods only callable from call sites suppressed via either a guard check or attribute).
There was a problem hiding this comment.
and presumably both require the same level of cross-method analysis
The check I'm taking about shouldn't require the same level of cross-method analysis. We would still annotate the Compile method as RequiresDynamicCode. The analyzer wouldn't need any more cross-method analysis than it does today - it just looks at the method being called and looks at its attributes. This would be the same behavior as the SupportedOSPlatform analyzer we have today - it doesn't warn about code inside if (OperatingSystem.IsWindows()) that calls windows-only code.
There was a problem hiding this comment.
We would still annotate the Compile method as RequiresDynamicCode
And we don't want to auto-propagate annotations up the call chain because... it's too expensive for the analyzer?
We're pushing onto developers work that the tool could do, it seems we're just choosing not to (presumably for a good reason, like it would be too slow).
There was a problem hiding this comment.
I also don't think it's a good UX
That may be. I think the same thing about having to add a bunch of attributes to suppress warnings that, from my perspective, are bogus.
There was a problem hiding this comment.
Also, the point of the requires attributes is also to tell developers -- these methods have special requirements. It's an API demand, so it should be visible as part of the API of the method.
There was a problem hiding this comment.
It's an API demand, so it should be visible as part of the API of the method.
For exposed API, sure. None of the ones in this PR are.
There was a problem hiding this comment.
I opened an issue to track this, dotnet/linker#2740
There was a problem hiding this comment.
Propagating these across methods is problematic because e.g. these annotations need to be consistent across virtuals (if base is not annotated, override cannot be annotated)
It's hard to imagine a good UX around situations when the propagation stops working at some point in the static callgraph. There can be big ripple effects. Also scenarios like adding InternalsVisibleTo to the assembly popping up warnings, etc.
...aries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexLWCGCompiler.cs
Outdated
Show resolved
Hide resolved
...libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.cs
Outdated
Show resolved
Hide resolved
…egularExpressions/Regex.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
The
RequiresDynamicCodeattribute target scope change got approved and creating a new PR with this change for Regex library annotation