Implement DAMT.*WithInherited
#109608
Implement DAMT.*WithInherited
#109608
Conversation
dotnet-issue-labeler
bot
added
area-Tools-ILLink
|
Note regarding the |
1 similar comment
|
Note regarding the |
| /// <summary> | ||
| /// Specifies all non-public constructors, including those inherited from base classes. | ||
| /// </summary> | ||
| NonPublicConstructorsWithInherited = NonPublicConstructors | 0x4000, |
There was a problem hiding this comment.
I haven't seen any discussion on the actual numerical values used here.
Why don't we add a "flag" which indicates "inherited"? Is it because there would be possible values which don't have any semantic meaning?
Alternatively we would not need to declare the flag, just use numeric values which effectively use the flag. In reality here we're introducing a new flag-like numeric value for each kind of metadata, which doesn't seem necessary. It consumes lot of available bits in this enum.
There was a problem hiding this comment.
It was discussed here: https://www.youtube.com/watch?v=uyXehmcAkHc&t=0h9m31s.
We want a bit for each of these because e.g. if I want a combination of non-public fields (including inherited) and public constructors (but not inherited), this shouldn't mean to inherit all constructors. So we need an extra bit for each "inherited" possibility.
We also set the existing bit just for convenience so that bit operations still work nicely (i.e. it would be sufficient to do NonPublicConstructorsWithInherited = 0x4000, but NonPublicConstructorsWithInherited = NonPublicConstructors | 0x4000 makes working with the enums easier because there's still the subset relationship and we can bit test without special casing.
The original proposal had two bits less (just "inherit constructors" and "inherit nested types") but the API review extended it to the more granular "inherit public constructors" and "inherit private constructors". Solely because the problem with .All is that it's too broad and we could have the same problem with "inherit constructors" being too broad in the future.
There was a problem hiding this comment.
Thanks a lot - makes sense.
Nit: Would be good to write this down somewhere in the issue or this PR - I have to admit I didn't watch the video.
There was a problem hiding this comment.
I've moved the explanation to the PR description so that it's at the top.
|
|
||
| namespace System.Diagnostics.CodeAnalysis | ||
| { | ||
| public static class DynamicallyAccessedMemberTypesEx |
There was a problem hiding this comment.
Just curious why we need to add this? Is it that we don't have live flow of bits between the libraries and the trimmer code base here? If that is the case, migh be worth adding a comment so that eventually we delete this.
There was a problem hiding this comment.
ILLink is needed to build the framework so we can't build it with the live framework.
| @@ -34,13 +34,15 @@ public static IEnumerable<TypeSystemEntity> GetDynamicallyAccessedMembers(this T | |||
|
|
|||
| if (memberTypes.HasFlag(DynamicallyAccessedMemberTypes.NonPublicConstructors)) | |||
| { | |||
| foreach (var c in typeDefinition.GetConstructorsOnType(filter: null, bindingFlags: BindingFlags.NonPublic)) | |||
| bool withInherited = !declaredOnly && memberTypes.HasFlag(DynamicallyAccessedMemberTypesEx.NonPublicConstructorsWithInherited); | |||
| foreach (var c in typeDefinition.ApplyIncludeInherited(t => t.GetConstructorsOnType(filter: null, bindingFlags: BindingFlags.NonPublic), withInherited)) | |||
There was a problem hiding this comment.
Is there a reason we can't make GetConstructorsOnType (and similar) walk up the base types, based on the binding flags passed in? (Is it just that we're trying to keep GetConstructorsOnType aligned with the reflection semantics?)
There was a problem hiding this comment.
I spent a good part of the day trying to figure out how to do this :)
The problem is that all of these APIs take BindingFlags, which is a public enum with documented meaning of all the flags. So:
- Private means: do not recurse in bases.
- DeclaredOnly also has a meaning for publics (it means do not recurse for publics)
I had an extra bool that says "please recurse into bases", but the extra argument then conflicts with the default reflection semantics and with whatever BindingFlags specify. This didn't feel particularly readable.
Then I considered just not using BindingFlags here, but we do read random BindingFlags using dataflow and just send them over to here so we need some translation to exist. We also sometimes inject DeclaredOnly by ourselves.
All in all what I have here felt like the cleanest because WithInherited behavior handling can be restricted to this one method instead of complicating the surface area everywhere.
There was a problem hiding this comment.
Makes sense, thanks for the explanation!
|
/ba-g timeout in an unrelated wasm leg |
Fixes #88512.
Add support for NonPublicConstructorsWithInherited et al in the shape that was approved by API review.
We want a bit for each of these new inherited options because e.g. if I want a combination of non-public fields (including inherited) and public constructors (but not inherited), this shouldn't mean to inherit all constructors. So we need an extra bit for each "inherited" possibility.
We also set the existing bit just for convenience so that bit operations still work nicely (i.e. it would be sufficient to do
NonPublicConstructorsWithInherited = 0x4000, butNonPublicConstructorsWithInherited = NonPublicConstructors | 0x4000makes working with the enums easier because there's still the subset relationship and we can bit test without special casing.The original proposal had two bits less (just "inherit constructors" and "inherit nested types") but the API review extended it to the more granular "inherit public constructors" and "inherit private constructors". Solely because the problem with .All is that it's too broad and we could have the same problem with "inherit constructors" being too broad in the future.
Cc @dotnet/illink @dotnet/ilc-contrib @eerhardt