Compilation.GetTypeByMetadataName does not account for accessibility

[sharwell]

The following code is potentially problematic:

runtime/src/libraries/System.Text.Json/gen/Reflection/MetadataLoadContextInternal.cs

Line 26 in c8a658c

INamedTypeSymbol? typeSymbol = _compilation.GetTypeByMetadataName(fullyQualifiedMetadataName);

If any assembly referenced by the compilation defines a second copy of a type as internal (e.g. how Microsoft.CodeAnalysis defines its own copy of the nullable attributes), GetTypeByMetadataName will return null for that type even if only one copy is accessible to the compilation. The problem occurred enough times that dotnet/roslyn-analyzers banned direct calls to Compilation.GetTypeByMetadataName outright and provided an alternative that falls back to an accessibility-aware check.

See also dotnet/roslyn#52399

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following code is potentially problematic:

runtime/src/libraries/System.Text.Json/gen/Reflection/MetadataLoadContextInternal.cs

Line 26 in c8a658c

INamedTypeSymbol? typeSymbol = _compilation.GetTypeByMetadataName(fullyQualifiedMetadataName);

If any assembly referenced by the compilation defines a second copy of a type as internal (e.g. how Microsoft.CodeAnalysis defines its own copy of the nullable attributes), GetTypeByMetadataName will return null for that type even if only one copy is accessible to the compilation. The problem occurred enough times that dotnet/roslyn-analyzers banned direct calls to Compilation.GetTypeByMetadataName outright and provided an alternative that falls back to an accessibility-aware check.

See also dotnet/roslyn#52399

Author:	sharwell
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[ericstj]

Removing @layomia assignment as he is out at the moment. We need to make this fix for 6.0 as it would be a blocker for source generator usage.

[steveharter]

@sharwell is the recommendation to copy the extension file and use it locally? I don't see a public (shipping) API.

For STJ, GetTypeByMetadataName() is currently only used for 2 cases:

• Types passed to MetadataLoadContextInternal.Resolve. This is used for several well-known types including all collection types as well as unsupported types like IntPtr.
• Hard-coded locations not likely to be re-used internally or by others:
  • "System.Text.Json.Serialization.JsonSerializerContext"
  • "System.Text.Json.Serialization.JsonSerializableAttribute"
  • "System.Text.Json.Serialization.JsonSourceGenerationOptionsAttribute"

[layomia]

Fixed for 6.0 in #58766.

[ericstj]

@layomia @maryamariyan was this fixed in the logging generator as well? If not, do we have an issue tracking that?

[ericstj]

Reopening because this doesn't appear to be addressed in the Logging generator.

runtime/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

Lines 68 to 94 in 73c0f8d

	INamedTypeSymbol loggerMessageAttribute = _compilation.GetTypeByMetadataName(LoggerMessageAttribute);
	if (loggerMessageAttribute == null)
	{
	// nothing to do if this type isn't available
	return Array.Empty<LoggerClass>();
	}
	INamedTypeSymbol loggerSymbol = _compilation.GetTypeByMetadataName("Microsoft.Extensions.Logging.ILogger");
	if (loggerSymbol == null)
	{
	// nothing to do if this type isn't available
	return Array.Empty<LoggerClass>();
	}
	INamedTypeSymbol logLevelSymbol = _compilation.GetTypeByMetadataName("Microsoft.Extensions.Logging.LogLevel");
	if (logLevelSymbol == null)
	{
	// nothing to do if this type isn't available
	return Array.Empty<LoggerClass>();
	}
	INamedTypeSymbol exceptionSymbol = _compilation.GetTypeByMetadataName("System.Exception");
	if (exceptionSymbol == null)
	{
	Diag(DiagnosticDescriptors.MissingRequiredType, null, "System.Exception");
	return Array.Empty<LoggerClass>();
	}

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following code is potentially problematic:

runtime/src/libraries/System.Text.Json/gen/Reflection/MetadataLoadContextInternal.cs

Line 26 in c8a658c

INamedTypeSymbol? typeSymbol = _compilation.GetTypeByMetadataName(fullyQualifiedMetadataName);

