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]: TypeName.MakeSimpleTypeName(AssemblyNameInfo) to avoid type name reparsing #106022

Closed
adamsitnik opened this issue Aug 6, 2024 · 4 comments · Fixed by #103713
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it blocking Marks issues that we want to fast track in order to unblock other important work in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@adamsitnik
Copy link
Member

adamsitnik commented Aug 6, 2024

Background and motivation

In #102263, we have approved a set of APIs for creating new TypeName instances without reparsing the input again.

I took a stab at it in #103713 and realized that I can not use the approved TypeName.CreateSimpleTypeName because it expects unescaped input. It's also supposed to escape such input (#102263 (comment)). In order to find out whether it needs to escape the input, it needs to reparse the input (search for multiple characters that need to be escaped). It's exactly what I need to avoid in NRBF, so I need a new method.

API Proposal

namespace System.Reflection.Metadata;

public partial class TypeName
{      
    public TypeName MakeSZArrayTypeName();
    public TypeName MakeArrayTypeName(int rank);
    public TypeName MakePointerTypeName();
    public TypeName MakeByRefTypeName();
    public TypeName MakeGenericTypeName(ImmutableArray<TypeName> typeArguments);
+   public TypeName MakeSimpleTypeName(AssemblyNameInfo? assemblyInfo);

    public static TypeName CreateSimpleTypeName(
        string metadataName,
        TypeName? declaringType = null,
        AssemblyNameInfo? assemblyInfo = null);
}

API Usage

static TypeName WithAssemblyName(this TypeName typeName, AssemblyNameInfo assemblyName)
{
    if (!typeName.IsSimple)
    {
        if (typeName.IsArray)
        {
            TypeName newElementType = typeName.GetElementType().WithAssemblyName(assemblyName);

            return typeName.IsSZArray
                ? newElementType.MakeSZArrayTypeName()
                : newElementType.MakeArrayTypeName(typeName.GetArrayRank());
        }
        else if (typeName.IsConstructedGenericType)
        {
            TypeName newGenericTypeDefinition = typeName.GetGenericTypeDefinition().WithAssemblyName(assemblyName);

            // We don't change the assembly name of generic arguments on purpose.
            return newGenericTypeDefinition.MakeGenericTypeName(typeName.GetGenericArguments());
        }
        else
        {
            // BinaryFormatter can not serialize pointers or references.
            ThrowHelper.ThrowInvalidTypeName(typeName.FullName);
        }
    }

    return typeName.MakeSimpleTypeName(assemblyName);
}

Alternative Designs

No response

Risks

Having two methods with similar names: MakeSimpleTypeName and CreateSimpleTypeName may be confusing to the end users.

@adamsitnik adamsitnik added area-System.Reflection.Metadata api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 6, 2024
@adamsitnik adamsitnik self-assigned this Aug 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Aug 6, 2024
@adamsitnik adamsitnik added blocking Marks issues that we want to fast track in order to unblock other important work and removed untriaged New issue has not been triaged by the area owner labels Aug 6, 2024
@ericstj ericstj added the binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it label Aug 6, 2024
@jkotas
Copy link
Member

jkotas commented Aug 6, 2024

I can not use the approved TypeName.CreateSimpleTypeName because it expects unescaped input.

I think you can use it. I agree that TypeName.CreateSimpleTypeName is a less efficient in the BF use case when compared to the API proposed here, but there is no fundamental problem doing what the BF needs to achieve using the approved APIs.

@terrajobst
Copy link
Member

terrajobst commented Aug 6, 2024

Video

  • Looks good as proposed
namespace System.Reflection.Metadata;

public partial class TypeName
{
    public TypeName MakeSimpleTypeName(AssemblyNameInfo? assemblyInfo);
}

@terrajobst terrajobst 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 Aug 6, 2024
@adamsitnik adamsitnik removed the blocking Marks issues that we want to fast track in order to unblock other important work label Aug 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 6, 2024
@ericstj ericstj added this to the 9.0.0 milestone Aug 6, 2024
@adamsitnik
Copy link
Member Author

Based on online (#103713 (comment)) and offline discussions with @jkotas I would like to discuss renaming the method from MakeSimpleTypeName to WithAssemblyName.

namespace System.Reflection.Metadata;

public partial class TypeName
{
    public TypeName WithAssemblyName(AssemblyNameInfo? assemblyInfo);
}

@adamsitnik adamsitnik added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Aug 13, 2024
@terrajobst
Copy link
Member

terrajobst commented Aug 13, 2024

Video

  • Looks good as proposed
namespace System.Reflection.Metadata;

public partial class TypeName
{
    public TypeName WithAssemblyName(AssemblyNameInfo? assemblyInfo);
}

@terrajobst terrajobst 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 Aug 13, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
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.Reflection.Metadata binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it blocking Marks issues that we want to fast track in order to unblock other important work in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants