Roslyn: Reconsider Compilation.GetTypeByMetadataName behavior

[matkoch]

When using Compilation.GetTypeByMetadataName we run into the situation, that their was returned null although the type was definitely available.

After a bit of debugging it turned out, that there were two types with the same name. We used JetBrains.Annotations one time internalized, and one time it was referenced through NuGet.

However, we now work around this issue by manually asking each assembly explicitely; but I think that returning null there is still a strange behavior, because it overlaps with returning null if no type was found at all. What about declaring a parameter like throwOnMultiple and throwing an exception? Or returns a collection? Or expose isWellKnownType in order to filter non-public types?

[gafter]

The API Compilation.GetTypeByMetadataName is not intended as a replacement for using a compiler and the diagnostics the compiler produces. As you've found, there are other APIs that give you the behavior you need.

[fschmied]

@gafter Really? What's the value of returning null when multiple matching types are found (as opposed to, e.g., throwing an exception or - better - returning a collection)?

[gafter]

@fschmied The value is that it doesn't break compatibility with existing clients of this API.

[matkoch]

@gafter are you serious? I used to find bugs in the reflection API like every second week, when I did my master thesis. This will lead us to the same situation. Why don't you just fix such failures. It's just a headache to find them (also for anyone in the future). I really don't see the problem in changing the API; todays developers rely on continuous integration. If someone would have a reasonable problem when you change that behaviour, I'm sure it is only one of the smaller problems they have.

[gafter]

We cannot change the signature of the (existing, released) API. That would be a serious binary breaking change.

We also should not throw an exception for an API that is used in the way it is expected to be used. As a general principle, we throw exceptions for misuse of the APIs, and report diagnostics for misuse of the language. This is closer to the latter than the former, but this API has no way to report diagnostics.

I think the best we could do would be to create a new, separate API for your request. Given that you have an easy workaround, I don't see why we'd do that.

@AnthonyDGreen @jaredpar @mattwar @tmat @VSadov You're welcome to chime in here if you have an opinion.

[matkoch]

My objection is, that there _will_ be developers, who will run into the same trap; and it takes much effort to find out why it is suddenly not working anymore. Like in my case, we just internalized types, while they were still public in some references. It is purely a bad designed API, which is okay, since Roslyn is still in an early stage. But on the other hand, we shouldn't argue with compatibility here... in my opinion.

My preferred way of solving this issue would be to remove the method Compilation.GetTypeByMetadataName at all and replace it with GetTypesByMetadataNames (the collection version). The single value solution makes no sense, because in a compilation, it is guaranteed possible that multiple types have the same name. People who rely on the old way could simply replace it with xxx.SingleOrDefault().

[jaredpar]

My preferred way of solving this issue would be to remove the method Compilation.GetTypeByMetadataName

As @gafter mentioned this is a shipped API so removing it isn't an option. Doing so would potentially break many existing customers.

[matkoch]

@jaredpar: As I already comprehensively explained, I don't see this as an argument.

@gafter, @jaredpar:
Until now you didn't give your opinion on the actual problem. Do you think it is good to return null from Compilation.GetTypeByMetadataName when there are multiple types? FYI, Type.GetMethod also throws an exception, if multiple methods with the same name exist; this is something I _would_ expect. What I do _not_ expect is that for two circumstances (a) no method and (b) multiple methods, the same value of null is returned. This is the most surprising behaviour I can think of.

As you've found, there are other APIs that give you the behavior you need.

Yes, but people are expecting this one to work too. And what I actually did was to write a method that does exactly that.

The value is that it doesn't break compatibility with existing clients of this API.

So the value of non-breaking code is worth more than a clean API, _although_ the fix would be easy as Pi?

[fschmied]

@gafter, @jaredpar I can certainly see the value of striving for compatibility in the Roslyn API, but surely you have a process for fixing bad APIs even after a release (for a given definition of "bad")? For example, you could deprecate the API (while keeping it), pointing users to a replacement API in the doc comments and/or by means of the ObsoleteAttribute.

So, if there is a process for fixing bad APIs after a release, it remains to be defined whether the Compilation.GetTypeByMetadataName API is bad enough to warrant a fix. This is of course your decision, but it would be cool if you could explain the reasoning behind it.

As one of those burnt by the API, IMO there are two issues with Compilation.GetTypeByMetadataName. First, it's non-intuitive and undocumented that the API returns null when the given name is ambiguous; second, the API seems to be the wrong one to use in most cases.

For the first issue, you could at least adapt the docs from:

/// <returns>Null if the type can't be found.</returns>

to:

/// <returns>Null if the type can't be found or it is ambiguous.</returns>

About the second issue - in what cases is Compilation.GetTypeByMetadataName meant to be used? For any type that might possibly be declared in two different assemblies (internal or public), and that includes System.Exception and other well-known types, it seems Assembly.GetTypeByMetadataName had better be used instead.

[AnthonyDGreen]

I think we can all agree that whatever solution we come up with doesn't involve changing signatures of existing methods or outright removing them and that if we wanted a different API shape we would introduce a new method and/or overloads. Let's not get distracted by that since I think that preserving the existing method overload signature is generally uncontroversial.

Regarding the original issue, I agree that the behavior of silently failing on an ambiguity error is undesirable. If its behavior isn't reliable and people are forced to fallback to manually iterating all referenced AssemblySymbols then the API is useless. In which case it should still be marked Obsolete and EditorBrowsable(Hidden).

If we want to preserve the value of an API at the Compilation level for looking up types by name we can introduce a new one. Perhaps LookupTypeByMetadataName or some such that returns a SymbolInfo object. SymbolInfo is the structure we use in the API for returning symbolic information but which also has the capacity to express errors - such as accessibility and ambiguity errors. The problem with this is that GetTypeByMetadataName is more permissive than true lookup if you were to lookup a symbol inside of a method body. Of the 15 or so candidate reasons GetSymbolInfo could return when binding an expression only 1 is applicable to this case. Are we OK with that? And technically multiple types having the same metadata name isn't an error case, so maybe this is too much reuse of SymbolInfo?

Or we can try to repair the existing method. Changing the doc comment is the lowest hanging fruit but not terribly satisfactory. We could add an overload which takes a parameter a la throwOnDuplicate but the problem with that is - what does someone do when the API throws that? As Neal indicates an Exception should indicate that an API was used incorrectly. This isn't an incorrect usage. Throwing just leads to the workaround the OP is using now. But the workaround is literally re-implementing what GetTypeByMetadataName (it checks the current assembly then iterates through references). It feels like the workaround is by its nature a rebuke of the broken way the method is currently implemented. Further even if we all move to the workaround what is the prescribed guidance on what to do when this situation arises? We might end up with various people using slightly different approaches. Some might throw (useless) and others will implement first-found wins (many accidentally). I guess we could punt the decision to the use and take an enum which specifies one of these strategies DuplicateOptions.ReturnFirst | Throw | ReturnNull. It's not my favorite approach to library design but it is a solution. @matkoch What do you do when you run into this ambiguity?

FWIW, I've been advising people writing library specific diagnostic analyzers to use this method as a soft-check on whether their analyzer is relevant. E.g. "To know whether this is a WPF-project or this project references WPF lookup "System.Windows.DependencyObject" or some other well-known type and register analyzer actions if GetTypeByMetadataName returns non-null. The current behavior unintentionally causes analyzers to short-circuit in scenarios where they shouldn't. We should get our story straight here for v1.1.

[AnthonyDGreen]

Re-opening this. We're NOT going to break existing clients by removing or changing this method signature but this is a design bug on how to we intend this API to be used (or not used) and whether a new API is needed to fulfill its purpose or additionally overloads to clarify its behavior.

[fschmied]

@AnthonyDGreen My preferred option would be to have a collection returned plus having the option of filtering for accessible types (like isWellKnownType in the {{CSharp}} implementation). In my case, we'd probably need to filter the collection for accessible types first. Then, if there's more than one left, we'd either just select one of those returned (if just rendered for code generation) or give a diagnostic error or warning (if used for analysis).

[robintheilade]

there are other APIs that give you the behavior you need.

As a brand new consumer of this API, I would really like to know what other API's I have available to workaround this lack of flexibility. A short code snippet is also appreciated.

However, we now work around this issue by manually asking each assembly explicitely

How?

(I vote for ObsoleteAttribute if you ask me, breaking changes are good in the long run, but so is a fair warning)

[fschmied]

@robintheilade The GetTypeByMetadataName API exists on both the Compilation and the IAssemblySymbol types. Therefore, if you have a Compilation, you can get the interesting IAssemblySymbols from that, and then call IAssemblySymbol.GetTypeByMetadataName.

compilation.References
    .Select(compilation.GetAssemblyOrModuleSymbol)
    .OfType<IAssemblySymbol>()
    .Select(assemblySymbol => assemblySymbol.GetTypeByMetadataName(typeMetadataName))

Depending on your use case, you might also want to include Compilation.Assembly in the list of searched IAssemblySymbols.

[jaredpar]

Given the behavior has been this way for 1+ years now and we haven't seen any reports if this tripping up users we've decided to keep the existing behavior. Will revisit if we run into any user reports on this being a problem.

[zarenner]

This behavior was super confusing to me today while trying to get the Thread type, which exists in System.Threading.Thread and System.Private.CoreLib in .NET Core.

I'm glad I found this thread, but it would be nice if the docs were updated to reflect this behavior and suggest alternate APIs to use (e.g. going through IAssemblySymbol.GetTypeByMetadataName).

[CyrusNajmabadi]

I'm glad I found this thread, but it would be nice if the docs were updated to reflect this behavior and suggest alternate APIs to use (e.g. going through IAssemblySymbol.GetTypeByMetadataName).

@zarenner PRs welcome.

[jzabroski]

Given the behavior has been this way for 1+ years now and we haven't seen any reports if this tripping up users we've decided to keep the existing behavior. Will revisit if we run into any user reports on this being a problem.

How do I know if I hit this, exactly?