If any assembly referenced by the compilation defines a second copy of a type as internal (e.g. how Microsoft.CodeAnalysis defines its own copy of the nullable attributes), GetTypeByMetadataName will return null for that type even if only one copy is accessible to the compilation. The problem occurred enough times that dotnet/roslyn-analyzers banned direct calls to Compilation.GetTypeByMetadataName outright and provided an alternative that falls back to an accessibility-aware check.

See also dotnet/roslyn#52399

Author:	sharwell
Assignees:	steveharter
Labels:	blocking-release, area-Extensions-Logging
Milestone:	6.0.0

[steveharter]

@sharwell this appears to only affect framework types that are redefined\cross-compiled, and not user-defined types since conflicts appear to be avoided by selecting the type in the first referenced assembly, whether the Type is public or not.

Since System.Text.Json resolves many internal types (all supported collection types and unsupported types) it makes sense to do that just in case. However, for other generators that use GetTypeByMetadataName() they just have a limited number of hard-coded cases that are unlikely to conflict. Is it still important to change these to use GetBestTypeByMetadataName()? Here's a list of remaining:

Microsoft.Extensions.Logging.Abstractions:
INamedTypeSymbol loggerMessageAttribute = _compilation.GetTypeByMetadataName(LoggerMessageAttribute);	C:\git\runtime1\src\libraries\Microsoft.Extensions.Logging.Abstractions\gen\LoggerMessageGenerator.Parser.cs	68	72
INamedTypeSymbol loggerSymbol = _compilation.GetTypeByMetadataName("Microsoft.Extensions.Logging.ILogger");	C:\git\runtime1\src\libraries\Microsoft.Extensions.Logging.Abstractions\gen\LoggerMessageGenerator.Parser.cs	75	62
INamedTypeSymbol logLevelSymbol = _compilation.GetTypeByMetadataName("Microsoft.Extensions.Logging.LogLevel");	C:\git\runtime1\src\libraries\Microsoft.Extensions.Logging.Abstractions\gen\LoggerMessageGenerator.Parser.cs	82	64
INamedTypeSymbol exceptionSymbol = _compilation.GetTypeByMetadataName("System.Exception");	C:\git\runtime1\src\libraries\Microsoft.Extensions.Logging.Abstractions\gen\LoggerMessageGenerator.Parser.cs	89	65

System.Diagnostics.Tracing:
INamedTypeSymbol? autogenerateAttribute = _compilation.GetTypeByMetadataName("System.Diagnostics.Tracing.EventSourceAutoGenerateAttribute");	C:\git\runtime1\src\libraries\System.Private.CoreLib\generators\EventSourceGenerator.Parser.cs	34	72
INamedTypeSymbol? eventSourceAttribute = _compilation.GetTypeByMetadataName("System.Diagnostics.Tracing.EventSourceAttribute");	C:\git\runtime1\src\libraries\System.Private.CoreLib\generators\EventSourceGenerator.Parser.cs	41	71

[steveharter]

Since System.Text.Json resolves many internal types (all supported collection types and unsupported types) it makes sense to do that just in case.

Actually the collection and unsupported types are not affected by usage of GetTypeByMetadataName since they are not defined in STJ.

[sharwell]

I would typically use the alternative method everywhere. I can't think of a downside of this, and it's the approach used by dotnet/roslyn-analyzers.

[ericstj]

unlikely to conflict

I bet @jaredpar can think of cases to counter this. Wouldn't be surprised if they are in our own codebases :)

[jaredpar]

The way I usually frame this is:

When a customer comes to you and says "I can't use your source generator because a library I reference re-defined some types" will you be happy with the response of "Sorry, you just can't use our source generator".

That is essentially the situation you can get into here.

In some ways it's not completely unreasonable. For instance do we want to bend over backwards to support libraries that redefine System.Exception? I can see a stance of "no" being completely reasonable there. But it's all about your tolerance level here.

I just want to make sure that the consequences and impact of the decisions is understood 😄

[danmoseley]

@mkArtakMSFT are there issues in ASP.NET's usage? https://grep.app/search?current=2&q=GetTypeByMetadataName&filter[repo][0]=dotnet/aspnetcore

[danmoseley]

ping @mkArtakMSFT 😄

[ericstj]

I believe this can be closed now since #59774 was merged. @danmoseley @mkArtakMSFT I made a new issue in ASP.NET to evaluate.