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]: MetadataReader.GetAssemblyName(string assemblyFile) #63960

Closed
VSadov opened this issue Jan 18, 2022 · 8 comments · Fixed by #71801
Closed

[API Proposal]: MetadataReader.GetAssemblyName(string assemblyFile) #63960

VSadov opened this issue Jan 18, 2022 · 8 comments · Fixed by #71801
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection.Metadata
Milestone

Comments

@VSadov
Copy link
Member

VSadov commented Jan 18, 2022

Background and motivation

As part of #63915 we are moving AssemblyName.GetAssemblyName(string assemblyFile) implementation to managed code, so we need a helper entry point in System.Reflection.Metadata.

Current implementation relies on VM assembly loader, but intentionally bypasses the internal caches so that the call has no lasting effects on the runtime state.
The implementation also needs to handle assemblies that do not match the current platform, which is an awkward situation for the assembly loader and require special handling.
As it is, every runtime (CoreClr, Mono, CoreRT) needs to implement AssemblyName.GetAssemblyName leading to inconsistencies in implementation. Providing a common managed implementation is very desirable.

Since this is all about PE/Assembly parsing, System.Reflection.Metadata appears to be a logical place for the implementation.

While internal helper could be sufficient, it is better if the helper is public. In fact the helper may become the primary/recommended way to fetch an assembly name.

API Proposal

namespace System.Reflection.Metadata
{
   public sealed partial class MetadataReader
    {
        // given a path to assembly returns its AssemblyName similarly to AssemblyName.GetAssemblyName
        // in fact AssemblyName.GetAssemblyName will use this implementation.
+       public static AssemblyName GetAssemblyName(string assemblyFile) { ... }
    }
}

== Exceptions:
AssemblyName.GetAssemblyName handles failure cases by throwing a range of exceptions. It may be difficult to match every error case precisely. Inconsistencies in the file format typically lead to BadImageFormatException.
There is less consistency for other issues like file/IO and there are some disagreements between platforms and different OSs.

== Proposal:
Map InvalidOperationException coming from MetadataReader to BadImageFormatException with the same message to minimize compat concerns. Propagate other exceptions as-is.

API Usage

using System.Reflection.Metadata;

AssemblyName assemblyName = MetadataReader.GetAssemblyName("Hello.dll");

Alternative Designs

It would be ok if the helper is internal, but measures will need to be taken that it is not trimmed by the linker and does not disappear in future/alternative implementations of System.Reflection.Metadata

Risks

Low. This is a new API entry point exposing existing implementation.

@VSadov VSadov added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 18, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Reflection.Metadata untriaged New issue has not been triaged by the area owner labels Jan 18, 2022
@ghost
Copy link

ghost commented Jan 18, 2022

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

Issue Details

Background and motivation

.

API Proposal

namespace System.Collections.Generic
{
    public class MyFancyCollection<T> : IEnumerable<T>
    {
        public void Fancy(T item);
    }
}

API Usage

// Fancy the value
var c = new MyFancyCollection<int>();
c.Fancy(42);

// Getting the values out
foreach (var v in c)
    Console.WriteLine(v);

Alternative Designs

No response

Risks

.

Author: VSadov
Assignees: -
Labels:

api-suggestion, area-System.Reflection.Metadata, untriaged

Milestone: -

@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Jan 18, 2022
@VSadov VSadov changed the title [API Proposal]: System.Reflection.Metadata.MetadataReader.GetAssemblyName(string assemblyFile) [API Proposal]: MetadataReader.GetAssemblyName(string assemblyFile) Jan 18, 2022
@VSadov VSadov added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jan 18, 2022
@VSadov
Copy link
Member Author

VSadov commented Jan 19, 2022

CC: @agocke

@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Jan 26, 2022
@buyaa-n buyaa-n added this to the 7.0.0 milestone Jan 26, 2022
@teo-tsirpanis teo-tsirpanis removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 8, 2022
@terrajobst
Copy link
Member

terrajobst commented Apr 12, 2022

Video

  • Looks good as proposed
    • Assuming @buyaa-n is also fine with this shape.
namespace System.Reflection.Metadata;

public sealed partial class MetadataReader
{
    public static AssemblyName GetAssemblyName(string assemblyFile);
}

@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 Apr 12, 2022
@buyaa-n
Copy link
Member

buyaa-n commented Apr 12, 2022

Yest I was OK with the API shape/name because @VSadov moved the existing API implementation from AssemblyName into MetadataReader, and the new API naming just same as the one in AssemblyName

public static AssemblyName GetAssemblyName(string assemblyFile)

But I believe it is not have to be same as one in AssemblyName, so I am open for better naming suggestions

@terrajobst
Copy link
Member

Personally, I think assemblyFile is fine, but so is assemblyPath or just path.

@dotnet/fxdc, any preferences?

@teo-tsirpanis
Copy link
Contributor

What I can't understand is how this method would be called from corelib, if that's what you want to do. System.Reflection.Metadata is in a separate library, and a sizable one. Or do you want to obsolete AssemblyName.GetAssemblyName and direct developers to the new one?

@jkotas
Copy link
Member

jkotas commented Apr 13, 2022

What I can't understand is how this method would be called from corelib,

It is called from corelib via reflection today:

return (s_getAssemblyName ?? InitGetAssemblyName())(assemblyFile);
. We have number of similar places in corelib where we call methods higher in the stack via reflection.

The reason to make the API public in the new location is to make this explicit. Alternative designs paragraph above talks about it.

@buyaa-n
Copy link
Member

buyaa-n commented Apr 20, 2022

[Triage:] we are OK with keeping the name as is, to express that it is same API as the one in AssemblyName and seems nobody from @dotnet/fxdc has other preferences

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 11, 2022
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
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants