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

Fix building proxies for classess with static interface members #3298

Merged
merged 5 commits into from
May 9, 2023

Conversation

hazzik
Copy link
Member

@hazzik hazzik commented May 2, 2023

C# 8 (.NET Core 3.1) has introduced static interface members. Currently they are throwing "IndexOutOfRangeException: Index was outside the bounds of the array."
C# 11 (.NET 7) has introduced abstract/virtual interface members. Currently they are throwing "TypeLoadException: Signature of the body and declaration in a method implementation do not match."

In general static interface members should not be proxiable (same as regular static methods) with one exception of interface-only entities for which, if such interface would have an abstract static member it must be implemented by a proxy. However, I could not find a valid use case for this. In this case proxy will fail with "System.TypeLoadException: Virtual static method 'XXX' is not implemented on type 'YYYProxy' from assembly 'YYYProxyAssembly'"

Fixes #3295

@hazzik
Copy link
Member Author

hazzik commented May 2, 2023

@bahusoid @fredericDelaporte, I want to know your opinion:

  1. Shall I target 5.4.x or master?
  2. Shall I upgrade test projects to .NET 7?

@hazzik hazzik changed the title Add support for C#11 static interface members Fix support for static interface members May 2, 2023
@hazzik hazzik changed the title Fix support for static interface members Fix building proxies for classess with static interface members May 2, 2023
@bahusoid
Copy link
Member

bahusoid commented May 2, 2023

Shall I target 5.4.x or master?

5.4

Shall I upgrade test projects to .NET 7?

No opinion. It seems static members are tested without it.

with one exception of interface-only entities for which, if such interface would have an abstract static member it must be implemented by a proxy.

I don't get it. Can you provide an example?

@hazzik
Copy link
Member Author

hazzik commented May 2, 2023

No opinion. It seems static members are tested without it.

I wrote my comment before I discovered that static interface members do not work in lower .NET (Core) versions.

I don't get it. Can you provide an example?

This is the modification of the test that will fail in .NET 7:

[Test(Description = "GH3295")]
public void VerifyProxyForInterfaceWithStaticAbstractMethod()
{
	var factory = new StaticProxyFactory();
	factory.PostInstantiate(
		typeof(IWithStaticMethods).FullName,
		typeof(object),
		new HashSet<System.Type> { typeof(INHibernateProxy), typeof(IWithStaticMethods) },
		null, null, null, true);

	var proxy = factory.GetProxy(1, null);
	Assert.That(proxy, Is.Not.Null);
}

There are two scenarios where this can be used:

  1. Using <class name="Smth" proxy="{interface}"> option in the mapping (chapter 21.1.3 of the documentation):

    <class name="Person" proxy="IPerson">
      <id name="Id" />
      <property name="Name" />
    </class>
    public interface IPerson
    {
      int Id { get; }
      string Name { get; set; }
      static abstract void HelloWorld(); // <-- this will fail in .NET 7
    }
    
    public class Person : IPerson
    {
      public int Id { get; private set; }
      public string Name { get; set; }
      public static void HelloWorld() { }
    }
  2. Another one is interface-only entities (<class name="{interface}">, vaguely described in 9.1.1. Table per class hierarchy and 9.1.2. Table per subclass of the documentation):

    <class name="IPerson">
      <id name="Id" />
      <property name="Name" />
    </class>
    public interface IPerson
    {
      int Id { get; }
      string Name { get; set; }
      static abstract void HelloWorld(); // <-- this will fail in .NET 7
    }

However, as I said, it does not make much sense for to.

@hazzik hazzik changed the base branch from master to 5.4.x May 2, 2023 23:25
@fredericDelaporte fredericDelaporte added this to the 5.4.3 milestone May 8, 2023
@fredericDelaporte
Copy link
Member

To enable the test for this case, yes we should switch net6.0 target to 7.0 in the test project.

Ok for the target branch.

@hazzik hazzik enabled auto-merge (squash) May 8, 2023 22:47
@hazzik hazzik added the r: Fixed label May 8, 2023
@hazzik hazzik merged commit 43c5fce into 5.4.x May 9, 2023
14 checks passed
@hazzik hazzik deleted the gh-3295 branch May 9, 2023 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# 8/11 Static interface members support
3 participants