Do not return file-local type requested by unmangled metadata name

[DoctorKrolic]

Fixes: #67079

[RikkiGibson]

It looks like the implementation of this method is calling GetTopLevelTypeByMetadataName. I would expect it to be necessary to use the "mangled" name of the file-local type in order to get it with this method. But I might have forgotten some of the finer details of how we handled metadata names with file-local types.

[DoctorKrolic]

Sorry, I don't understand, you don't like my implementation or what? My logic was that the fact, that file-local type names are mangled makes them completely unusable as well-known types, since when they are loaded from binary metadata later they are not considered special well-known types anymore. Am I not correct?

[RikkiGibson]

I mean to say: I don't know why the bug is occurring here. I'm not innately opposed to your solution. But I'll need to investigate more throughly when I get the chance, hopefully during the coming week.

[RikkiGibson]

Have looked more closely at the scenario.

We think the core problem is that for a type like file class C { }, GetTypeByMetadataName("C") is returning the symbol for the file type. We shouldn't be able to obtain a symbol for a file type from GetTypeByMetadataName when we are passing in the source name of the type.

The bug probably originates here:

roslyn/src/Compilers/CSharp/Portable/Symbols/NamespaceOrTypeSymbol.cs

Lines 316 to 330 in cc8e2f1

	namespaceOrTypeMembers = scope.GetTypeMembers(emittedTypeName.TypeName);
	foreach (var named in namespaceOrTypeMembers)
	{
	if (!named.MangleName && (forcedArity == -1 || forcedArity == named.Arity))
	{
	if ((object?)namedType != null)
	{
	namedType = null;
	break;
	}
	namedType = named;
	}
	}

We think the condition on line 320 should be modified to also require that the type we found is not a file type in order to match. (We can assume that the candidate type we are looking at is an original definition, i.e. its OriginalDefinition is equal to itself--and it would be good to assert this.) This should eliminate the need for the change to IsValidWellKnownType.

We think that the GetTypeByMetadataName tests below should be expanded:

roslyn/src/Compilers/CSharp/Test/Symbol/Symbols/Source/FileModifierTests.cs

Lines 3575 to 3578 in cc8e2f1

	Assert.Null(comp.GetTypeByMetadataName("<>FE3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855__D"));
	Assert.Null(comp.GetTypeByMetadataName("<>F1__C"));
	Assert.Null(comp.GetTypeByMetadataName("F0__C"));
	Assert.Null(comp.GetTypeByMetadataName("<file>F0__C"));

We would want to be able to add a new assert like Assert.Null(comp.GetTypeByMetadataName("C")); and have the test pass. We'd like to have additional testing showing this to be the case whether:

• the file type is declared in source, or
• the file type is imported from metadata, or
• the file type is being retargeted (e.g. it is being declared in one compilation against one mscorlib, then referenced in another compilation which uses a different mscorlib--if you search for "retargeting" in the compiler tests, many existing test cases should come up.)

I think the new test you added in InitOnlyMemberTests will serve as a great end-to-end test showing we are doing the right thing when GetTypeByMetadataName or related internal helpers are only being used indirectly.

Let us know if you are willing to take a stab at implementing these changes--otherwise we will try and get to it when we have time.

[DoctorKrolic]

Let us know if you are willing to take a stab at implementing these changes

Yes

We shouldn't be able to obtain a symbol for a file type from GetTypeByMetadataName when we are passing in the source name of the type.

This would completely disable, let's say, and IDE from getting a file-local type directly from the Compilation. Is such limitation desired?

[AlekseyTs]

This would completely disable, let's say, and IDE from getting a file-local type directly from the Compilation. Is such limitation desired?

I don't think this is going to be the case. Several tests have the following:

        var comp = (CSharpCompilation)verifier.Compilation;
        var symbol = comp.GetMember<NamedTypeSymbol>("C");
        AssertEx.Equal("<file2>F66382B88D8E28FDD21CEADA0DE847F8B00DA1324042DD28F8FFC58C454BD6188__C", symbol.MetadataName);

So. if a type is looked by its metadata name (as the name of the GetTypeByMetadataName API suggests), the metadata name should be used. There is nothing new in this requirement. For example, a generic type won't be found unless a metadata name is used to find it.

[RikkiGibson]

Also, there are many places in the public API one can obtain type symbols. For example, the IDE can obtain a symbol for a file type from the GetMembers or LookupSymbols APIs. Those lookups will still be done based on source names and this change won't affect it.

[CyrusNajmabadi]

My position on behalf of the ide is that GTBMN should follow clr metadata naming rules.
That said, have we clearly doc'ed the naming rule we are committed to supporting for file scoped types?

[AlekseyTs]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[AlekseyTs]

It looks like build is failing due to nullable warnings in test files. Consider disabling nullable analysis #nullable disable in the test files.

[AlekseyTs]

Do we have tests for all the scenarios outlined at #67183 (comment)?

[AlekseyTs]

Done with review pass (commit 3)

[RikkiGibson]

Tagging @AlekseyTs for second review

[AlekseyTs]

Done with review pass (commit 12).

[AlekseyTs]

Done with review pass (commit 13)

[AlekseyTs]

@dotnet/roslyn-ide, @dotnet/roslyn-analysis It looks like the remaining issue in this PR is in Analyzers and Workspaces layers. Please work with @DoctorKrolic towards resolving it to your liking.

[CyrusNajmabadi]

IDE side lgtm.

[AlekseyTs]

LGTM (commit 15)

[AlekseyTs]

@DoctorKrolic Thanks for the contribution