Compilation.GetTypeByMetadataName returns null even though the other ObsoleteAttribute is internal

[jnm2]

Version Used: Visual Studio 16.9.2 (hosting analyzers), 3.9.0 NuGet packages (repro below)

Roslyn should be able to see that the internal System.ObsoleteAttribute declared by System.DirectoryServices is not accessible to the compilation. (System.DirectoryServices does not declare its internals visible to my project.) GetTypeByMetadataName should return the only public one.

The compilation succeeds, so there is a well-known way to resolve this conflict if it's even a conflict.

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using System;
using System.IO;

var compilation = CSharpCompilation.Create(
    assemblyName: null,
    syntaxTrees: new[] { CSharpSyntaxTree.ParseText("[System.Obsolete] class C { }") },
    references: new[]
    {
        // These both get included in projects that target net5.0-windows
        MetadataReference.CreateFromFile(@"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\5.0.0\ref\net5.0\System.Runtime.dll"),
        MetadataReference.CreateFromFile(@"C:\Program Files\dotnet\packs\Microsoft.WindowsDesktop.App.Ref\5.0.0\ref\net5.0\System.DirectoryServices.dll"),
    },
    new(OutputKind.DynamicallyLinkedLibrary));

Console.WriteLine("Compilation success: " + compilation.Emit(Stream.Null).Success);

var type = compilation.GetTypeByMetadataName(typeof(ObsoleteAttribute).FullName!);
Console.WriteLine("GetTypeByMetadataName returns null: " + (type is null));

Prints:

Compilation success: True
GetTypeByMetadataName returns null: True

In case this is "by design"

This is a pit of failure for people writing our own analyzers and source generators. For example, my analyzer stopped ignoring obsolete members as soon as it happened to be used in a net5.0-windows project. If there is some reason that this behavior can't be fixed, analyzer writers should get a warning if we try to use it. I think leaving things in the current pitfall-inducing state should not be seen as acceptable.

[sharwell]

This behavior is By Design. It's part of the reason the API is banned for the entire dotnet/roslyn-analyzers repository. Here's the entry point for our implementation of an alternative approach: https://github.com/dotnet/roslyn-analyzers/blob/dc515b0412abf13d9d2b8e34543c61c9abbee87b/src/Utilities/Compiler/WellKnownTypeProvider.cs#L185

[jnm2]

In that case, I'm asking for the state of things to be solved so that there is no pit of failure for analyzer devs that don't know about this pitfall yet. Everyone out in the community in my experience knows about GetTypeByMetadataName and seems to think it is preferred.

[jnm2]

For what it's worth, @jaredpar in https://github.com/dotnet/roslyn/issues/3864#issuecomment-285732623:

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.

I hoped this user report on this being a problem would trigger that revisit.

[jaredpar]

This behavior is "By Design" and there is a long discussion on #3864 on why we can't take breaking changes here. That discussion was ~5 years ago and the reasoning in that thread is even stronger now. Essentially:

1. The API has existed with this behavior for ~7 years now and hence the compatibility is very much set at this point.
2. The number of complaints about this behavior remains extremely low.

I think we'd be receptive of APIs additions that helped deal with the ambiguity issues here but at this point changing this API isn't an option.

[jaredpar]

If we want to flip this to a discussion to explore new API ideas here then let me know and I'll move this to a discussion. If this issue just wants to track changing the behavior though my response would be that we should dupe off of #3864.

[jnm2]

@jaredpar Thanks. I don't care which way you go so long as we end up with an open issue/something tracking a fix to keep people from falling into using GetTypeByMetadataName for things they shouldn't, and providing reasonable simple alternatives for people building their own analyzers.

[CyrusNajmabadi]

@jaredpar FWIW< we already have teh replacement API we use since the compiler one isn't viable. I think we could jsut take a PR to move that into roslyn, with an obsolete attribute on the existing API that points to the new one.

[jaredpar]

I'm not sure about the motivation for obsoleting the current version. Again I'd point to 7+ years of usage with a tiny handful of complaints. That wouldn't meet my bar for force everyone to move the cheese.

[jmarolf]

