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]: Creating new TypeName with given AssemblyNameInfo #102263

Open
adamsitnik opened this issue May 15, 2024 · 7 comments
Open

[API Proposal]: Creating new TypeName with given AssemblyNameInfo #102263

adamsitnik opened this issue May 15, 2024 · 7 comments
Assignees
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Reflection.Metadata
Milestone

Comments

@adamsitnik
Copy link
Member

Background and motivation

As we have started using the new TypeName APIs all over the place, I've realized that it would be very nice to have the ability to create a new TypeName instance with given AssemblyNameInfo without the need of re-parsing the concatenated input again.

For example, in BinaryFormatter the type and library names are provided separately (#102014 (comment)), I would like to join them without re-parsing the whole thing again.

API Proposal

namespace System.Reflection.Metadata;

public sealed class TypeName
{
    /// <summary>
    /// Creates a new TypeName with provided assembly name.
    /// </summary>
    public TypeName With(AssemblyNameInfo newAssemblyNameInfo);
}

API Usage

TypeName WithoutAssemblyVersion(TypeName typeName) 
    => typeName.With(
        new AssemblyNameInfo(
            typeName.AssemblyName.Name,
            version: null,
            typeName.AssemblyName.CultureName,
            typeName.AssemblyName.Flags,
            typeName.AssemblyName.PublicKeyOrToken));

Alternative Designs

The name could be changed to sth that indicates that it creates and returns a new instance.

Risks

Some users may miss the fact that the API creates a new instance of TypeName rather than mutating the current instance.

@adamsitnik adamsitnik added area-System.Reflection.Metadata api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 15, 2024
@adamsitnik adamsitnik added this to the 9.0.0 milestone May 15, 2024
@adamsitnik adamsitnik self-assigned this May 15, 2024
Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented May 15, 2024

Should we rather address the more general problem of creating TypeName of any shape through constructors or factory methods?

It happens that the AssemblyName case showed up with the binary formatter. I think it is a tell-take of a general need for creating arbitrary TypeNames.

@adamsitnik
Copy link
Member Author

Should we rather address the more general problem of creating TypeName of any shape through constructors or factory methods?

All TypeName ctors that are going to accept strings of any kind are going to need to be fully validated, which is almost like reparsing them. And this is something that I would like to avoid with the provided API.

I am not saying no, I just don't have a clear vision of how these APIs would look like.

@jkotas
Copy link
Member

jkotas commented May 16, 2024

All TypeName ctors that are going to accept strings of any kind are going to need to be fully validate

Why do the strings need to be validated? Type names can be anything.

@jkotas
Copy link
Member

jkotas commented May 16, 2024

Ah, it is because the APIs returned escaped names. The escaping validation should be very cheap, much cheaper than parsing.

The more I see the issues with the name escaping, the more I think that it was a bad design decision to make the Name and FullName properties to match reflection behavior. Should we revisit that?

The APIs can look like this:

public static TypeName MakeSimpleTypeName(string fullName, TypeName? declaringType = null, AssemblyNameInfo? assemblyInfo = null);
public static TypeName MakeArrayTypeName(TypeName elementType);
public static TypeName MakeArrayTypeName(TypeName elementType, int rank);
public static TypeName MakePointerTypeName(TypeName elementType);
public static TypeName MakeByRefTypeName(TypeName elementType);
public static TypeName MakeGenericTypeName(TypeName genericTypeDefinition, ImmutableArray<TypeName> typeArguments);

// Matches Unscape method proposed in #101774
public static string Escape(string name);

@adamsitnik
Copy link
Member Author

the more I think that it was a bad design decision to make the Name and FullName properties to match reflection behavior

But it allows for things like if (typeName.FullName == typeof(T).FullName) which already proved very useful in payload reader.

I know it may sound silly but IMO we would not have that problem if we would simply forbid using certain characters without the ability to escape them in the first place.

@jkotas
Copy link
Member

jkotas commented May 16, 2024

if (typeName.FullName == typeof(T).FullName)

Yeah, that's the conundrum when you want to use this together with reflection. It is hard to tell how common this use case is going to be.

Anyway, I edited my suggestion at #102263 (comment) to include Escape method, so that there is a reasonable path when you have the raw name.

we would not have that problem if we would simply forbid using certain characters without the ability to escape them in the first place.

It would produce non-compliant type name parser. It may be ok for BF-specific parser. I do not think it is ok for S.R.Metadata or the runtime that need compliant and backward compatible type name parsers. It would be a needless breaking change to drop support for escaped type names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Reflection.Metadata
Projects
None yet
Development

No branches or pull requests

2 participants