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.

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

[API Proposal]: Add Type.IsReferenceType #90993

Closed
Neme12 opened this issue Aug 23, 2023 · 24 comments
Closed

[API Proposal]: Add Type.IsReferenceType #90993

Neme12 opened this issue Aug 23, 2023 · 24 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection needs-author-action An issue or pull request that requires more info or actions from the author.
Milestone

Comments

@Neme12
Copy link

Neme12 commented Aug 23, 2023

Background and motivation

Type already has the IsValueType property, but there's no IsReferenceType property. This is necessary because it's not equivalent to just !type.IsValueType, and that's because there are types that are neither value or reference types, such as unconstrained generic parameters.

API Proposal

 namespace System;

 public abstract partial class Type
 {
     // Existing APIs for reference:
     public bool IsValueType { get; }
     protected virtual bool IsValueTypeImpl();

     // Proposed APIs:
+    public bool IsReferenceType { get; }
+    protected virtual bool IsReferenceTypeImpl();
 }

API Usage

if (type.IsReferenceType)
{
    // do something
}

Alternative Designs

No response

Risks

No response

@Neme12 Neme12 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Type already has the IsValueType property, but there's no IsReferenceType property. This is necessary because it's not equivalent to just !type.IsValueType, and that's because there are types that are neither value or reference types, such as generic parameters.

API Proposal

 namespace System;

 public abstract partial class Type
 {
     // Existing APIs for reference:
     public bool IsValueType { get; }
     protected virtual bool IsValueTypeImpl();

     // Proposed APIs:
+    public bool IsReferenceType { get; }
+    protected virtual bool IsReferenceTypeImpl();
 }

API Usage

if (type.IsReferenceType)
{
    // do something
}

Alternative Designs

No response

Risks

No response

Author: Neme12
Assignees: -
Labels:

api-suggestion, area-System.Reflection

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 23, 2023
@steveharter
Copy link
Member

ECMA-335 states that at the highest level there are only value types and reference types, so IsReferenceType would be the same as !ValueType.

For unconstrained generic parameters, yes that is an edge case and I don't think we want to add IsReferenceType just to return false for that and for IsValueType. We already have IsConstructedGenericType or IsGenericTypeDefinition.

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Aug 24, 2023
@steveharter steveharter added this to the Future milestone Aug 24, 2023
@steveharter
Copy link
Member

Setting to "future" for now; I believe we'll end up closing this.

cc @jkotas @MichalStrehovsky

@Neme12
Copy link
Author

Neme12 commented Aug 24, 2023

It's not just generics, there are also by-ref types. It's not that easy to check whether something is a regular reference type. I think it should be easier. Why not add the property, it feels natural to have it next to the existing IsValueType.

@En3Tho
Copy link
Contributor

En3Tho commented Aug 24, 2023

I believe it's better to collect concrete examples with possible workarounds to see if there are inconsistencies that really get into the eye or workarounds that are completely non intuitive or something like that.

Otherwise this is just easily done via extensions method in your own codebase.

@jkotas
Copy link
Member

jkotas commented Aug 24, 2023

Could you please share example of existing code in this repos or elsewhere on github that would benefit from this API? I would not be opposed to introducing a convenience API like this if we have a proof of its usefulness.

There are multiple conflicting definitions of "reference type". The C# definition is different from ECMA-335 definition. It would be best for the proposal to show implementation of the API using existing properties. It would clarify its exact behavior and it is something that we would need for the baseline System.Type implementation.

IsValueTypeImpl

Nit: I do not think that this would need to through public property + protected implementation split. This split is historic artifact, we have not done it for newer System.Type APIs.

@Neme12
Copy link
Author

Neme12 commented Aug 24, 2023

It would be best for the proposal to show implementation of the API using existing properties.

I don't know how it should be implemented. I don't know all the possible types that can occur and all the derived classes etc. That's the problem and that's why the API is needed - because it's not obvious or trivial to do this correctly, because of all the possible kinds of types that exist.

@jkotas
Copy link
Member

jkotas commented Aug 25, 2023

An implementation that matches ECMA-335 definition of a reference type would be bool IsReferenceType=>!IsValueType && !IsGenericParameter;. Does it match your expectations for how this API should behave?

@steveharter
Copy link
Member

It's not just generics, there are also by-ref types. It's not that easy to check whether something is a regular reference type

A by-ref parameter is considered a "reference type" in terms of ECMA-335 meaning, for example, that it will be tracked by the runtime for GC (at least if not a simple blittable value type). So treating "IsReferenceType == false" for by-ref parameters probably wouldn't pass review.

@Neme12
Copy link
Author

Neme12 commented Aug 30, 2023

An implementation that matches ECMA-335 definition of a reference type would be bool IsReferenceType=>!IsValueType && !IsGenericParameter;. Does it match your expectations for how this API should behave?

That's not good enough. If a type parameter is constrained to be : class or derived from a reference type, it should return true.

This is why I'm proposing the API. It's easy to get this wrong if you do it manually.

@Neme12
Copy link
Author

Neme12 commented Aug 31, 2023

@steveharter Fair enough. I would expect that to return false, but if that's what the spec says, then so be it.

@Neme12
Copy link
Author

Neme12 commented Aug 31, 2023

I didn't expect so much pushback. To me it feels like a natural companion to the existing IsValueType property, i.e. even if there weren't the edge cases with generics, I'd expect this property to be there just for completeness. I mean, look at Type.IsAssignableFrom and IsAssignableTo - there's both of them and there's no new scenario that the second one enables at all.

@jkotas
Copy link
Member

jkotas commented Sep 1, 2023

I didn't expect so much pushback.

I see two concerns with this proposal (reiterating #90993 (comment)):

  • It is not clear how the property should behave exactly, including all corner cases.
  • It is not clear that this proposed property is useful in practice.

If you can address these two concerns, the proposal will have much better chance.

look at Type.IsAssignableFrom and IsAssignableTo

There was no ambiguity how IsAssignableTo should be implemented, how it should behave and why it is useful.

@Neme12
Copy link
Author

Neme12 commented Sep 2, 2023

  • It is not clear that this proposed property is useful in practice.

In my case I needed it to determine whether a Type can contain null values, so basically this:

public static bool IsNullable(this Type type)
{
    return type.IsReferenceType || type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>);
}
  • It is not clear how the property should behave exactly, including all corner cases.

I think the only question was regarding by-ref types, and @steveharter answered that. It would be well defined for generic type parameters - it should return true if the type parameter has the class constraint or a constraint as deriving from a specific class - just like IsValueType returns true for a type parameter if it has the struct constraint.

@jkotas
Copy link
Member

jkotas commented Sep 3, 2023

To demonstrate general usefulness of an API proposal, it is best to share pointers to somebody else's existing code in this repo or elsewhere on github that would benefit from the API.

just like IsValueType returns true for a type parameter if it has the struct constraint.

This behavior is not necessarily a good prior art. No other System.Type property takes the generic constraints into account this way. The behavior of IsValueType property for generic parameters is questionable for corner cases. It happens to be ok for types that can be expressed in C# today, but it is not quite right for valid types expressible in IL in general.

For example, consider this:

using System.Reflection;
using System.Reflection.Emit;

var ab = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("MyAssembly"), AssemblyBuilderAccess.Run);
var mb = ab.DefineDynamicModule("MyModule");

var tb = mb.DefineType("MyType");
var gb = tb.DefineGenericParameters("T")[0];
gb.SetBaseTypeConstraint(typeof(ValueType));

var genericType = tb.CreateType();
var genericParameter = genericType.GetGenericArguments()[0];
// This prints `true` (not quite what one would expect) ...
Console.WriteLine(genericParameter.IsValueType);

var instantiatedGenericType = genericType.MakeGenericType(typeof(ValueType));
var instantiatedGenericParameter = instantiatedGenericType.GetGenericArguments()[0];
// ... but this prints false.
Console.WriteLine(instantiatedGenericParameter.IsValueType);

@Joe4evr
Copy link
Contributor

Joe4evr commented Sep 4, 2023

ECMA-335 states that at the highest level there are only value types and reference types, so IsReferenceType would be the same as !ValueType.

Does ECMA not say anything about pointer types? Because those are neither reference nor value types, and Type has a distinguishing property for those.

