Resolving ILLink warnings on System.Private.Xml (Part 1) #49413
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,15 +14,15 @@ namespace System.Xml.Xsl.Xslt | |
| internal class Scripts | ||
| { | ||
| private readonly Compiler _compiler; | ||
| private readonly TrimSafeDictionary _nsToType = new TrimSafeDictionary(); | ||
| private readonly XmlExtensionFunctionTable _extFuncs = new XmlExtensionFunctionTable(); | ||
|
|
||
| public Scripts(Compiler compiler) | ||
| { | ||
| _compiler = compiler; | ||
| } | ||
|
|
||
| public TrimSafeDictionary ScriptClasses | ||
| { | ||
| get { return _nsToType; } | ||
| } | ||
|
|
@@ -44,43 +44,28 @@ public LinkerSafeDictionary ScriptClasses | |
| return null; | ||
| } | ||
|
|
||
| internal class TrimSafeDictionary | ||
| { | ||
| private readonly Dictionary<string, Type?> _backingDictionary = new Dictionary<string, Type?>(); | ||
|
|
||
| public Type? this[string key] | ||
| { | ||
| [UnconditionalSuppressMessage("TrimAnalysis", "IL2073:MissingDynamicallyAccessedMembers", | ||
| Justification = "The getter of the dictionary is not annotated to preserve the constructor, but the sources that are adding the items to " + | ||
| "the dictionary are annotated so we can supress the message as we know the constructor will be preserved.")] | ||
| [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] | ||
|
There was a problem hiding this comment. Can we just put the attribute on the whole property? There was a problem hiding this comment. Chatted offline, this is a problem specific to Indexers and have logged the following issue for investigation tracking: dotnet/linker#1902 |
||
| get => ((IDictionary<string, Type?>)_backingDictionary)[key]; | ||
| [param: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] | ||
| set => ((IDictionary<string, Type?>)_backingDictionary)[key] = value; | ||
| } | ||
|
|
||
| public ICollection<string> Keys => ((IDictionary<string, Type?>)_backingDictionary).Keys; | ||
|
|
||
| public int Count => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).Count; | ||
|
There was a problem hiding this comment. Why are all these casts of There was a problem hiding this comment. I'll fix it, not necessary, I just used the quick refactoring ctrl+'.' options in VS that added the casts: |
||
|
|
||
| public bool ContainsKey(string key) => ((IDictionary<string, Type?>)_backingDictionary).ContainsKey(key); | ||
|
|
||
| public bool TryGetValue(string key, [MaybeNullWhen(false)] out Type? value) => ((IDictionary<string, Type?>)_backingDictionary).TryGetValue(key, out value); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -371,15 +371,10 @@ internal XsltOutput Output | |
|
|
||
| _scriptExtensions = new Hashtable(_stylesheet.ScriptObjectTypes.Count); | ||
| { | ||
| // Scripts are not supported on stylesheets | ||
|
There was a problem hiding this comment. Do we need a test for this or something? Are we concerned about the behavior change? There was a problem hiding this comment. This is a functional change as far as I can tell - we should have the proper tests and get it reviewed by the area owners. There was a problem hiding this comment. Agreed, @krwq AFAICT Script objects on xsls are not supported at all in .NET Core. I tried with several different approaches trying to create an xsl that had scripts and all entrypoints that I found trhough XslCompiledTransform would all eventually throw PNSE. I think that this has been dead code that we just originally imported from .NET Framework, but never really did anything as it throws PNSE when you actually invoke the transform operation. There was a problem hiding this comment. IIRC that's correct and intentional. @joperezr you can double check by creating an xslt which works on full fx and make sure it throws on 5.0 - check for all types of scripts. |
||
| if (_stylesheet.ScriptObjectTypes.Count > 0) | ||
|
There was a problem hiding this comment. might be a good idea to also simplify parsing this to save even more bytes |
||
| { | ||
| throw new PlatformNotSupportedException(SR.CompilingScriptsNotSupported); | ||
|
There was a problem hiding this comment. Is it worth/possible to add a test here? I'm not asking for a bunch of work, but if it is reasonable (say an hour) it might be nice to ensure this scenario throws PNSE correctly. #Resolved There was a problem hiding this comment. Sure, I can add a unit test for this. This is actually addressing a warning on the old xslt which is not currently covered by this PR (except for this one) but I can add a test here that ensures that using scripts on old xslt throws PNSE at the compile stage which is earlier than this code comes into play. #Resolved |
||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Why the change here to enumerate the keys and then index into the dictionary? Why not just expose
GetEnumeratorfromTrimSafeDictionary?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 problem here is that if we keep it as it was, then we would get a warning (even when using TrimmerSafeDictionary) since the Type would be comming form method:
System.Collections.Generic.KeyValuePair<Tkey, TValue>.getwhich wouldn't be annotated. Of course we could suppress that, but by doing it the way I'm doing it here, the value gets accessed via the index operator on TrimmerSafeDictionary, so that way the linker will not throw a warning and we won't need an additional suppression.