Add API to get an AssemblyName that doesn't throw on a native binary

[ericstj]

I've seen patterns like this in countless tools:

try
{
    return AssemblyName.GetAssemblyName(sourcePath);
}
catch(BadImageFormatException)
{
    return null;
}

It would be better if we could just do:

AssemblyName name;
return AssemblyName.TryGetAssemblyName(sourcePath, out name) ? name : null;

API signature:

  public static bool TryGetAssemblyName(string assemblyFile, out AssemblyName assemblyName);

By making this a new API we don't change the behavior of the existing API. By making the API determine if a file is an assembly and returning the name we avoid having to open the file twice.

The other alternative available today requires using System.Reflection.Metadata which is not part of netstandard and requires redistributing a sizeable dependency (800KB), or writing your own PE parsing code (which requires opening the file twice).

/cc @gkhanna79

[danmoseley]

There is an issue somewhere with a proposed implementation, but I cannot find it right now.

[danmoseley]

@atsushikan do you support this proposal? If so please add api-ready-for-review label.

[ghost]

A couple of questions:

• You're only suppressing the PE-not-a-managed-assembly exception, not any other exception, right?

• Do we need a bool/out signature for this? We could use a null AssemblyName as a sentinel.

• I'd like to see that proposed implementation.

[ericstj]

It should throw for things like a null path, malformed path, a non-existent file, sharing violation when reading the file. If its successfully able to open the file then I'd expect it to not throw.

Returning null as sentinel seems reasonable. Ultimately this one seems like an FxDC nit.

I didn't have the proposed implementation. Perhaps its over in CoreCLR repo?

[ghost]

Ok, marked api-ready-for-review. Not implying I'm signing up for the implementation :-)

[karelz]

@atsushikan as area owner you are responsible for the API surface - doing first level API review, making sure it is in line with area plans.
Once the API is approved, area owners are responsible to mark it as up-for-grabs (if appropriate) with brief notes on complexity and hint of next steps as for any other issue - see triage rules.
When there is contributor interested in grabbing the issue, area owners are responsible to shepherd the effort, incl. guidance and code review. If the time investment is not appropriate for a particular issue, you can always discuss options/guidance with CoreFX leads (me and Dan) or your lead.

Area owners are NOT responsible to implement every single bug fix / API request.
However, area owners are expected to prioritize their area and make sure we keep high quality in the area via fixing the right bugs and adding/implementing the right APIs at the right moment.

Hope that makes sense :)

[karelz]

API review: (channeling @KrzysztofCwalina's insights from history)
Historically, we used Try* only for perf reasons. This is likely not the case here (opening assembly hits disk already), so exception is small contributor. @ericstj can you please confirm it is not perf? (having some data would help)

If it is syntax of exception handling, we should talk to Roslyn team.
If it is about debuggability, then we are on slippery slope road - we rejected Try* pattern in the past in such cases (mainly due to "how common is it to warrant Try*). If we want to change our guidance in general, we should discuss that.

Actionable items:

1. What is the primary motivation of the API? Perf vs. syntax vs. debuggability?
2. We should discuss general guidance on exceptions from BCL in each of those cotnexts - @KrzysztofCwalina will present & organize (with C# / VS Debugger teams in the room)

Deferring decision until we have more info.

[ericstj]

Primary driver was debuggability. Current API will result in an exception even in the normal case.

See dotnet/standard#326 /cc @KirillOsenkov to add his opinion.

Note that Try isn't a requirement in the method name. We could just as well have a different method name that returns null instead of an AssemblyName (as Ati suggested). We don't want the existing API to do that because it'd be a breaking change. So an alternative suggestion would be
AssemblyName.GetAssemblyName(string assemblyFile, bool throwOnInvalidAssembly).

[KirillOsenkov]

Yes, debuggability is really bad. I think we should rethink the approach of not having Try methods just because they let us avoid a first-chance exception.

I should write a blog post about first-chance exceptions, a lot of people don't realize a few important things there. Basically in a codebase where you're disciplined about first-chance exceptions there's a magnificent way to find hidden bugs - have a FirstChanceException handler with a whitelist of known benign ones. This catches bugs like crazy.

If the framework itself doesn't play along, it renders this entire class of bug finding ineffective. I can't tell you how many bugs and crashes in Roslyn we've prevented because we adopted this policy. It's hard to overstate.

[KrzysztofCwalina]

The .NET platform was designed in a way that it does not play along with first change exceptions debugging. The technique works in C++ because of a completely different philosophy for the design of error reporting (more error codes).

I am skeptical that we can make it first chance exceptions friendly 20 years after it was conceived. Every now and there there are people who push for various local "fixes" that IMO only make the platform inconsistent without actually solving the problem, i.e we won't be able to undo the exceptions we already throw.

[davkean]

@KrzysztofCwalina But you can move the platform forward by adding non-throwing versions of APIs - and new consumers will use them by default, and existing consumers will change to use as they churn those areas. No difference from having int.Parse in v1 and adding TryParse in future versions.

If it is about debuggability, then we are on slippery slope road - we rejected Try* pattern in the past in such cases (mainly due to "how common is it to warrant Try*). If we want to change our guidance in general, we should discuss that.

This doesn't match my recollection, nor past usage, here's a couple of examples:

https://docs.microsoft.com/en-us/dotnet/api/system.threading.thread.trysetapartmentstate?view=netframework-4.7#System_Threading_Thread_TrySetApartmentState_System_Threading_ApartmentState_
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.portableexecutable.pereader.tryopenassociatedportablepdb?view=netcore-2.0#System_Reflection_PortableExecutable_PEReader_TryOpenAssociatedPortablePdb_System_String_System_Func_System_String_System_IO_Stream__System_Reflection_Metadata_MetadataReaderProvider__System_String__

You could also put "File.Exists" in the same bucket here - as it doesn't leak any exceptions, but swallows them for convenience.

[nguerrera]

have a FirstChanceException handler with a whitelist of known benign ones. This catches bugs like crazy.

IMHO, this catching bugs is indicative of catching all exceptions way too aggressively. Sadly, most programs are very, very guilty of that.

But I do think it's fine to offer non-throwing alternatives as a debugging aid and not just for perf.

[davkean]

Another example is the CanXXX, SupportsXXX light-up APIs vs. catching InvalidOperationException or NotSupportedException.

[agocke]

I think it's important to note that the goal is not to eliminate exceptions, it's to eliminate exceptions on non-exceptional control paths. If a call is expected, and allowed, to fail it would be far better to utilize some other error mechanism.

[KirillOsenkov]

Exactly. It's about non-avoidable exceptions vs. easily avoidable exceptions. I only want to reduce the easily avoidable exceptions like when Guid.TryParse was added to complement Guid.Parse.

[KrzysztofCwalina]

It's like the 10th time we are rehashing this. It always ends with people proposing the mythical "exceptional condition" and then not being able to define what it means. The reason is that whether some error is exceptional or not depends on the call site, not on the API being called. parsing a literal int is very different than parsing an int from a file, yet these two operations use the same API.

Secondly, as I said before, we already have throwing APIs, and adding Try alternatives will not make them go away, programs will keep throwing, and debug-ability will be affected. At the same time we will introduce lots of inconsistencies, duplication, and problems that come with error codes.

I do think we should improve C# syntax for exception handling, and to improve how first change exception debugging works (with some ideas mentioned in this thread, e.g. better filtering), but as to whether we use exceptions or error codes for errors in .NET, IMO the ship has sailed.

[KirillOsenkov]

Keep in mind, I'm not advocating for error codes, heaven forbid. I'm not advocating for anything drastic.

All I'm saying is that when we introduced Guid.TryParse, it was a wonderful thing, it was the right thing to do, and when VS switched to using it in a few places it was a significant improvement in the quality of life of people debugging. Every little bit helps. It's OK that the old APIs stay. But we want to have a choice in case an exception is easily avoidable and really expected for a quite common class of inputs.

[KrzysztofCwalina]

Try* pattern is a poors-man-error-code; it has very similar effect on the platform. We decided to use error codes/try in rare circumstances when exceptions are too slow, and accept the negatives, because both: a) there is no known solution to perf problem, b) the perf problem is limited to a small set of APIs (e.g. low level parsing routines, basic datastructure accessors, etc.). The debug-ability problem is not limited to any particular API.

[jairbubbles]

As it's a language wide problem don't we need a new tool to achieve that ?

For instance, a new keyword swallow that will never trigger the debugger. Of course this would be dangerous and should be used with precaution.

try
{
return AssemblyName.GetAssemblyName(sourcePath);
}
swallow(BadImageFormatException)
{
    return null;
}

Depending one the type of exceptions swallowed, static analyzers could warn the programmer.

[aKzenT]

There are various methods in the framework that can throw a BadImageFormatException if the file is not a valid assembly.

Instead of adding Try-overloads to all these functions, wouldn't it be better to have generic function (IsValidAssembly, CheckAssembly, ...) that checks if a file represents a valid assembly.

Just like I might do a check to File.Exists before calling GetAssemblyName, I could also check this new method to differentiate between a valid assembly and some random file.

Of course this has the disadvantage of having to open the file various times, but if that is a concern, you can always go back to try-catch.

[danmoseley]

@nguerrera

IMHO, this catching bugs is indicative of catching all exceptions way too aggressively. Sadly, most programs are very, very guilty of that.

VS (and other UI apps I've worked on) catch everything - I imagine that will always be the case. It used to be that breaking on first chance exceptions in VS would find bugs - unfortunately some components throw them excessively now. First chance can also be helpful when the exception is caught and rethrown asynchronously. For console apps you are probably right.

[danmoseley]

The .NET platform was designed in a way that it does not play along with first change exceptions debugging.

This isn't my experience (just anecdata of course). At least with just my code switched on. Even without JMC, there are often only a few exception types that throw in the "normal" case and those you can filter out.

Tooling and language improvements have a long lead time. I don't think if we offer a Try version of this method, it will lead to some slippery slope. We can have a budget of just a few a year :)

[jnm2]

Just like I might do a check to File.Exists before calling GetAssemblyName

File.Exists is vulnerable to race conditions- just embrace it and catch the exception at whatever level is responsible to know how it wants to respond to it.

anecdata

I'm stealing this. 😆

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/W_r6oG7nnK4?t=680 (15 min duration)

[spottedmahn]

For me, it is syntax and debug-ability. Interesting discussion in API review.

[KrzysztofCwalina]

Yeah, I agree with have a problem with syntax and to some extent debug ability. It would be good to fix both. @jaredpar and I worked on a project trying to address the syntax issues, and I think it was very promissing. For example, what if you could do:

x = try MethodThatThrows() else 5; 

// or
try MethodThatThrows() else SomeOtherMethod();

[jairbubbles]

For the debug-ability issue maybe we should allow attributes on catch statements, allowing to give further information to debugger (similar to DebuggerStepThroughAttribute).

That being said I'll let smart people make the necessary modifications 😸

[spottedmahn]

This discussion seems similar to Enum.Parse() and Enum.TryParse(), no? I was very happy when Enum.TryParse() was introduced 😄

[jnm2]

Relevant: dotnet/csharplang#220, dotnet/csharplang#464, dotnet/csharplang#786, dotnet/roslyn#16072, https://github.com/dotnet/roslyn/issues/16078.

My opinion on it, like many others’, has always been negative: dotnet/roslyn#16072 (comment)

[jairbubbles]

I feel like whe should focus on the debug-ability problem. Maybe in another issue?

TryGetAssemblyName is not going to happen as there's no performance issue with the try / catch pattern. Anyone can just add a new function which encapsulates it, everyone is happy, it's readable and the performances are the same. Moreover GetAssemblyName is clearly less used than Enum.TryParse or int.TryParse so we should not compare.

Remains the debug-ability issue, hence "my not going to happen" proposal.

[danmoseley]

@KrzysztofCwalina how would you feel about AssemblyName.GetAssemblyName(string assemblyFile, bool throwOnInvalidAssembly). ? It's Try under another name, but at least it wouldn't establish a precedent.

[KrzysztofCwalina]

If the only debugability problem we have in the Framework is throwing GetAssemblyName, then we should just add whether we need/want. But I think the issue is general; it's not just about GetAssemblyName.

[DavesApps]

@KrzysztofCwalina I had a similar idea of avoiding all of these TryParse like methods, that jnm2 referenced about statement level exception handling dotnet/csharplang#965

Take a look there are potential improvements for:
syntax
debuggability
performance.
avoids hiding exceptions

[HaloFour]

@KrzysztofCwalina

This conversation does belong elsewhere but my concern with amending the language to make this simpler is that the majority of developers won't know when not to use this. It'll not only make ignoring exceptions much easier than it is currently, it'll make ignoring exceptions much easier than having to dealing with them. I worry that enshrining such behavior directly in the language would serve as encouragement that this is the appropriate pattern to adopt for exception handling in general.

Also, why isn't there a corresponding issue for this on https://github.com/dotnet/csharplang/? If @jaredpar is actively working on it enough to build out a prototype it would seem "championed" enough.

[jnm2]

Another thing occurred to me last night. The Try* pattern does not mean "return false rather than allowing any exception to bubble through." You return false for tightly-defined straightforward criteria and still might throw for exceptional situations. A trivial example is ArgumentException; a more realistic one could be certain IOExceptions when TryParsing a stream or path.

Conflating the Try* pattern with catch-all is a dangerous precedent for the reasons @HaloFour mentioned and other semantic responsibility reasons which I've discussed before. Catch-all is a pit of failure and only belongs in places like bare threads, tasks, message loops, and other top-level handlers, and only then if they do not keep the software operator and the developer in the dark as to possible corruption in unforseen scenarios.

[CyrusNajmabadi]

With VS a lot of the first chance exceptions thrown (usually thousands of them) all boiled down to a small set of actual methods that did not have TryXXX versions. I don't think a general solution is needed. I think we could actually get quite far just by targetting the few bad cases, and updating the higher levels to use those instead.

[KrzysztofCwalina]

Do we have the list?

[KirillOsenkov]

May I suggest that you start a new VS instance under debugger with first-chance exceptions on. It is educational and worth doing at least once as the best way to familiarize yourself with the problem. Try creating a C# console app, editing, building.

[CyrusNajmabadi]

Do we have the list?

Not of the top of my head. And I'm not really using VS anymore. But it's not hard to collect just by running the experiment.

[davkean]

These are the ones that I'm aware of:

Exception	Throw by?	Notes
FileNotFoundException	Normal probing of XmlSerializer	No workaround.
BadImageFormatException/FileLoadException	Assembly.GetAssemblyName
MissingManifestResourceException	Normal probing of satellite assemblies	No workaround.
MissingMethodException	Binder.DefaultBinder.BindToMethod when it cannot find appropriate method	Plan on working around around this by copying huge chunks of DefaultBinder and avoiding thrown exception.
InvalidCastException/FormatException/OverflowException	Convert.ChangeType when it cannot convert from type	Don't know how to workaround yet, probably attempt to come up with a list of known "allowed" conversions and manually call TryParse.

[jnm2]

@davkean Lol! My list is identical.

[gulbanana]

there are few enough badly behaved apis which encourage throw-and-catch that i routinely rework code to avoid using them - i was reminded of this issue today when a coworker mentioned avoiding some uses of “new XmlSerializer()” for the same reason. if you can get near to 0 first-chance exceptions it’s a great aid to debuggability, and every little feature like TryGetAssemblyName helps.

please don’t discard the idea just because it isn’t a performance issue!

[davkean]

Yes, forgot about that one, added it to the list.

[danmoseley]

-> Future as not needed for 2.1

[steveharter]

Due to 5.0 schedule constraints, this is being moved to Future.

[jkotas]

The other alternative available today requires using System.Reflection.Metadata which is not part of netstandard

System.Reflection.Metadata is part of .NET Core surface now, so this is no longer concern.

System.Reflection.Metadata is the right way to solve this going forward. Also, System.Reflection.Metadata is faster and lighter weight API than AssemblyName.GetAssemblyName that runs the full runtime assembly loader in a special mode.