Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type.GetType throws when passed Program&& #98426

Closed
mrvoorhe opened this issue Feb 14, 2024 · 9 comments
Closed

Type.GetType throws when passed Program&& #98426

mrvoorhe opened this issue Feb 14, 2024 · 9 comments
Labels
area-System.Reflection breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. regression-from-last-release
Milestone

Comments

@mrvoorhe
Copy link
Contributor

Description

With net7 Type.GetType("Program&&") will return null because a ref to a ref is not a valid type. Starting with net8, this code now throws.

Reproduction Steps

Create a console application targeting net8 with the following code

internal class Program
{
    public static void Main(string[] args)
    {
        var result = Type.GetType("Program&&");
        Console.WriteLine(result == null ? "null" : result);
    }
}

Expected behavior

The output should be

null

Actual behavior

The output is

Unhandled exception. System.TypeLoadException: Could not load type 'Program&' from assembly 'TypeGetTypeWorksWithByRefModifier, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'.
   at System.RuntimeTypeHandle.MakeByRef()
   at System.RuntimeType.MakeByRefType()
   at System.Reflection.TypeNameParser.ModifierTypeName.ResolveType(TypeNameParser& parser, String containingAssemblyIfAny)
   at System.Reflection.TypeNameParser.Parse()
   at System.Reflection.TypeNameParser.GetType(String typeName, Func`2 assemblyResolver, Func`4 typeResolver, Assembly requestingAssembly, Boolean throwOnError, Boolean ignoreCase, Boolean extensibleParser)
   at System.Type.GetType(String typeName)
   at Program.Main(String[] args) in C:\UnitySrc\dev\TestGround\TypeGetTypeWorksWithByRefModifier\TypeGetTypeWorksWithByRefModifier\Program.cs:line 8

Regression?

Yes this is a regression. .NET7, .NET Framework, and mono (a version from mono github repo) all return null. Our test that hit this is not new, so suspect earlier versions of .NET also returned null.

Known Workarounds

No response

Configuration

I was using 8.0.200. Windows, x64

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 14, 2024
@ghost
Copy link

ghost commented Feb 14, 2024

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

With net7 Type.GetType("Program&&") will return null because a ref to a ref is not a valid type. Starting with net8, this code now throws.

Reproduction Steps

Create a console application targeting net8 with the following code

internal class Program
{
    public static void Main(string[] args)
    {
        var result = Type.GetType("Program&&");
        Console.WriteLine(result == null ? "null" : result);
    }
}

Expected behavior

The output should be

null

Actual behavior

The output is

Unhandled exception. System.TypeLoadException: Could not load type 'Program&' from assembly 'TypeGetTypeWorksWithByRefModifier, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'.
   at System.RuntimeTypeHandle.MakeByRef()
   at System.RuntimeType.MakeByRefType()
   at System.Reflection.TypeNameParser.ModifierTypeName.ResolveType(TypeNameParser& parser, String containingAssemblyIfAny)
   at System.Reflection.TypeNameParser.Parse()
   at System.Reflection.TypeNameParser.GetType(String typeName, Func`2 assemblyResolver, Func`4 typeResolver, Assembly requestingAssembly, Boolean throwOnError, Boolean ignoreCase, Boolean extensibleParser)
   at System.Type.GetType(String typeName)
   at Program.Main(String[] args) in C:\UnitySrc\dev\TestGround\TypeGetTypeWorksWithByRefModifier\TypeGetTypeWorksWithByRefModifier\Program.cs:line 8

Regression?

Yes this is a regression. .NET7, .NET Framework, and mono (a version from mono github repo) all return null. Our test that hit this is not new, so suspect earlier versions of .NET also returned null.

Known Workarounds

No response

Configuration

I was using 8.0.200. Windows, x64

Other information

No response

Author: mrvoorhe
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@steveharter
Copy link
Member

