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

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Mar 10, 2021

Contributes to #45623

cc: @eerhardt @krwq

This is the first part of resolving the System.Private.Xml linker warnings, which for now involves most of the XslCompiledTransform ones.

@ghost
Copy link

ghost commented Mar 10, 2021

Tagging subscribers to this area: @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45623

cc: @eerhardt @krwq

This is the first part of resolving the System.Private.Xml linker warnings, which for now involves most of the non-serializer ones.

Author: joperezr
Assignees: -
Labels:

area-System.Xml

Milestone: -

@joperezr joperezr requested review from eerhardt and krwq and removed request for eerhardt March 10, 2021 03:58
@joperezr joperezr added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 10, 2021
@ghost
Copy link

ghost commented Mar 10, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45623

cc: @eerhardt @krwq

This is the first part of resolving the System.Private.Xml linker warnings, which for now involves most of the non-serializer ones.

Author: joperezr
Assignees: -
Labels:

area-System.Xml, linkable-framework

Milestone: -

@joperezr
Copy link
Member Author

joperezr commented Mar 10, 2021

Some of the annotations touch public API so I'll rerun genapi to have those annotations on the ref as well.

@joperezr
Copy link
Member Author

joperezr commented Mar 11, 2021

This is almost ready for review, but I'm still addressing a couple of warnings still so hold off on reviewing for now

@joperezr
Copy link
Member Author

This is now ready for review

@@ -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.

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.

[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

@@ -371,15 +371,10 @@ internal XsltOutput Output

_scriptExtensions = new Hashtable(_stylesheet.ScriptObjectTypes.Count);
{
foreach (DictionaryEntry entry in _stylesheet.ScriptObjectTypes)
// Scripts are not supported on stylesheets
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a test for this or something? Are we concerned about the behavior change?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@krwq krwq Mar 16, 2021

Choose a reason for hiding this comment

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

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.

…rlyBoundInfo.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@@ -371,15 +371,10 @@ internal XsltOutput Output

_scriptExtensions = new Hashtable(_stylesheet.ScriptObjectTypes.Count);
{
foreach (DictionaryEntry entry in _stylesheet.ScriptObjectTypes)
// Scripts are not supported on stylesheets
if (_stylesheet.ScriptObjectTypes.Count > 0)
Copy link
Member

Choose a reason for hiding this comment

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

might be a good idea to also simplify parsing this to save even more bytes

@@ -238,6 +238,7 @@ public void Load(Type compiledStylesheet)
throw new ArgumentException(SR.Format(SR.Xslt_NotCompiledStylesheet, compiledStylesheet.FullName), nameof(compiledStylesheet));
}

[RequiresUnreferencedCode("This method will call into constructors of the earlyBoundTypes array which cannot be statically analized.")]
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.

analized => analyzed repeated elsewhere.

@@ -70,6 +70,10 @@ 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.
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:MissingRequiresUnreferencedCode",
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.

(nit) typically I've been using just IL2026:RequiresUnreferencedCode. The word after the number doesn't really matter to the trimmer. But warning IL2026 is because you are calling a method attributed with RequiresUnreferencedCode. So when you suppress the warning, you are suppressing the fact that you are calling a method with RequiresUnreferencedCode, not that RequiresUnreferencedCode is "missing".

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:MissingRequiresUnreferencedCode",
Justification = "This method will generate the IL methods using RefEmit at runtime, which will then try to call them " +
"using methods that are annotated as RequiresUnreferencedCode. In this case, these uses can be suppressed as the " +
"linker won't be able to trim any IL that gets generated at runtime.")]
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.

linker => trimmer #Resolved

}
_scriptExtensions.Add(namespaceUri, Activator.CreateInstance((Type)entry.Value!,
BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic, null, null, null));
throw new PlatformNotSupportedException(SR.CompilingScriptsNotSupported);
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.

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

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.

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


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

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

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @joperezr!

@joperezr joperezr merged commit fbb6a1c into dotnet:main Mar 19, 2021
@joperezr joperezr deleted the LinkerXml branch March 19, 2021 16:51
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Xml linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants