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 |
|---|---|---|
|
|
@@ -4,23 +4,25 @@ | |
| // <spec>http://devdiv/Documents/Whidbey/CLR/CurrentSpecs/BCL/CodeDom%20Activation.doc</spec> | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Xml.Xsl.Runtime; | ||
|
|
||
| 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; } | ||
| } | ||
|
|
@@ -41,5 +43,29 @@ public Scripts(Compiler compiler) | |
| } | ||
| 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 => _backingDictionary[key]; | ||
| [param: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] | ||
| set => _backingDictionary[key] = value; | ||
| } | ||
|
|
||
| public ICollection<string> Keys => _backingDictionary.Keys; | ||
|
|
||
| public int Count => _backingDictionary.Count; | ||
|
|
||
| public bool ContainsKey(string key) => _backingDictionary.ContainsKey(key); | ||
|
|
||
| public bool TryGetValue(string key, [MaybeNullWhen(false)] out Type? value) => _backingDictionary.TryGetValue(key, out value); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -371,15 +371,10 @@ public Processor( | |
|
|
||
| _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.
(minor) I assume we don't need to worry about the size of these objects.
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.
@krwq do you know if that's the case? Also, do you know if EarlyBoundInfos get used for anything other than Scripts in Xsls? Because if that's the case, we can potentially just remove this whole type as it would be currently just dead code.