Related: @Neme12 Remember that pointer types can also be assigned null, in case that can be a thing in whatever you're doing.

@jkotas
Copy link
Member

jkotas commented Sep 4, 2023

Does ECMA not say anything about pointer types? Because those are neither reference nor value types

This is true for C# definition of reference types.

ECMA 335 has different definition of reference types that considers pointer type to be a reference type:

I.8.2.1 Value types and reference types
There are two kinds of types: value types and reference types.

  • Value types – The values described by a value type are self-contained (each can be understood without reference to other values).
  • Reference types – A value described by a reference type denotes the location of another value. There are four kinds of reference type:
    • An object type is a reference type of a self-describing value (see §I.8.2.3). Some object types (e.g., abstract classes) are only a partial description of a value.
    • An interface type is always a partial description of a value, potentially supported by many object types.
    • A pointer type is a compile-time description of a value whose representation is a machine address of a location. Pointers are divided into managed (§I.8.2.1.1, §I.12.1.1.2) and unmanaged (§I.8.9.2).

Also, ECMA 335 has second different definition of a reference type for the purposes of assignment compatibility:

A type T is a reference type if and only if one of the following holds.

  1. T is a possibly-instantiated object, delegate or interface of the form N<T1,…,Tn> (n ≥ 0)
  2. T is an array type
    [Note: generic parameters are not reference types. ...]

@Neme12
Copy link
Author

Neme12 commented Sep 4, 2023

Related: @Neme12 Remember that pointer types can also be assigned null, in case that can be a thing in whatever you're doing.

Thanks, I'll make sure that the IsNullableType helper handles that correctly, even though I won't actually encounter pointer types in practice, because this is all for a TypeConverter, which uses object, and as far as I know, pointers can't be boxed.

@Neme12
Copy link
Author

Neme12 commented Sep 4, 2023

To demonstrate general usefulness of an API proposal, it is best to share pointers to somebody else's existing code in this repo or elsewhere on github that would benefit from the API.

This is the existing code I have to write:

public static bool IsReferenceType(this Type type)
{
    if (type.IsGenericParameter)
    {
        var attributes = type.GenericParameterAttributes;
        if (attributes.HasFlag(GenericParameterAttributes.ReferenceTypeConstraint))
            return true;

        var constraints = type.GetGenericParameterConstraints();
        if (constraints.Any(type => type != typeof(ValueType) && !type.IsInterface && type.IsReferenceType()))
            return true;

        return false;
    }

    return !type.IsValueType;
}

The behavior of IsValueType property for generic parameters is questionable for corner cases. It happens to be ok for types that can be expressed in C# today, but it is not quite right for valid types expressible in IL in general.

I'm not too worried about that because people don't write IL in practice. Also, couldn't this behavior be fixed? It seems like a corner case that wouldn't actually break anyone in practice if it got changed.

...
I only now noticed that there is already an IsClass property. It doesn't work though - it returns true for an unconstrained type parameter, which doesn't have to be a class at all.

@steveharter
Copy link
Member

I don't think IsReferenceType is common enough to be included. There have been no other similar asks AFAIK.

Also, if we did this, we would need to consider whether we should also update the semantics of IsValueType, IsClass and IsInterface when used as a generic parameter and those would be breaking changes. And if we did update those semantics, then perhaps we wouldn't need IsReferenceType anymore.

@Neme12 can you elaborate on your reflection usage of it - are you trying to invoke a member and\or close an open generic parameter, generating code from reflection metadata, or something else?

Some misc searches for ".GetGenericParameterConstraints(" didn't yield anything.

@steveharter
Copy link
Member

Based on my previous reply, an alternative solution then would be to update IsValueType, IsClass and IsInterface so that they respect constraints.

@steveharter steveharter added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 2, 2023
@ghost
Copy link

ghost commented Nov 2, 2023

This issue has been marked needs-author-action and may be missing some important information.

@ghost ghost added the no-recent-activity label Nov 17, 2023
@ghost
Copy link

ghost commented Nov 17, 2023

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Dec 1, 2023

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@ghost ghost closed this as completed Dec 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2023
@ghost ghost removed the no-recent-activity label Dec 31, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

No branches or pull requests

5 participants