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.

Sign up for GitHub

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

Jump to bottom

Preserve DebuggerTypeProxyAttribute classes #39126

Merged
merged 3 commits into from
Jul 14, 2020
Merged

Preserve DebuggerTypeProxyAttribute classes #39126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0cbbe39
Preserve DebuggerTypeProxyAttribute classes
eerhardt Jul 10, 2020
37fccc5
Add test for DebuggerTypeProxy string overload.
eerhardt Jul 13, 2020
cfa6355
Update ref to match impl.
eerhardt Jul 13, 2020
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
13 changes: 12 additions & 1 deletion eng/testing/linker/SupportFiles/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@
</ItemGroup>
</Target>

<!-- This will need to change to AfterTargets=PrepareForILLink when we get a new SDK -->
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
<Target Name="EnsureAllAssembliesAreLinked"
AfterTargets="_SetILLinkDefaults">
<ItemGroup>
<!-- This will need to change to ManagedAssemblyToLink and TrimMode=link when we get a new SDK -->
<_ManagedAssembliesToLink>
<action>link</action>
</_ManagedAssembliesToLink>
</ItemGroup>
</Target>

<Target Name="PublishTrimmed" DependsOnTargets="RestoreProject;UpdateRuntimePack;Publish" />

</Project>
</Project>
11 changes: 9 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/DebuggerTypeProxyAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;

namespace System.Diagnostics
{
[AttributeUsage(AttributeTargets.Struct | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = true)]
public sealed class DebuggerTypeProxyAttribute : Attribute
{
private Type? _target;

public DebuggerTypeProxyAttribute(Type type)
public DebuggerTypeProxyAttribute(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type)
{
if (type == null)
{
Expand All @@ -18,11 +21,15 @@ public DebuggerTypeProxyAttribute(Type type)
ProxyTypeName = type.AssemblyQualifiedName!;
}

public DebuggerTypeProxyAttribute(string typeName)
public DebuggerTypeProxyAttribute(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] string typeName)
{
ProxyTypeName = typeName;
}

// The Proxy is only invoked by the debugger, so it needs to have its
// members preserved
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
Copy link
Member

Choose a reason for hiding this comment

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

What does this attribute mean when used on a get-only auto-prop?

Copy link
Member

Choose a reason for hiding this comment

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

Linker will auto-propagate it to the backing field - and the field assignment in .ctor will see it and act accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. What does it do that the same attribute on the ctor args doesn't achieve?

Copy link
Member

Choose a reason for hiding this comment

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

Linker doesn't auto-propagate annotations across method bodies. So the store to the backing field in the .ctor will NOT annotate the backing field. And so if something somewhere consumes the backing field (or rather the property getter), it would not get the annotation. The best way to think about this is sort of backwards:

