-
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
Preserve DebuggerTypeProxyAttribute classes #39126
Conversation
Also, ensure the test app assembly is linked, and not copied during trimming tests, which was a bug in our test infrastructure. Fix dotnet#37307
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
{ | ||
ProxyTypeName = typeName; | ||
} | ||
|
||
// The Proxy is only invoked by the debugger, so it needs to have its | ||
// members preserved | ||
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] |
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.
What does this attribute mean when used on a get-only auto-prop?
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.
Linker will auto-propagate it to the backing field - and the field assignment in .ctor will see it and act accordingly.
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.
Thanks. What does it do that the same attribute on the ctor args doesn't achieve?
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.
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.
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.
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.
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.
They'd get a warning, but would the linker have actually removed things they may be depending on from the type?
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.
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.
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.
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.
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.
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.
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.
Thanks for the details.
src/libraries/System.Runtime/tests/TrimmingTests/DebuggerTypeProxyAttributeTests.cs
Show resolved
Hide resolved
88649dd
to
cfa6355
Compare
CI failure is #39291. Merging. |
Also, ensure the test app assembly is linked, and not copied during trimming tests, which was a bug in our test infrastructure.
Fix #37307
Note that this will actually make apps larger now that we are preserving the
DebuggerTypeProxyAttribute
classes by default.Once #39000 is merged, we can add the
DebuggerTypeProxyAttribute
(and other debugging related attributes) to be removed when the feature switchSystem.Diagnostics.Debugger.IsSupported
isfalse
.cc @stephentoub @MichalStrehovsky @vitek-karas