How do we not know its 7+ years with a tiny handful of usage and a large number of complaints?

[jaredpar]

https://github.com/dotnet/roslyn/search?q=GetTypeByMetadataName&type=issues

106 issues where customers are referencing GetTypeByMetadataName. For me that qualifies as significant amount of usage.

Compare that with two issues where people complain about this specific behavior and ~5 years in between complaints. That is the information I'm going off of.

[jmarolf]

Yeah there are ~400 usages of this API in the wild in non-Microsoft owned repos which amounts to 4 repos.

[CyrusNajmabadi]

I would obsolete because:

1. the existing api is a pit of fail.
2. teh new api is a drop-in replacement.

No good comes from using the existing api. At best, it gives a false sense of security, but is a ticking timebomb. And, when someone runs into thsi, they'll have to then go and try to find out what to do. Obsolete (warning is fine with me) immediately tells people about the problem, points them immediately to teh right solution, and future proofs their code to issues they may not even know they have.

We could certainly also deliver this with an easy code-fixer that will make this change to their solution.

[jaredpar]

Yeah there are ~400 usages of this API in the wild in non-Microsoft owned repos which amounts to 4 repos.

Yeah I guess those 100+ issues opened don't exist and we can totally discount them as an indication of consumption.

Obsolete (warning is fine with me) immediately tells people about the problem,

It's still moving the cheese for a lot of people who don't really care about this corner case.

I think we should honestly divorce the two discussions here:

1. Considering a new API that gives consumers more control over this case. Expecting that should be able to move forward.
2. What to do with the existing API should (1) complete.

[jmarolf]

I dunno man, the .NET library folks are ok with deprecating APIs used by millions of people. I am ok with saying GetTypeByMetadataName is deprecated in .NET 6. We'd want to follow the same breaking change notifications as any other .NET API of course, but that seems fine to me.

[CyrusNajmabadi]

It's still moving the cheese

If the cheese is moldy (and not in a 'mmmm, this stilton is delicious' way) i'm ok with moving it :)

[sharwell]

I'm not certain deprecation is the right approach for all scenarios where this API is used. It might be better to start with an analyzer in Microsoft.CodeAnalysis.Analyzers that encourages analyzer authors to migrate. Of course all of this depends on the shape of the new API, which still hasn't been decided.

[jnm2]

For the benefit of those who aren't sure of one, what is a scenario where this is definitely not a time bomb?

[jaredpar]

The 99% case where type names are unique across assemblies. This only is an issue when there are duplicate type names. That is an exceedingly rare event for the majority of types out there. In the vast majority of cases users consume this API for types that don't have a reasonable expectation of ever having a duplicate. Asking everyone to move the cheese doesn't make sense to me.

Again though, think we should split up the conversations.

[jnm2]

Again though, think we should split up the conversations.

Okay, let's do that. :+1:

[jaredpar]

Think we should do the following:

1. Open a new issue that tracks adding the API @CyrusNajmabadi believes will be more appropriate here
2. Open an issue to track discussing whether to obsolete the existing API should (1) get merged
3. Close this issue

I thought about re-purposing this issue for (2) but feel like this issue is pretty setup for a different convo and there is a lot of unnecessary discussion here. Better to start clean.

Thoughts?

[jnm2]

Yes, start clean.

[jnm2]

Quick question. If this is true:

The 99% case where type names are unique across assemblies. This only is an issue when there are duplicate type names. That is an exceedingly rare event for the majority of types out there. In the vast majority of cases users consume this API for types that don't have a reasonable expectation of ever having a duplicate.

And some of the 1% cases are certainly time bombs, like mine, why not fix the design flaw? You've just said it can't affect the vast majority. The effect it has on the tiny minority in this case seems like a strictly good one.

[jnm2]

Since I prefer the name that contains the word 'best,' reminding you that there is more than one symbol it may be choosing between, I prefer there to be a warning on GetTypeByMetadataName pointing you to GetBestTypeByMetadataName. So my question is really just holding up my preferred outcome, even though I feel it's an open question that hasn't been answered to my understanding.

[jnm2]

Filed a new issue: https://github.com/dotnet/roslyn/issues/52118