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

Open classes/Virtual extension methods #11211

Closed
orthoxerox opened this issue May 10, 2016 · 26 comments
Closed

Open classes/Virtual extension methods #11211

orthoxerox opened this issue May 10, 2016 · 26 comments

Comments

@orthoxerox
Copy link
Contributor

orthoxerox commented May 10, 2016

I propose to extend "extension everything" (#11159) with virtual methods. In my opinion, the conservative approach of MultiJava should be good enough to handle most use cases and can be relaxed later.

  1. Virtual extension methods can be marked virtual and override. Extension methods cannot be marked abstract or new.
  2. Virtual extension methods cannot have interfaces or value types in the pseudo-receiver position.
  3. Virtual extension methods marked override must belong to the same static class as the virtual method being overridden.
  4. Virtual extension methods are automatically incorporated into the class bodies if all class bodies are declared in the same assembly as extension methods. A stub extension method remains in the static class where they've been described.
class A { }
class B : A { }
class C : A { }

class FooExtension
{
    public virtual static int Foo(this A @this) => ...
    public override static int Foo(this B @this) => ...
    public override static int Foo(this C @this) => ...
}

//is transformed into

class A
{
    public virtual int Foo() => ...
}

class B : A
{
    public override int Foo() => ...
}

class C : A
{
    public override int Foo() => ...
}

class FooExtension
{
    public static int Foo(A @this) => @this.Foo();
}

\5. Virtual extension methods that are declared in a different assembly than at least one of the classes being overridden are implemented as a typeswitch method with multiple private helper methods:

class FooExtension
{
    public virtual static int Foo(this AnotherAssembly.A @this) => ...
    public override static int Foo(this AnotherAssembly.B @this) => ...
    public override static int Foo(this AnotherAssembly.C @this) => ...
}

class FooExtension
{
    public static int Foo(this AnotherAssembly.A @this)
    {
        switch (@this) {
            case AnotherAssembly.B b:
                return FooHelper(b);
            case AnotherAssembly.C c:
                return FooHelper(c);
            default:
                return FooHelper(a);
        }
    }

    private static int FooHelper(AnotherAssembly.A @this) => ...
    private static int FooHelper(AnotherAssembly.B @this) => ...
    private static int FooHelper(AnotherAssembly.C @this) => ...
}

This approach solves the use case of sharing one inheritance hierarchy between two assemblies and using virtual dispatch in both. It doesn't solve use cases like adding your own efficient LINQ methods to specialized enumerables, but I think traits/higher-order generics will serve better.

@alrz
Copy link
Member

alrz commented May 10, 2016

#258

@orthoxerox
Copy link
Contributor Author

@alrz Nope, that's two different things. My proposal is about extension methods that are literally virtual and it requires no CLR changes. 258 is about default implementations of interface methods.

@HaloFour
Copy link

What's the point of declaring these methods as virtual static methods in a static extension class if the methods have to then be added to the target classes directly by the compiler? Why wouldn't the programmer just add the virtual members directly to the target class instead? Using extension methods in this way seems like it would be incredibly confusing since you can't extend anything outside of your assembly, and why would you ever write an extension method for a type that you already own and can modify?

@alrz
Copy link
Member

alrz commented May 10, 2016

@orthoxerox Yes it's about interfaces because the point is to be able to alter virtual dispatch from outside of the class hierarchy, i.e. interface default methods (or potentially extension implementations #8127).

IFoo foo = GetFoo();
// virtual dispatch to the implementation,
// or the default implementation 
// or the extension implementation
foo.Method(); 

The pattern that you described looks like conflating functional paradigm with OOP design. Roslyn itself does this all the time, you switch on the syntax node and call the appropriate method. Imagine all of these methods was actually defined inside syntax class hierarchy as virtual methods. That'd be maintenance hell.

@orthoxerox
Copy link
Contributor Author

@alrz switching on the syntax node is exactly the same amount of maintenance hell as overriding all the virtual methods of a class, except you have to order your manually written dispatch tables yourself. (Extension virtual methods would be even easier to use if you could have abstract methods, but alas, this cannot be supported without runtime errors until enum classes are implemented)

@HaloFour in case of the same assembly the only benefit to writing extension virtual methods is code organization, just as it is for partial classes. Including the methods into the classes during compilation allows others to override them further in derived assemblies (this is impossible when they are written as typeswitch methods). However, the real purpose is extending classes from other assemblies with additional virtual methods, because typeswitch methods are maintenance hell.

@HaloFour
Copy link

@orthoxerox

My proposal is about extension methods that are literally virtual and it requires no CLR changes.

However, the real purpose is extending classes from other assemblies with additional virtual methods, because typeswitch methods are maintenance hell.

Those two statements are in conflict. There is nothing in the CLR that would allow you to amend a type in a different assembly and add new virtual methods.

@orthoxerox
Copy link
Contributor Author

@HaloFour They are not. Classes in the same assembly are extended, classes in another assembly are left untouched and the methods are rewritten as a single typeswitch static extension method (see point 5 of the proposal).

@HaloFour
Copy link

HaloFour commented May 10, 2016

@orthoxerox I see. How do you propose to resolve the issues of type switching with interfaces in play? (point 2, although I don't think ignoring interfaces is a good idea)

Why wouldn't the overloads of Foo remain public?

@orthoxerox
Copy link
Contributor Author

@HaloFour what name would they have? Their original name is taken by the generated method. Interfaces are excluded because the problem space has already been investigated by Chambers et al, and the restriction copied into my proposal ensures no runtime errors or unresolvable conflicts. Perhaps it can be relaxed later when the design team has time to explore the problem space.

@HaloFour
Copy link

@orthoxerox Why couldn't they just be Foo? And instead of emitting separate helper methods you could just have the type switch ensure that its dealing with the correct type or dispatch to the appropriate overload.

public class A { }
public class B : A { }
public class C : A { }
public class D : C { }

class FooExtension
{
    public virtual static int Foo(this AnotherAssembly.A @this) => ...
    public override static int Foo(this AnotherAssembly.B @this) => ...
    public override static int Foo(this AnotherAssembly.C @this) => ...
    public override static int Foo(this AnotherAssembly.D @this) => ...
}
class FooExtension
{
    public static int Foo(this AnotherAssembly.A @this) {
        switch (@this) {
            case AnotherAssembly.D d:
                return Foo(d);
            case AnotherAssembly.C c:
                return Foo(c);
            case AnotherAssembly.B b:
                return Foo(b);
        }
        // default logic for Foo(A) here
    }


    public static int Foo(this AnotherAssembly.C @this) {
        switch (@this) {
            case AnotherAssembly.D d:
                return Foo(d);
        }
        // default logic for Foo(C) here
    }

    // no need to rewrite either method to include a type switch since there is no more specific type
    public static int Foo(this AnotherAssembly.B @this) => ...;
    public static int Foo(this AnotherAssembly.D @this) => ...;
}

@orthoxerox
Copy link
Contributor Author

orthoxerox commented May 10, 2016

@HaloFour What will happen is this case, then?

B b = new D();
b.Foo();

You would have to copy the typeswitch into every method to make virtual dispatch work, and then there'd no way to call them statically anyway, so you're better off with a single method.

@HaloFour
Copy link

@orthoxerox

What will happen is this case, then?

Compiler error, D doesn't derive from B.

You'd only need a type-switch in each of the overloads where the "virtual" argument could have a more derived type. In my example, there were no overloads of a more specific type than B so Foo(B) does not require a type-switch. Neither does Foo(D). But Foo(C) does require a type-switch since a C could be a D and Foo(A) also requires a type-switch since an A can be B, C or D.

If you were to change my example so that each class would be further derived from the previous class then each overload except the last would have at least a partial type-switch to cover the potentially more derived cases, e.g.:

public class A { }
public class B : A { }
public class C : B { }
public class D : C { }

class FooExtension
{
    public virtual static int Foo(this AnotherAssembly.A @this) => ...
    public override static int Foo(this AnotherAssembly.B @this) => ...
    public override static int Foo(this AnotherAssembly.C @this) => ...
    public override static int Foo(this AnotherAssembly.D @this) => ...
}
class FooExtension
{
    public static int Foo(this AnotherAssembly.A @this) {
        switch (@this) {
            case AnotherAssembly.D d:
                return Foo(d);
            case AnotherAssembly.C c:
                return Foo(c);
            case AnotherAssembly.B b:
                return Foo(b);
        }
        // default logic for Foo(A) here
    }

    public static int Foo(this AnotherAssembly.B @this) {
        switch (@this) {
            case AnotherAssembly.D d:
                return Foo(d);
            case AnotherAssembly.C c:
                return Foo(c);
        }
        // default logic for Foo(B) here
    }

    public static int Foo(this AnotherAssembly.C @this) {
        switch (@this) {
            case AnotherAssembly.D d:
                return Foo(d);
        }
        // default logic for Foo(C) here
    }

    // no need to rewrite either method to include a type switch since there is no more specific type
    public static int Foo(this AnotherAssembly.D @this) => ...;
}

@orthoxerox
Copy link
Contributor Author

@HaloFour sorry, D was derived from B in my example. But why do you want to have all these methods if many of them have to be pseudo-virtual? Better performance for leaf classes?

@HaloFour
Copy link

@orthoxerox Just feels cleaner to me. You end up with the same number of public methods with the same signatures as you've written and don't need any compiler-generated methods. You could still have compiler-generated methods for leaf nodes to avoid the double type-switch when passing an intermediate type (e.g. passing a B to Foo(A)) but I think that overhead is minimal and stack traces would look nicer without a synthetic method in the mix.

@alrz
Copy link
Member

alrz commented May 11, 2016

Passing a dynamic would do the same, no?

@orthoxerox
Copy link
Contributor Author

@alrz dynamic depends on the discipline of the consumer of your classes, while this feature provides compile-time guarantee that the call will be successfully dispatched.

@alrz
Copy link
Member

alrz commented May 11, 2016

In my opinion, this seems like a code generation problem, you could use code generators to generate the type switch based on [Virtual] extention methods.

@HaloFour
Copy link

@alrz

You're right.

I think that generators are going to become a very interesting playground for potential language features going forward.

@alrz
Copy link
Member

alrz commented May 11, 2016

@HaloFour Yeah, in fact we are using virtual and override as marker keywords here, these are not actual virtual and override methods,

static partial class FooExtension {
    [Virtual]
    public static int Foo(this A @this) {..}
    // private to prevent static dispatch
    private static int Foo(this B @this) {..}
    private static int Foo(this C @this) {..}
}

static partial class FooExtension {
  replace public static int Foo(this A @this) {
    switch(@this) {
      case C c: return c.Foo();
      case B b: return b.Foo();
    }
    return original(@this);
  }
}

etc.

@orthoxerox
Copy link
Contributor Author

@alrz This depends on how powerful generators are. replace/original aren't powerful enough to rewrite several methods and verify that they are compatible with each other.

@alrz
Copy link
Member

alrz commented May 11, 2016

@orthoxerox In fact, they are. you can simply use replace on any method you want. Though, in this particular case, you just need to replace one of the methods as you can see the example above. As for powerfulness of generators, it actually depends on the one who writes them, I agree that writing this generator might be tricky (particularly the part that you need to order case labels so that they do not subsume each other, however, you will likely get warnings if you didn't do it right), but it certainly is not impossible in any ways.

@HaloFour
Copy link

HaloFour commented May 11, 2016

Actually I'd think that this would be pretty simple with generators:

MyExtensions.cs:

public class A { }
public class B : A { }
public class C : B { }
public class D : C { }

public partial static class MyExtensions {
    [Virtual] public static int Foo(this A a) => 1;
    [Override] public static int Foo(this B b) => 2;
    [Override] public static int Foo(this C c) => 3;
    [Override] public static int Foo(this D d) => 4;
}

MyExtensions.replace.cs:

public partial static class MyExtensions {
    replace static int Foo(A a) {
        switch (a) {
            case D d:
                return Foo(d);
            case C c:
                return Foo(c);
            case B b:
                return Foo(b);
            default:
                return original(a);
        }
    }

    replace static int Foo(B b) {
        switch (a) {
            case D d:
                return Foo(d);
            case C c:
                return Foo(c);
            default:
                return original(b);
        }
    }

    replace static int Foo(C c) {
        switch (a) {
            case D d:
                return Foo(d);
            default:
                return original(c);
        }
    }
}

@alrz
Copy link
Member

alrz commented May 11, 2016

@HaloFour You don't need to replace every method and generate a different switch for each one of them. I think the above example is much more straightforward in both genration and execution.

@HaloFour
Copy link

@alrz

There could be multiple strategies depending on the accessibility of the methods. I personally prefer the "overridden" methods to be public and to allow them to be called directly. This is how normal instance virtual methods work. Either way, even my more "complicated" version is still pretty simple and should be a relative cakewalk for a generator. The most complicated bit, in either case, is identifying the "overload" methods and constructing the type hierarchy, but that's still quite easy.

@alrz
Copy link
Member

alrz commented May 11, 2016

@HaloFour I think identifying overloaded methods is the easy part: every other methods with exact same name, number of parameters and types. I think the hard part is to order case labels so that they don't subsume each other. Anyways, the actual code generation is just a matter of preference.

@orthoxerox
Copy link
Contributor Author

Discussion continued in dotnet/csharplang#310

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants