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]: Create array from array type #76478

Closed
Tracked by #79053
jkotas opened this issue Oct 1, 2022 · 9 comments
Closed
Tracked by #79053

[API Proposal]: Create array from array type #76478

jkotas opened this issue Oct 1, 2022 · 9 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Oct 1, 2022

Background and motivation

The existing reflection APIs for creating array instances take array element type. If one has the actual array type available, this is both inefficient (requires going from the array type to the element type and back to the array type that we have started with) and requires suppressing warnings about the array type potentially not being available with Aot.

The proposal is the introduce a set of APIs that mirror the existing APIs for creating array instances, except that they take the array type instead of the element type.

API Proposal

namespace System;

public class Array
{
        // Proposed APIs
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2, int length3);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params int[] lengths);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int[] lengths, int[] lowerBounds);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params long[] lengths);

        // Existing APIs
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, int length);
        public static System.Array CreateInstance(System.Type elementType, int length1, int length2);
        public static System.Array CreateInstance(System.Type elementType, int length1, int length2, int length3);
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, params int[] lengths);
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, int[] lengths, int[] lowerBounds);
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, params long[] lengths);
}

API Usage

We can replace Array.CreateInstance(arrayType.GetElementType()!, length) pattern in number of places in dotnet/runtime libraries with Array.CreateInstanceFromArrayType(arrayType, length):

Alternative Designs

Introduce overload for single dimensional arrays only. The single dimensional arrays are likely the 99+% use case for this API.

Risks

No response

@jkotas jkotas added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 1, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 1, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas jkotas added area-System.Runtime and removed untriaged New issue has not been triaged by the area owner labels Oct 1, 2022
@ghost
Copy link

ghost commented Oct 1, 2022

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

Issue Details

Background and motivation

The existing reflection APIs for creating array instances take array element type. If one has the actual array type available, this is both inefficient (requires going from the array type to the element type and back to the array type that we have started with) and requires suppressing warnings about the array type not being present with Aot.

The proposal is the introduce a set of APIs that mirror the existing APIs for create array instances, except that they take the array type instead of element type.

API Proposal

namespace System;

public class Array
{
        // Proposed APIs
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2, int length3);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params int[] lengths);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int[] lengths, int[] lowerBounds);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params long[] lengths);

        // Existing APIs
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, int length);
        public static System.Array CreateInstance(System.Type elementType, int length1, int length2);
        public static System.Array CreateInstance(System.Type elementType, int length1, int length2, int length3);
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, params int[] lengths);
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, int[] lengths, int[] lowerBounds);
        [System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
        public static System.Array CreateInstance(System.Type elementType, params long[] lengths);
}

API Usage

We can replace Array.CreateInstance(arrayType.GetElementType()!, length) pattern in number of places in dotnet/runtime libraries with Array.CreateInstanceFromArrayType(arrayType, length):

Alternative Designs

Introduce overload for single dimensional arrays only. The single dimensional arrays are likely the 99+% use case for this API.

Risks

No response

Author: jkotas
Assignees: -
Labels:

api-suggestion, area-System.Runtime

Milestone: -

@jkotas
Copy link
Member Author

jkotas commented Oct 1, 2022

cc @dotnet/ilc-contrib Does this look reasonable?

@teo-tsirpanis

This comment was marked as resolved.

@jkotas jkotas added this to the 8.0.0 milestone Oct 1, 2022
@MichalStrehovsky
Copy link
Member

cc https://github.com/orgs/dotnet/teams/ilc-contrib Does this look reasonable?

Looks reasonable to me! I'll leave it up to the API review board to think about the naming or the location of the API (we could also consider stashing it somewhere else, such as on Activator (add another overload of CreateInstance?), or RuntimeHelpers (GetUninitializedArray?).

@MichalStrehovsky MichalStrehovsky added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 4, 2022
@bartonjs
Copy link
Member

bartonjs commented Feb 28, 2023

Looks good as proposed.

The implementer is encouraged to consider whether all of these overloads are actually required, as the suggestion that the first overload will be in excess of 99% of all calls seems correct.

namespace System;

public partial class Array
{
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2, int length3);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params int[] lengths);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int[] lengths, int[] lowerBounds);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params long[] lengths);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 28, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 10, 2023
@AlexRadch
Copy link
Contributor

The implementer is encouraged to consider whether all of these overloads are actually required, as the suggestion that the first overload will be in excess of 99% of all calls seems correct.

@bartonjs I decided to implement only the next overloads:

namespace System;

public partial class Array
{
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params int[] lengths);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int[] lengths, int[] lowerBounds);
}

And I have not implemented the following overloads:

namespace System;

public partial class Array
{
        // It is not used in the current code and overload with `params int[] lengths` can be used instead
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2); 

        // It is not used in the current code and overload with `params int[] lengths` can be used instead
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2, int length3);

        // Array does not support lengths more than `int.Max`. Array always uses `int` for lengths, not `long`
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params long[] lengths);
}

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2023
@tannergooding tannergooding modified the milestones: 8.0.0, Future Jul 24, 2023
@tannergooding
Copy link
Member

Not going to land in .NET 8. This can effectively be done at any time after main opens for .NET 9 next month

@jkotas
Copy link
Member Author

jkotas commented Nov 16, 2023

Fixed by #88620

@jkotas jkotas closed this as completed Nov 16, 2023
@adamsitnik adamsitnik modified the milestones: Future, 9.0.0 Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

No branches or pull requests

7 participants