Remove redundant and incorrect checking.

[fanyang-mono]

Mono class MethodOnTypeBuilderInst was designed to does the same job as two of CoreCLR classes MethodBuilderInstantiation and MethodOnTypeBuilderInstantiation. By tracking down the definition of the constructors and function MakeGenericMethod, I came to a conclusion that the argument length checking is redundant and incorrect.

Contributes to #2389

[Dotnet-GitSync-Bot]

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

[naricc]

I'm not sure I understood why this is wrong? It sounds like they should be the same; what do they really represent?

[fanyang-mono]

I checked the implementation on CoreCLR side, this check doesn't exist there. Here are the CoreCLR implementations:

• On one side, MethodBuilder::MakeGenericMethod

  runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/MethodBuilder.cs

  Lines 543 to 546 in 0fd78a8

  	public override MethodInfo MakeGenericMethod(params Type[] typeArguments)
  	{
  	return MethodBuilderInstantiation.MakeGenericMethod(this, typeArguments);
  	}

  calls https://github.com/dotnet/runtime/blob/master/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/MethodBuilderInstantiation.cs#L11-L17 which eventually calls the constructor
• On the other side, the constructor of MethodOnTypeBuilderInstantiation assign type to a variable of TypeBuilderInstantiation type. And MakeGenericMethod of TypeBuilderInstantiation could be found here

  runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilderInstantiation.cs

  Lines 20 to 37 in 1cdafd2

  	internal static Type MakeGenericType(Type type, Type[] typeArguments)
  	{
  	Debug.Assert(type != null, "this is only called from RuntimeType.MakeGenericType and TypeBuilder.MakeGenericType so 'type' cannot be null");
  	if (!type.IsGenericTypeDefinition)
  	throw new InvalidOperationException();
  	if (typeArguments == null)
  	throw new ArgumentNullException(nameof(typeArguments));
  	foreach (Type t in typeArguments)
  	{
  	if (t == null)
  	throw new ArgumentNullException(nameof(typeArguments));
  	}
  	return new TypeBuilderInstantiation(type, typeArguments);
  	}

[lambdageek]

@naricc Evidently CoreCLR actually ends up generating some invalid IL (ie IL that applies the generic method to an incorrect number of arguments) and then only throws an error at runtime https://gist.github.com/lambdageek/e8173ed7b0e3f416ae1ff656d5141f98

[am11]

@lambdageek, if this is a potential bug in coreclr implementation, should mono introduce the same bug or report it instead to find resolution?

[lambdageek]

@am11 I don't think it's a bug - or at least I could conceive of a rationalization for why CoreCLR behaves as it does. Just because IL today does not allow - for example - defaulted generic parameters, doesn't mean that it won't support it in the future. So while the IL is bad today, you could imagine using that at some point in the future it might actually be sensible in some circumstances. So it makes sense to allow the IL to be constructed and to leave the check is at the place where it actually matters - in the runtime when it needs to decide how to execute the IL.

(By defaulted generic parameters I mean something like void Method<T1, T2> (T1 arg1, T2 arg2) where T2 : default(T2)=int and then uses could be Method<string>("abcd", 42) // actually Method<string,int>(...) )

[ghost]

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

Issue Details

---

Mono class MethodOnTypeBuilderInst was designed to does the same job as two of CoreCLR classes MethodBuilderInstantiation and MethodOnTypeBuilderInstantiation. By tracking down the definition of the constructors and function MakeGenericMethod, I came to a conclusion that the argument length checking is redundant and incorrect.

Author:	fanyang-mono
Assignees:	-
Labels:	area-CoreLib-mono, area-System.Reflection-mono
Milestone:	-

[SamMonoRT]

please verify the previously failing tests are included in the validation runs with this PR.

[fanyang-mono]

please verify the previously failing tests are included in the validation runs with this PR.

For System.Reflection.Emit.Tests running on Mono, there were 1233 tests before this PR and 1235 tests with this PR. Thus, the tests should be enabled successfully.

[lambdageek]

LGTM. In the long term we should try to unify MethodBuilderInstantiation (CoreCLR) with MethodOnTypeBuilderInst (Mono) - with the caveat that actually MethodOnTypeBuilderInst is doing double-duty as MethodBuilderInstantiation and also MethodOnTypeBuilderInstantiation (CoreCLR). And the second caveat that the native side in mono in sre.c will need to be updated to look for the new class names. Slightly non-trivial.

But it's good to first get the tests working and then to undertake a more thorough refactoring.

[fanyang-mono]

Created #46149 to track future refactoring work.