-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from 1 commit
538bb29
f688780
7cbd958
8b57fe5
6656141
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
{ | ||
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)); | ||
} | ||
} | ||
|
||
|
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 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; } | ||
} | ||
|
@@ -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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just put the attribute on the whole property? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are all these casts of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
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(); | ||
} | ||
} | ||
} |
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.