  • We annotate the property getter because the type should fulfill such requirements (this specific example is a bit weird since the requirement doesn't come from managed code)
  • Linker will auto-propagate that annotation to the backing field (this is the only auto-propagation we do, mostly as it's relatively easy and it doesn't require many attribute in one place)
  • Now the store in the .ctor will generate a warning as it's assigning unannotated value to an annotated field
  • So we annotate the .ctor argument
  • This specific .ctor argument is pretty much only used with statically known types (that is typeof(TypeName) expressions) - so the linker will auto-apply the annotation to the statically known type - the chain ends here.

Copy link
Member

Choose a reason for hiding this comment

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

But why does it matter that the field is annotated? There only way to construct this type is with the two ctors. Both take a type (one as a string) input, and both are attributed the same way. When a type is passed to either ctor, the linker should see that and keep its members accordingly.

What is an example where just annotating the ctor arguments here is insufficient? What does also attributing the backing field enable? That's what I'm missing.

Copy link
Member

Choose a reason for hiding this comment

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

They'd get a warning, but would the linker have actually removed things they may be depending on from the type?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, annotating the constructor is all that's required to prevent the type from being linked away.

Annotating the property allows the linker analysis to prove that the code is correct. We want to drive the warnings baselines in #39133 to zero.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not pushing back, eliminating the warnings is important. But it is confusing that the annotation required to make it correct/safe is insufficient to make it warning-free, and that there's no distinction between the attributes used for those.

Copy link
Member

Choose a reason for hiding this comment

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

This specific case is not a good example of how the annotations should work. But let's assume the debugger is somewhere in managed code in the app - so something has a reference to the ProxyTypeName property and presumably it does something like:

var propertiesToShow = Type.GetType(attr.ProxyTypeName).GetProperties();

When linker sees that it goes (basically backwards):

  • I need all properties on the type returned by Type.GetType (cause there's a call to GetProperties() on it)
  • So it means that the type name coming to Type.GetType must be a type with all properties
  • So it means I need the attr.ProxyTypeName to be a type name with all properties -> it should have at least DynamicallyAccessedMembers(DynamicallyAccessedMemberType.PublicProperties).
  • If it doesn't -> warn
  • So we go and annotate the ProxyTypeName with that annotation
  • Now running the linker again it will not warn on the usage above anymore, but it will warn in the attribute's .ctor since we're trying to assign an unannotated string to the backing field of ProxyTypeName
  • So we go and fix that by annotating the .ctor argument.
  • Running linker again it will not warn in the .ctor itself, but it now requires all the callers to fulfill that annotation. But since this is an attribute all callers provide constant strings as the argument - for this linker knows what to do, it finds the type of that name, keeps it and then also keeps all of its properties.

So for the application to work - only the last step is needed, for which only the annotation on the .ctor argument is needed. But that would not be enough for linker to figure out that the code using it is correct - and so it would have to warn (it doesn't do cross method analysis). So the other annotations, like the one on the property, are there to help linker prove that the code is correct and that it will work.

The other advantage is that if later on we change the usage and now call for example .GetProperties(BindingFlags.NonPublic), linker would suddenly warn, because the annotations don't cover private properties anymore and the app may break. Without the annotations flowing through everything it would have no way to tell that there's something wrong.

The reason why the annotations are the same everywhere is that they always mean the same thing - it's a requirement on the value which is passed in. That can be fulfilled either by:

  • passing in value with same or "bigger" annotation
  • passing in statically known value which the linker can actually find - and then it can make sure that type in question fulfills the requirements

Since the method/property itself doesn't which one of the two cases it will be, it applies the annotation - always the same one.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the details.

public string ProxyTypeName { get; }

public Type? Target
Expand Down
5 changes: 3 additions & 2 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5715,8 +5715,9 @@ public DebuggerStepThroughAttribute() { }
[System.AttributeUsageAttribute(System.AttributeTargets.Assembly | System.AttributeTargets.Class | System.AttributeTargets.Struct, AllowMultiple=true)]
public sealed partial class DebuggerTypeProxyAttribute : System.Attribute
{
public DebuggerTypeProxyAttribute(string typeName) { }
public DebuggerTypeProxyAttribute(System.Type type) { }
public DebuggerTypeProxyAttribute([System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] string typeName) { }
public DebuggerTypeProxyAttribute([System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] System.Type type) { }
[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)]
public string ProxyTypeName { get { throw null; } }
public System.Type? Target { get { throw null; } set { } }
public string? TargetTypeName { get { throw null; } set { } }
Expand Down
73 changes: 73 additions & 0 deletions src/libraries/System.Runtime/tests/TrimmingTests/DebuggerTypeProxyAttributeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
using System;
using System.Diagnostics;

/// <summary>
/// Tests that types used by DebuggerTypeProxyAttribute are not trimmed out
/// when Debugger.IsSupported is true (the default).
/// </summary>
class Program
{
static int Main(string[] args)
{
MyClass myClass = new MyClass() { Name = "trimmed" };
MyClassWithProxyString myClassWithString = new MyClassWithProxyString() { Name = "trimmed" };

Type[] allTypes = typeof(MyClass).Assembly.GetTypes();
bool foundDebuggerProxy = false;
bool foundStringDebuggerProxy = false;
for (int i = 0; i < allTypes.Length; i++)
{
Type currentType = allTypes[i];
if (currentType.FullName == "Program+MyClass+DebuggerProxy" &&
currentType.GetProperties().Length == 1 &&
currentType.GetConstructors().Length == 1)
{
foundDebuggerProxy = true;
}
else if (currentType.FullName == "Program+MyClassWithProxyStringProxy" &&
currentType.GetProperties().Length == 1 &&
currentType.GetConstructors().Length == 1)
{
foundStringDebuggerProxy = true;
}
}

return foundDebuggerProxy && foundStringDebuggerProxy ? 100 : -1;
}

[DebuggerTypeProxy(typeof(DebuggerProxy))]
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
public class MyClass
{
public string Name { get; set; }

private class DebuggerProxy
{
private MyClass _instance;

public DebuggerProxy(MyClass instance)
{
_instance = instance;
}

public string DebuggerName => _instance.Name + " Proxy";
}
}

[DebuggerTypeProxy("Program+MyClassWithProxyStringProxy")]
public class MyClassWithProxyString
{
public string Name { get; set; }
}

internal class MyClassWithProxyStringProxy
{
private MyClassWithProxyString _instance;

public MyClassWithProxyStringProxy(MyClassWithProxyString instance)
{
_instance = instance;
}

public string DebuggerName => _instance.Name + " StringProxy";
}
}
4 changes: 4 additions & 0 deletions src/libraries/System.Runtime/tests/TrimmingTests/DefaultValueAttributeCtorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ class Program
{
static int Main(string[] args)
{
// workaround TypeConverterAttribute not being annotated correctly
// https://github.com/dotnet/runtime/issues/39125
var _ = new MyStringConverter();

TypeDescriptor.AddAttributes(typeof(string), new TypeConverterAttribute(typeof(MyStringConverter)));

var attribute = new DefaultValueAttribute(typeof(string), "Hello, world!");
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
<TestConsoleAppSourceFiles Include="AppDomainGetThreadWindowsPrincipalTest.cs">
<SkipOnTestRuntimes>osx-x64;linux-x64</SkipOnTestRuntimes>
</TestConsoleAppSourceFiles>
<TestConsoleAppSourceFiles Include="DebuggerTypeProxyAttributeTests.cs" />
<TestConsoleAppSourceFiles Include="DefaultValueAttributeCtorTest.cs" />
<TestConsoleAppSourceFiles Include="GenericArraySortHelperTest.cs" />
<TestConsoleAppSourceFiles Include="StackFrameHelperTest.cs">
<!-- There is a bug with the linker where it is corrupting the pdbs while trimming
Expand Down