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.

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

Resolving ILLink warnings on System.Private.Xml (Part 1) #49413

Merged
merged 5 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,6 @@
<property name="Scope">member</property>
<property name="Target">M:System.Xml.Serialization.XmlReflectionImporter.GetMethodFromSchemaProvider(System.Xml.Serialization.XmlSchemaProviderAttribute,System.Type)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2070</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Xml.Xsl.Runtime.EarlyBoundInfo.#ctor(System.String,System.Type)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2070</argument>
Expand Down Expand Up @@ -345,15 +339,15 @@
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2080</argument>
<argument>IL2067</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Xml.Xsl.Runtime.XmlExtensionFunction.Bind</property>
<property name="Target">M:System.Xml.Xsl.Runtime.XmlExtensionFunction.#ctor(System.String,System.String,System.Int32,System.Type,System.Reflection.BindingFlags)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2080</argument>
<argument>IL2067</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Xml.Xsl.Runtime.XmlExtensionFunction.CanBind</property>
<property name="Target">M:System.Xml.Xsl.Runtime.XmlExtensionFunctionTable.Bind(System.String,System.String,System.Int32,System.Type,System.Reflection.BindingFlags)</property>
</attribute>
</assembly>
</linker>
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Xml.Xsl.Qil;
using System.Xml.Xsl.Runtime;
Expand Down Expand Up @@ -172,7 +173,7 @@ public int DeclareGlobalValue(string name)
/// Add early bound information to a list that is used by this query. Return the index of
/// the early bound information in the list.
/// </summary>
public int DeclareEarlyBound(string namespaceUri, Type ebType)
public int DeclareEarlyBound(string namespaceUri, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type ebType)
{
if (_earlyInfo == null)
_earlyInfo = new UniqueList<EarlyBoundInfo>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ internal class XmlILVisitor : QilVisitor
private IteratorDescriptor? _iterNested;
private int _indexId;

[RequiresUnreferencedCode("Method VisitXsltInvokeEarlyBound will require code that cannot be statically analyzed.")]
public XmlILVisitor()
{ }

//-----------------------------------------------
// Entry
Expand Down Expand Up @@ -3597,6 +3600,9 @@ protected override QilNode VisitXsltInvokeLateBound(QilInvokeLateBound ndInvoke)
/// <summary>
/// Generate code for QilNodeType.XsltInvokeEarlyBound.
/// </summary>
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:MissingRequiresUnreferencedCodeAttribute",
Justification = "Supressing warning about not having the RequiresUnreferencedCode attribute since we added " +
"the attribute to this subclass' constructor. This allows us to not have to annotate the whole QilNode hirerarchy.")]
protected override QilNode VisitXsltInvokeEarlyBound(QilInvokeEarlyBound ndInvoke)
{
QilName ndName = ndInvoke.Name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#nullable disable
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

namespace System.Xml.Xsl.Runtime
Expand All @@ -14,13 +15,16 @@ internal sealed class EarlyBoundInfo
{
private readonly string _namespaceUri; // Namespace Uri mapped to these early bound functions
private readonly ConstructorInfo _constrInfo; // Constructor for the early bound function object
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
private readonly Type _ebType;
Copy link
Member

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.

Copy link
Member Author

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.


public EarlyBoundInfo(string namespaceUri, Type ebType)
public EarlyBoundInfo(string namespaceUri, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type ebType)
{
Debug.Assert(namespaceUri != null && ebType != null);

// Get the default constructor
_namespaceUri = namespaceUri;
_ebType = ebType;
_constrInfo = ebType.GetConstructor(Type.EmptyTypes);
Debug.Assert(_constrInfo != null, "The early bound object type " + ebType.FullName + " must have a public default constructor");
}
Expand All @@ -33,7 +37,11 @@ public EarlyBoundInfo(string namespaceUri, Type ebType)
/// <summary>
/// Return the Clr Type of the early bound object.
/// </summary>
public Type EarlyBoundType { get { return _constrInfo.DeclaringType; } }
[property: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
joperezr marked this conversation as resolved.
Show resolved Hide resolved
public Type EarlyBoundType
{
get { return _ebType; }
}

/// <summary>
/// Create an instance of the early bound object.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Reflection;
using System.Globalization;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Xml.Xsl.Runtime
{
Expand Down Expand Up @@ -57,6 +58,7 @@ internal class XmlExtensionFunction
private string _namespaceUri; // Extension object identifier
private string _name; // Name of this method
private int _numArgs; // Argument count
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
private Type _objectType; // Type of the object which will be searched for matching methods
private BindingFlags _flags; // Modifiers that were used to search for a matching signature
private int _hashCode; // Pre-computed hashcode
Expand Down Expand Up @@ -95,7 +97,7 @@ public XmlExtensionFunction(string name, string namespaceUri, int numArgs, Type
/// <summary>
/// Initialize, but do not bind.
/// </summary>
public void Init(string name, string namespaceUri, int numArgs, Type objectType, BindingFlags flags)
public void Init(string name, string namespaceUri, int numArgs, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicMethods | DynamicallyAccessedMemberTypes.PublicMethods)] Type objectType, BindingFlags flags)
{
_name = name;
_namespaceUri = namespaceUri;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#nullable disable
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Xml.Xsl.IlGen;
using System.Xml.Xsl.Qil;
Expand Down Expand Up @@ -35,6 +36,7 @@ internal class XmlQueryStaticData
/// <summary>
/// Constructor.
/// </summary>
[RequiresUnreferencedCode("This method will create a copy that uses earlybound types which cannot be statically analyzed.")]
public XmlQueryStaticData(XmlWriterSettings defaultWriterSettings, IList<WhitespaceRule> whitespaceRules, StaticDataManager staticData)
{
Debug.Assert(defaultWriterSettings != null && staticData != null);
Expand Down Expand Up @@ -70,6 +72,7 @@ public XmlQueryStaticData(XmlWriterSettings defaultWriterSettings, IList<Whitesp
/// <summary>
/// Deserialize XmlQueryStaticData object from a byte array.
/// </summary>
[RequiresUnreferencedCode("This method will create EarlyBoundInfo from passed in ebTypes array which cannot be statically analyzed.")]
public XmlQueryStaticData(byte[] data, Type[] ebTypes)
{
MemoryStream dataStream = new MemoryStream(data, /*writable:*/false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public XmlILGenerator()
// SxS Note: The way the trace file names are created (hardcoded) is NOT SxS safe. However the files are
// created only for internal tracing purposes. In addition XmlILTrace class is not compiled into retail
// builds. As a result it is fine to suppress the FxCop SxS warning.
[RequiresUnreferencedCode("This method will use the XmlILVisitor which will use types that cannot be statically analyzed.")]
public XmlILCommand? Generate(QilExpression query, TypeBuilder? typeBldr)
{
_qil = query;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,14 @@ private QilExpression Compile(Compiler compiler)
}

// Create list of all early bound objects
Dictionary<string, Type?> scriptClasses = compiler.Scripts.ScriptClasses;
Scripts.LinkerSafeDictionary scriptClasses = compiler.Scripts.ScriptClasses;
joperezr marked this conversation as resolved.
Show resolved Hide resolved
List<EarlyBoundInfo> ebTypes = new List<EarlyBoundInfo>(scriptClasses.Count);
foreach (KeyValuePair<string, Type?> pair in scriptClasses)
foreach (string key in scriptClasses.Keys)
Copy link
Member

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 GetEnumerator from TrimSafeDictionary?

Copy link
Member Author

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>.get which 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.

{
if (pair.Value != null)
Type? value = scriptClasses[key];
if (value != null)
{
ebTypes.Add(new EarlyBoundInfo(pair.Key, pair.Value));
ebTypes.Add(new EarlyBoundInfo(key, value));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 Dictionary<string, Type?> _nsToType = new Dictionary<string, Type?>();
private readonly LinkerSafeDictionary _nsToType = new LinkerSafeDictionary();
private readonly XmlExtensionFunctionTable _extFuncs = new XmlExtensionFunctionTable();

public Scripts(Compiler compiler)
{
_compiler = compiler;
}

public Dictionary<string, Type?> ScriptClasses
public LinkerSafeDictionary ScriptClasses
{
get { return _nsToType; }
}
Expand All @@ -41,5 +43,44 @@ public Scripts(Compiler compiler)
}
return null;
}

internal class LinkerSafeDictionary : IDictionary<string, Type?>
joperezr marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly Dictionary<string, Type?> _backingDictionary = new Dictionary<string, Type?>();

public Type? this[string key]
{
[UnconditionalSuppressMessage("TrimAnalysis", "IL2093:MissingAttributeOnBaseClass", Justification = "This implementation of IDictionary must have extra annotation attributes in order to be trim safe")]
[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)]
Copy link
Member

Choose a reason for hiding this comment

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

Can we just put the attribute on the whole property?

Copy link
Member Author

Choose a reason for hiding this comment

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

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];
[UnconditionalSuppressMessage("TrimAnalysis", "IL2092:MissingAttributeOnBaseClass", Justification = "This implementation of IDictionary must have extra annotation attributes in order to be trim safe")]
[param: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
set => ((IDictionary<string, Type?>)_backingDictionary)[key] = value;
}

public ICollection<string> Keys => ((IDictionary<string, Type?>)_backingDictionary).Keys;

public ICollection<Type?> Values => ((IDictionary<string, Type?>)_backingDictionary).Values;

public int Count => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).Count;
Copy link
Member

@eerhardt eerhardt Mar 17, 2021

Choose a reason for hiding this comment

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

Why are all these casts of _backingDictionary necessary? Can't you just return _backingDictionary.Count? #Resolved

Copy link
Member Author

@joperezr joperezr Mar 18, 2021

Choose a reason for hiding this comment

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

I'll fix it, not necessary, I just used the quick refactoring ctrl+'.' options in VS that added the casts:
image
#Resolved


public bool IsReadOnly => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).IsReadOnly;

[UnconditionalSuppressMessage("TrimAnalysis", "IL2092:MissingAttributeOnBaseClass", Justification = "This implementation of IDictionary must have extra annotation attributes in order to be trim safe")]
public void Add(string key, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type? value) => ((IDictionary<string, Type?>)_backingDictionary).Add(key, value);
public void Add(KeyValuePair<string, Type?> item) => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).Add(item);
public void Clear() => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).Clear();
public bool Contains(KeyValuePair<string, Type?> item) => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).Contains(item);
public bool ContainsKey(string key) => ((IDictionary<string, Type?>)_backingDictionary).ContainsKey(key);
public void CopyTo(KeyValuePair<string, Type?>[] array, int arrayIndex) => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).CopyTo(array, arrayIndex);
public IEnumerator<KeyValuePair<string, Type?>> GetEnumerator() => ((IEnumerable<KeyValuePair<string, Type?>>)_backingDictionary).GetEnumerator();
public bool Remove(string key) => ((IDictionary<string, Type?>)_backingDictionary).Remove(key);
public bool Remove(KeyValuePair<string, Type?> item) => ((ICollection<KeyValuePair<string, Type?>>)_backingDictionary).Remove(item);
public bool TryGetValue(string key, [MaybeNullWhen(false)] out Type? value) => ((IDictionary<string, Type?>)_backingDictionary).TryGetValue(key, out value);
IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)_backingDictionary).GetEnumerator();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ private void Reset()

// SxS: This method does not take any resource name and does not expose any resources to the caller.
// It's OK to suppress the SxS warning.
[RequiresUnreferencedCode("This method requires a call to LoadInternal which might use types that cannot be statically analyzed.")]
joperezr marked this conversation as resolved.
Show resolved Hide resolved
public void Load(XmlReader stylesheet)
{
Reset();
Expand All @@ -97,6 +98,7 @@ public void Load(XmlReader stylesheet)

// SxS: This method does not take any resource name and does not expose any resources to the caller.
// It's OK to suppress the SxS warning.
[RequiresUnreferencedCode("This method requires a call to LoadInternal which might use types that cannot be statically analyzed.")]
public void Load(XmlReader stylesheet, XsltSettings? settings, XmlResolver? stylesheetResolver)
{
Reset();
Expand All @@ -105,6 +107,7 @@ public void Load(XmlReader stylesheet, XsltSettings? settings, XmlResolver? styl

// SxS: This method does not take any resource name and does not expose any resources to the caller.
// It's OK to suppress the SxS warning.
[RequiresUnreferencedCode("This method requires a call to LoadInternal which might use types that cannot be statically analyzed.")]
public void Load(IXPathNavigable stylesheet)
{
Reset();
Expand All @@ -113,12 +116,14 @@ public void Load(IXPathNavigable stylesheet)

// SxS: This method does not take any resource name and does not expose any resources to the caller.
// It's OK to suppress the SxS warning.
[RequiresUnreferencedCode("This method requires a call to LoadInternal which might use types that cannot be statically analyzed.")]
public void Load(IXPathNavigable stylesheet, XsltSettings? settings, XmlResolver? stylesheetResolver)
{
Reset();
LoadInternal(stylesheet, settings, stylesheetResolver);
}

[RequiresUnreferencedCode("This method requires a call to LoadInternal which might use types that cannot be statically analyzed.")]
public void Load(string stylesheetUri)
{
Reset();
Expand All @@ -129,6 +134,7 @@ public void Load(string stylesheetUri)
LoadInternal(stylesheetUri, XsltSettings.Default, CreateDefaultResolver());
}

[RequiresUnreferencedCode("This method requires a call to LoadInternal which might use types that cannot be statically analyzed.")]
public void Load(string stylesheetUri, XsltSettings? settings, XmlResolver? stylesheetResolver)
{
Reset();
Expand All @@ -139,6 +145,7 @@ public void Load(string stylesheetUri, XsltSettings? settings, XmlResolver? styl
LoadInternal(stylesheetUri, settings, stylesheetResolver);
}

[RequiresUnreferencedCode("This method calles CompileQilToMsil method which might use types that cannot be statically analyzed.")]
private CompilerErrorCollection LoadInternal(object stylesheet, XsltSettings? settings, XmlResolver? stylesheetResolver)
{
if (stylesheet == null)
Expand Down Expand Up @@ -184,6 +191,7 @@ private void CompileXsltToQil(object stylesheet, XsltSettings settings, XmlResol
return null;
}

[RequiresUnreferencedCode("This method calls XmlILGenerator.Generate which might require types that cannot be statically analyzed.")]
private void CompileQilToMsil(XsltSettings settings)
{
_command = new XmlILGenerator().Generate(_qil!, /*typeBuilder:*/null)!;
Expand All @@ -194,7 +202,7 @@ private void CompileQilToMsil(XsltSettings settings)
//------------------------------------------------
// Load compiled stylesheet from a Type
//------------------------------------------------

[RequiresUnreferencedCode("This method uses the types that are grabbed from the compileStylesheet which cannot be statically analyzed.")]
public void Load(Type compiledStylesheet)
{
Reset();
Expand Down Expand Up @@ -238,6 +246,7 @@ public void Load(Type compiledStylesheet)
throw new ArgumentException(SR.Format(SR.Xslt_NotCompiledStylesheet, compiledStylesheet.FullName), nameof(compiledStylesheet));
}

[RequiresUnreferencedCode("This method uses the constructors of the passed in earlyBoundTypes which cannot be statically analyzed.")]
public void Load(MethodInfo executeMethod, byte[] queryData, Type[]? earlyBoundTypes)
{
Reset();
Expand Down