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

Add MemberInfo.IsCollectible & Assembly.IsCollectible #25671

Open
JonHanna opened this Issue Dec 4, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@JonHanna
Copy link
Collaborator

JonHanna commented Dec 4, 2017

We may wish to treat a collectible assembly or member info differently to a non-collectible. Most obviously, we might want to avoid caching a collectible type or method for a long time and preventing that possible collection. (Type has an IsCollectible method, but it is not exposed.)

Proposed API:

namespace System.Reflection
{
    public abstract class Assembly
    {
        public virtual bool IsCollectible { get; }
    }

    public abstract class MemberInfo
    {
        public virtual bool IsCollectible { get; }
    }
}

Note that collectibility of assemblies is not the only factor on collectibility of member infos. TypeBuilder is derived from Type and instances are always collectible.

TypeDelegator is itself collectible, but it should report on its delegatee.

Editted to include all of MemberInfo and also Assembly.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 4, 2017

cc @AtsushiKan

JonHanna added a commit to JonHanna/coreclr that referenced this issue Dec 4, 2017

Have a Type.IsConvertible property.
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

jkotas added a commit to dotnet/coreclr that referenced this issue Dec 5, 2017

Have a Type.IsConvertible property. (#15365)
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

dotnet-bot added a commit to dotnet/corert that referenced this issue Dec 5, 2017

Have a Type.IsConvertible property. (dotnet/coreclr#15365)
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas added a commit to dotnet/corert that referenced this issue Dec 5, 2017

Have a Type.IsConvertible property. (dotnet/coreclr#15365)
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@terrajobst

This comment has been minimized.

Copy link
Member

terrajobst commented Dec 5, 2017

Why would this property live on Type rather than Assembly? My understanding is that we have collectible assemblies, not collectible types IOW collectible types would be typed defined in a collectible assembly, no?

@JonHanna

This comment has been minimized.

Copy link
Collaborator

JonHanna commented Dec 5, 2017

I was thinking about suggesting it on assemblies too, but for here:

  1. Most developers deal with types more often than assemblies.
  2. In terms of implementation, types is currently easy (already done, but not exposed), assemblies not (the only approach I can find being used is to get a type from it and examine that).

Assembly might still be worth doing too.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Dec 5, 2017

I still don't understand why the current implementation in CoreCLR is on Type - is it because of LCG @jkotas?
The only true collectibility is at assembly level ...

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Dec 5, 2017

FYI: The API review discussion was recorded - see https://youtu.be/BI3iXFT8H7E?t=5891 (4 min duration)

@JonHanna

This comment has been minimized.

Copy link
Collaborator

JonHanna commented Dec 6, 2017

The current implementation might just be because it's needed, since it's internal (and goes back to before corefx) and used internally, with the full implementation being extern. That in itself though shows that the CLR at least had a need for such a property (well, it was a method until yesterday) but not for one on assemblies.

Not that I think exposing that information about assemblies is necessarily without use either; ironically while @jkotas' had clearly been thinking of that CLR method in his suggestion to my looking for a better prediction of collectibility, I found it through a google hit on someone using it to determine if assemblies were collectible, so there's at least one person out there wanting that too.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 6, 2017

As @JonHanna said, this is about what the current need is (examining collectibility at type level) vs. what the implementation is constrained to today (collectibility at Assembly / AssemblyLoadContext level and Method level). In theory, we can have collectibility at type level at some point in future. E.g. we can have DynamicType that is close sibling of existing DynamicMethod that can live without really having own assembly.

So we have a choice:

  1. Expose what matches the constraints of the current implementation. Today, assemblies and methods are individually collectible. So it would make sense to expose Assembly.IsCollectible and MethodBase.IsCollectible.
  2. Expose what the current uses care about: Type.IsCollectible
  3. Assume that everything can be individually collectible eventually, and expose all: Assembly.IsCollectible and MemberInfo.IsCollectible (this covers Type.IsCollectible and MethodBase.IsCollectible).
@JonHanna

This comment has been minimized.

Copy link
Collaborator

JonHanna commented Dec 6, 2017

There's perhaps a nomenclature issue in that Is Collectible in the sense "is this a thing that can at some point be collected" is perfectly apt for Type and indeed is always true for some implementations (TypeBuilder, EnumBuilder, etc. are all types of Type and can all be collected, as indeed can TypeDelegator though I would assume that someone asking this of a delegator cares about the delegatee).

Indeed, for runtime types while it's the assembly that dictates whether the type could ever be collected, it's the types that have more effect on whether this happens as the types being reachable keep their assembly alive but the assembly being reachable does not keep its types allive.

Is Collectible in the sense of "is this an assembly that has that special condition that allows it and its types to be collected" obviously only applies to assemblies.

I think though that Is Collectible is reasonable for "can this ever be collected" and preferable to other terms I could think of (Is Permanent, Is Transient).

Assume that everything can be individually collectible eventually, and expose all: Assembly.IsCollectible and MemberInfo.IsCollectible (this covers Type.IsCollectible and MethodBase.IsCollectible).

I think there's a case for all of these. Alas I haven't been able to retrace my google breadcrumbs, but I did come across someone seeking to determine Assembly.IsCollectible (accepted answer: get a type from it and use reflection to call RuntimeTypeHandle.IsCollectible() on its type handle), and one of the uses of the conservative guessing S.L.Expressions does to determine collectability really wants MethodBase.IsCollectible). So if this is feasible, I'll happily change the request to this option.

jashook added a commit to jashook/coreclr that referenced this issue Dec 12, 2017

Have a Type.IsConvertible property. (dotnet#15365)
dotnet/corefx#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at dotnet/corefx#25663 and dotnet/corefx#25670

Mirror PR to corert will need and overload making runtime types return
false there.

@JonHanna JonHanna changed the title API Request: Type.IsCollectible API Request: MemberInfo.IsCollectible & Assembly.IsCollectible Dec 14, 2017

@terrajobst

This comment has been minimized.

Copy link
Member

terrajobst commented Jan 2, 2018

Looks good as proposed; my only question is whether we need/should expose this on TypeInfo as well?

Edit Looks like TypeInfo now inherits from Type so we should be fine.

@karelz karelz added this to the Future milestone Jan 2, 2018

@karelz karelz changed the title API Request: MemberInfo.IsCollectible & Assembly.IsCollectible Add MemberInfo.IsCollectible & Assembly.IsCollectible Jan 2, 2018

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 2, 2018

FYI: The API review discussion was recorded - see https://youtu.be/dOpH6bUNbcw?t=992 (8 min duration)

dotnet-bot added a commit that referenced this issue Jan 13, 2018

Have a Type.IsConvertible property. (dotnet/coreclr#15365)
#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at #25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot added a commit that referenced this issue Jan 13, 2018

Have a Type.IsConvertible property. (dotnet/coreclr#15365)
#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at #25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit that referenced this issue Jan 16, 2018

Have a Type.IsConvertible property. (dotnet/coreclr#15365)
#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at #25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit that referenced this issue Jan 16, 2018

Have a Type.IsConvertible property. (dotnet/coreclr#15365)
#25671 asks for this as a public API, but this just seeks
to make it available to corefx.

Motivation discussed at #25663 and #25670

Mirror PR to corert will need and overload making runtime types return
false there.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
@JonHanna

This comment has been minimized.

Copy link
Collaborator

JonHanna commented Feb 5, 2018

With the extension from the original proposal, I don't have a good idea about implementation. For Type we can just expose what's already there. For Assembly the best one can currently do is find a Type and see if it is collectible but that doesn't work if there's no types (an edge case admittedly). Other MethodInfos are either DynamicMethods which are collectible, or collectible if they belong to a collectible type.

Am I missing another case?

Are false negatives okay on assemblies with no types? Do we need an external method to give a more dependable answer? (I haven't done any C or C++ code in a long time, so I'm not confident about that side of things).

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 5, 2018

Yes, new calls into the unmanaged portion of the runtime need to be added to handle the method and assembly cases well. https://github.com/dotnet/coreclr/blob/master/Documentation/botr/mscorlib.md#calling-from-managed-to-native-code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment