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

@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.

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.

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.

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

These are the ones that I’m aware of:

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.

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.

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.

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

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!

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.

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.

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 😃

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

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.

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();

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.

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.

@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.

@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.

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 😸

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

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.

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

See https://github.com/dotnet/standard/issues/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).

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.