Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Preserve DebuggerTypeProxyAttribute classes #39126
Changes from 1 commit
0cbbe39
37fccc5
cfa6355
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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:When linker sees that it goes (basically backwards):
DynamicallyAccessedMembers(DynamicallyAccessedMemberType.PublicProperties)
.ProxyTypeName
with that annotationProxyTypeName
.ctor
argument..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:
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.