I was able to repro; be sure to include namespace in the string ".Program&&"

 	System.Private.CoreLib.dll!System.RuntimeTypeHandle.MakeByRef()	Unknown
 	System.Private.CoreLib.dll!System.RuntimeType.MakeByRefType()	Unknown
 	System.Private.CoreLib.dll!System.Reflection.TypeNameParser.ModifierTypeName.ResolveType(ref System.Reflection.TypeNameParser parser, string containingAssemblyIfAny)	Unknown
 	System.Private.CoreLib.dll!System.Reflection.TypeNameParser.Parse()	Unknown
 	System.Private.CoreLib.dll!System.Reflection.TypeNameParser.GetType(string typeName, System.Func<System.Reflection.AssemblyName, System.Reflection.Assembly> assemblyResolver, System.Func<System.Reflection.Assembly, string, bool, System.Type> typeResolver, System.Reflection.Assembly requestingAssembly, bool throwOnError, bool ignoreCase, bool extensibleParser)	Unknown
 	System.Private.CoreLib.dll!System.Type.GetType(string typeName)	Unknown
>	ConsoleApp217.dll!ConsoleApp217.Program.Main(string[] args = {string[0]}) Line 7	C#

@steveharter
Copy link
Member

@mrvoorhe what are you trying to do?

Note that the doc for GetType(string) does throw TypeLoadException in at least one other case.

We should determine if this change from 7.0 to 8.0 was on purpose or not.

@steveharter steveharter added investigate and removed untriaged New issue has not been triaged by the area owner labels Feb 14, 2024
@steveharter steveharter added this to the 9.0.0 milestone Feb 14, 2024
@steveharter steveharter added help wanted [up-for-grabs] Good issue for external contributors regression-from-last-release labels Feb 14, 2024
@mrvoorhe
Copy link
Contributor Author

@steveharter At Unity we have a large suite of tests for IL2CPP that compares IL2CPP to mono, .NET Framework, and .NET. I'm updating our projects from net7 to net8 and a test that had code similar to this went from returning null to throwing which caused our test to fail since IL2CPP no longer gives the same result as net8.

I didn't dig into the history of the test. It's plausible we added the test in response to a bug report from a Unity user and maybe we did the wrong thing once upon a time. Or maybe we were just writing tests for invalid cases to make sure we did the same thing as mono/.NET Framework.

I don't really care if the API throws or returns null in this case. I'm happy to let you decide that. I thought I would file a bug given that it was a long-standing behavior to return null and now that has changed. I thought maybe there was a chance it changed by accident and if that was the case maybe a bug report would be appreciated.

@jkotas
Copy link
Member

jkotas commented Feb 16, 2024

This change was introduced by #83484 and related changes. We had multiple type name parsers and it was not unusual for them to have different behavior in corner cases like this one. The behavior was unified on:

  • If the type with the given name is not found, return null.
  • If the type is invalid, throw TypeLoadException. This includes reasons like generic constrain violations or invalid composition of param types.

https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/9.0/type-instance that will appear in .NET 9 may also cause some new test cases to fail in your extensive suite. We allowed certain invalid array types to be loaded that caused all sorts of problems when these invalid types were passed to other APIs. We disallow loading these types now that will show up through Type.GetType too.

System.TypeLoadException: Could not load type 'Program&' from assembly

This error message is wrong. I have submitted #98548 to fix it.

@mrvoorhe
Copy link
Contributor Author

Thanks for the explanation @jkotas !

@MichalPetryka
Copy link
Contributor

This change was introduced by #83484 and related changes. We had multiple type name parsers and it was not unusual for them to have different behavior in corner cases like this one. The behavior was unified on:

  • If the type with the given name is not found, return null.
  • If the type is invalid, throw TypeLoadException. This includes reasons like generic constrain violations or invalid composition of param types.

https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/9.0/type-instance that will appear in .NET 9 may also cause some new test cases to fail in your extensive suite. We allowed certain invalid array types to be loaded that caused all sorts of problems when these invalid types were passed to other APIs. We disallow loading these types now that will show up through Type.GetType too.

System.TypeLoadException: Could not load type 'Program&' from assembly

This error message is wrong. I have submitted #98548 to fix it.

Should this get a breaking change notice added in such case?

@jkotas jkotas added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed help wanted [up-for-grabs] Good issue for external contributors investigate labels Feb 20, 2024
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Feb 20, 2024
@ghost
Copy link

ghost commented Feb 20, 2024

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

  1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

@jkotas jkotas removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Feb 20, 2024
@jkotas
Copy link
Member

jkotas commented Feb 20, 2024

Should this get a breaking change notice added in such case?

Yes: dotnet/docs#39594

@jkotas jkotas closed this as completed Feb 20, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. regression-from-last-release
Projects
None yet
Development

No branches or pull requests

4 participants