Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

Support explicit Interface implementation #397

Closed
thargy opened this issue Mar 5, 2015 · 19 comments
Closed

Support explicit Interface implementation #397

thargy opened this issue Mar 5, 2015 · 19 comments

Comments

@thargy
Copy link

thargy commented Mar 5, 2015

Hi, after reviewing issue #395, I've talked this over in more detail with my team. Explicit interface implementation is actually quite an important case in C# development. A common pattern is to have a generic and non-generic version of an interface, where the generic version's members override the non-generic members, being more type-specific. The most obvious example is IEnumerable<T>, but there are numerous other examples in the framework and associated libraries (and in our own libraries).

There are also occasions when a single class implements two interfaces that have a common member name, and explicit implementation is then a necessity (see issue #275, which is currently resolved with something of a hack).

One solution is to have runtime member resolution, but it would be complex to introduce and would impact performance so the cost benefit is probably not there (as you've concluded). However, there are other approaches, Salterelle already solves a similar problem in a compile-time way - method overload resolution, which it does with the old-school trick of giving overload a new name, and using call-site rewriting. As you already support call-site writing in your framework, then we have a much easier (and efficient way) to solve the problem.

Firstly, when creating class prototypes in the Javascript we add an explicit member for every interface member, whether implicit or explicit in the C#, for example:

interface IFoo
{
    int DoSomething();
}

class Foo : IFoo
{
    public int DoSomething()
    {
        throw new NotImplementedException();
    }
}

Will produce the following:

(function () {
    'use strict';
    var $asm = {};
    ss.initAssembly($asm, 'Saltarelle Explicit Interface Bug');
    ////////////////////////////////////////////////////////////////////////////////
    // InterfaceExample.Foo
    var $InterfaceExample_$Foo = function () {
    };
    $InterfaceExample_$Foo.__typeName = 'InterfaceExample.$Foo';
    ////////////////////////////////////////////////////////////////////////////////
    // InterfaceExample.IFoo
    var $InterfaceExample_$IFoo = function () {
    };
    $InterfaceExample_$IFoo.__typeName = 'InterfaceExample.$IFoo';
    ss.initInterface($InterfaceExample_$IFoo, $asm);
    ss.initClass($InterfaceExample_$Foo, $asm, {
        $doSomething: function () {
            throw new ss.NotImplementedException();
        }
    }, null, [$InterfaceExample_$IFoo]);
})();

ss.initClass then adds $doSomething to the prototype of the new $InterfaceExample_$Foo class. We can modify this behaviour so that it also adds explicit members for the members of each interface implemented by the class (in this case $InterfaceExample_$IFoo). So as well as having a $doSomething member it will also have a $InterfaceExample_$IFoo_$doSomething member that directly references the value of $doSomething. In this way calling $InterfaceExample_$IFoo_$doSomething will be identical (in cost) to calling $doSomething.

Once this change is made to ss.initClass, we then need to change the compiler so that any call to an interface member uses this new name. Currently:

IFoo foo = new Foo();
foo.DoSomething();

Compiles to:

var foo = new $InterfaceExample_$Foo();
foo.$doSomething();

However, when accessing a member on an object typed (at compile time) as IFoo, the compiler can instead write out:

var foo = new $InterfaceExample_$Foo();
foo.$InterfaceExample_$IFoo_$doSomething();

Although there is a trivial performance hit on initial class creation (the modification to ss.initClass will cause it to run slightly slower on classes that implement interfaces) this cost is paid only once. There is also a very small memory increase, but as most interfaces are implicit then this is the cost of a new pointer on the prototype which is pretty trivial. However, the runtime cost of calling the method in this way is completely unchanged, as it is just a new reference.

With these two small changes, we can now move to allowing support for explicit interfaces. In fact it's a pretty trivial compiler enhancement. When we see an explicit interface implementation we add the function to the anonymous object passed to ss.initClass and use it's new explicit name. So long as ss.initClass only adds explicit interface members to the prototype where the function hasn't already been supplied then we're done.

For example, if we change Foo:

class Foo : IFoo
{
    public object DoSomething()
    {
        throw new NotImplementedException();
    }
    int IFoo.DoSomething()
    {
    }
}

We can now compile this to:

(function () {
    'use strict';
    ... // Skipped unchanged code
    $InterfaceExample_$IFoo.__typeName = 'InterfaceExample.$IFoo';
    ss.initInterface($InterfaceExample_$IFoo, $asm);
    ss.initClass($InterfaceExample_$Foo, $asm, {
        $doSomething: function () {
            throw new ss.NotImplementedException();
        },
        $InterfaceExample_$IFoo_$doSomething: function() { }
    }, null, [$InterfaceExample_$IFoo]);
})();

And we have full support for explicit interfaces. The various edge case checks needed to ensure this approach is safe are already the responsibility of the C# compiler.

We've investigated the compiler and believe this change is relatively easy to achieve, and will in fact remove the need for the duplicate member check currently present, and we don't need the previously proposed IgnoreDuplicateMembersAttribute.

We would like to submit a pull-request but want to check with you whether you agree with this approach before proceeding.

Finally, we are seriously considering using Salterelle in one of our core projects, so I am concerned that we don't spam you with pull-requests and issues. We have our own fork to help prevent this, and will endeavour to only supply quality pull-requests after we've evaluated fully, please feel free to contact me if you have any concerns, or you feel my teams are not being constructive. We are keen to work with you in the spirit of open source collaboration.

Thanks,

Craig

@erik-kallen
Copy link
Contributor

I don't really understand what you mean. It should already be possible to do explicit implementation, like

interface I1 {
    [ScriptName("m1")]
    void M();
}
interface I2 {
    [ScriptName("m2")]
    void M();
}
class C : I1, I2 {
    public void M() {} // implements I1.M, script name = m1
    void I2.M() {} // implements I2.M, script name = m2
}

A problem only arises when the members have the same script name, because that would mean that we would need two methods with the same name on the same object, which is for obvious reasons not possible. I have had thoughts about solving this with proxy objects, but I haven't thought through all possible issues with that solution.

Is your suggestion to automatically add namespace and interface names to the script names of all interface members? I do not think that is a good idea.

As I read it, your post also touches on a separate problem: that code like this is not possible:

interface I1 {
    [ScriptName("m1")]
    void M();
}
interface I2 {
    [ScriptName("m2")]
    void M();
}
class C : I1, I2 {
    public void M() {} // implements both I1.M and I2.M, compile-time error because the method needs two different names
}

This does not have a conceptual problem, it is just a (yet) unimplemented feature.

And then there is an issue with the 2.x branch that code like this is not allowed either:

interface I1 {
    [ScriptName("m")]
    void M();
}
interface I2 {
    [ScriptName("m")]
    void M();
}
class C : I1, I2 {
    public void M() {} // implements both I1.M and I2.M, v2.x of the compiler reports an error because it is not clever enough to understand that there is no problem.
}

This issue will be fixed in version 3.0 (Roslyn-based).

@thargy
Copy link
Author

thargy commented Mar 5, 2015

You've raised a number of separate issues here, so I'll try to address them all as I understand them. We weren't aware of the ScriptName workaround you propose, but, as you say, it doesn't work in the 2nd and 3rd examples you give. The solution we propose solves both explicit interface implementation (automatically) AND allows a single member to implement multiple interfaces.

We've implemented the proposal and it is a surprisingly small amount of code changes, but we will test thoroughly and upload a pull request tomorrow if it passes testing (we need to check generics and finish the changes to ss.initClass). At that point it might be easier to see what we're suggesting. We'll also look at the impact of the ScriptName attribute.

In essence we are prototyping the following changes:

  1. Every call to a member via an interface uses a member name that includes the interface type name, e.g. $InterfaceExample_$IFoo_$doSomething for InterfaceExample.IFoo.DoSomething. (This is similar to what happens when you specify ScriptName attributes).
  2. Every implicitly implemented interface will have a member added to the prototype for the fully qualified name that will point directly to the class implementation. e.g. $InterfaceExample_$IFoo_$doSomething will be set to the value of $doSomething on the $InterfaceExample_$Foo class prototype. Both $doSomething and $InterfaceExample_$IFoo_$doSomething will exist on the prototype and point to the same function. (This will be done automatically by ss.initClass)
  3. Every explicitly implemented interface will directly set the value of the fully qualified name (Script Name), rather than pointing it to an existing function (it will just pass the function definition in to ss.initClass as in my example in the proposal).

These will occur automatically with no need to add attributes. If you add a ScriptName attribute to an interface then the fully qualified member name (as shown above) will be replaced with the script name instead, so in you example $Namespace_$I1_$m would be m and '$Namespace_$I2_$m' would be m2.

The proposal does create a default 'script name' on interfaces that is fully qualified, as it is essential to avoid collision. It is matched by creating an 'alias' on the class prototype that maps the script name to the actual function, when the class implicitly implements the interface; or a custom explicit implementation.

The actual method implementation will still exist on the class prototype. We need to test the existing behaviour of your example, but this may be the difference? i.e. under our proposal both I1.m and I1.$m would exist on the prototype and point to the same function.

Under our proposal your third example can still be considered valid (with the same fix you'd need to make already). However, the following would never be valid:

interface I1
{
    [ScriptName("m")]
    void M();
}
interface I2
{
    [ScriptName("m")]
    void M();
}
class C : I1, I2
{
    /*
     * Implements I1.m as 'm' on the prototype (instead of '$Namespace_$I1_$m')
     */
    void I1.M() { ... } 

    /*
     * Invalid as this would redefine 'm' on the prototype (if ScriptName was absent 
     * then this would be valid as it would implement '$Namespace_$I2.$m' which would not
     * collide
     */
    void I2.M() { ... }
}

@thargy
Copy link
Author

thargy commented Mar 5, 2015

To keep things separate, here are some edge cases that I hope make it clearer:

Explicit only implementation

interface I1
{
    void M();
}
class C : I1
{
    /*
     * Would add $Namespace_$I1_$m function to class prototype.
     */
    void I1.M() { ... }
}

Multiple interface implementation

interface I1
{
    void M();
}
interface I2
{
    void M();
}
class C : I1, I2
{
    /*
     * Would add '$m' to the class prototype, and ss.initClass would add:
     *   $Namespace_$I1_$m = $m
     *   $Namespace_$I2_$m = $m
     */
    public void M() { ... }
}

Mixture of explicit and implicit implementations

interface I1
{
    void M();
}
interface I2
{
    void M();
}
class C : I1, I2
{
    /*
     * Would add '$m' to the class prototype, and ss.initClass would add:
     *   $Namespace_$I1_$m = $m
     */
    public void M() { ... }

    /*
     * Would add '$Namespace_$I2_$m' to the class prototype.
     */
    void I2.M() { ... }
}

Interface Inheritance
Note that due to call-site rewrite rules, all members have to be present for an interface, including inherited ones - in this case I2.M

interface I1
{
    void M();
}
interface I2 : I1
{
}
class C : I2
{
    /*
     * Would add '$m' to the class prototype, and ss.initClass would add:
     *   $Namespace_$I1_$m = $m
     *   $Namespace_$I2_$m = $m
     */
    public void M() { ... }
}

I haven't shown properties for simplicity, but they are handled in the same way.

@erik-kallen
Copy link
Contributor

So it seems that what you want is to have the names of interface members start with the fully qualified name of the interface. I do not think that this is a good idea, but you can use [ScriptName] on your interfaces if you want this convention.

Your first example should already work, no problem.

Your second example will work in the 3.x branch.

Your third example requires different script names for the methods. As I have said, I do not think that it is a good idea to always include the name of the interface in the method names, but you can use [ScriptName]. Or we'd need to create some kind of proxy when converting a C to an I2.

Your fourth example is invalid, there is no method I2.M. I2 inherits I1, but that fact does not declare a method called I2.M, the only M method that exists is I1.M.

@thargy
Copy link
Author

thargy commented Mar 6, 2015

Sorry for taking a while to respond, we wanted to go back and test ScriptName thoroughly and look at the Roslyn branch to make sure we're not misunderstanding what you're saying.

So it seems that what you want is to have the names of interface members start with the fully qualified name of the interface. I do not think that this is a good idea, but you can use [ScriptName] on your interfaces if you want this convention.

Yes, in essence that's part of the solution. We're proposing automatically adding a script name for interface members that is the fully qualified name (to avoid name collision), when a script name is not manually specified.

You've mentioned a number of times that you think creating implicit script names is not 'a good idea'. We'd really like to understand why you think this, and would be grateful if you could explain your reservations. We don't believe we'd be creating a precedent, for example, you already generate script names automatically for method overloads to prevent name collision, and you already generate a default name (e.g. Member -> $member).

Your first example should already work, no problem.

Agreed, it was only included for completeness, not to imply it doesn't currently work; sorry if that was confusing.

Your second example will work in the 3.x branch.

We've tried the Roslyn branch, and it does work as written, but it doesn't work if you add explicit (and different) ScriptNames, as there's only one method trying to implement 2 different script names. This is effectively one of the cases we are attempting to fix using prototype aliases.

The problem is that multiple classes can implement the interfaces in different ways and to get scenario 2 working in the Roslyn branch you can't have different script names, which will then break the proposed solution for scenario 3, meaning that you can't implement both classes.

Our proposal aims to fix all the different scenarios automatically, and would still support explicit script names on interfaces and would even allow a different script name on the class' own implementation (which is currently invalid).

Your third example requires different script names for the methods. As I have said, I do not think that it is a good idea to always include the name of the interface in the method names, but you can use [ScriptName]. Or we'd need to create some kind of proxy when converting a C to an I2.

We're not really sure what you mean by the proxy solution in this scenario? A traditional runtime proxy would have a performance penalty, and we're proposing a solution that wouldn't suffer that fate.

Your fourth example is invalid, there is no method..

Agreed, we weren't sure whether the compiler could readily access the inheritance tree for the interfaces and so deduce where the member was declared, on further testing it already does this so there is no need to use an alias for the inherited members of interfaces. Sorry for the confusion.

Conclusion
Looking at the second example (the current non-working one), and after further study, the Saltarelle philosophy is to see the member C.M() as implementing 2 things I1.M() and I2.M(), and the script names of these methods are therefore used for the implementation of C.M() in the prototype for the C class (and any other classes that implement the interfaces).

What we're proposing is to reconsider this and instead view C.M() as implementing 3 things I1.M(), I2.M() and C.M() itself. This isn't possible in the current design as there's no real prototype aliasing support (i.e. you can't have multiple names referencing a single function implementation). What we're proposing is making it so that ss.initClass will add aliases (a property referencing the same function) automatically for interface members that are implicitly implemented, when it builds the prototype. Where interface members are explicitly implemented they are supplied using the script name defined on the interface to ss.initClass by the compiler.

At that moment when you call C.M() directly, it is considered as implementing the interface and so the call site rewriter will use the interface's script name. It appear's to be this reason that means Saltarelle doesn't support adding a ScriptName to a member name on a class when that member implements an interface. We believe that is conflating concepts and causing the various issues that are now trying to be worked around. By separating the concept of a class member implementation from the interface implementations you can resolve all these issues in what appears to be a more robust way.

In our proposal calling M() on an object of type C would call the script name for the member M() as defined on the class (not the interface or interfaces it implements). Calling M() on an object of type I1 would call the script name for member M() as defined on the interface I1, etc.

Because we're using aliases for implicit implementations then where interfaces define a colliding script name we don't have an issue for implicit implementations, we only have an issue if two different explicit implementations exist for two interface members with the same script name - which is the main rationale for adding a default fully qualified script name for interface members, so this scenario only occurs when someone defines colliding script names on interfaces, and then attempts to explicitly implement them. In reality the changes we're proposing don't require a default script name on interface members to be useful, but they certainly make the developer experience easier. The concept of aliasing interface names is still sound.

To allow this to work then the script name for I1.M() has to be available on the prototype for C as an 'alias' (i.e. pointing directly to the class' implementation of M), in the case that C implicitly implements I1.M(). In the case where C explicitly implements I1.M() then the function body will be provided to the ss.initClass directly.

Sorry for the wall of text, this is a very complex area and we're trying to avoid any misunderstanding. Thank you for taking the time to look at this!

We'd be happy to provide a pull request demonstrating the proposal's implementation so that you can review the exact changes necessary (which we feel are quite small), if that is something you would prefer?

@thargy
Copy link
Author

thargy commented Mar 6, 2015

Here is another example that doesn't build in the Rosyln branch:

public interface I1
{
    void M();
}

public class Base : I1
{
    public virtual void M()
    {
        // This implements Base.M() and I1.M();
    }
}

// Note re-implmentation of I1 is valid in C#
public class C : Base, I1 {
    public override void M()
    {
        // This implements C.M()
    }

    void I1.M()
    {
        // This implements I1.M()
    }
}

But can with our changes.

@thargy
Copy link
Author

thargy commented Mar 6, 2015

We have confirmed all the information is present to modify ss.initClass to automatically add implicit interfaces names, however we can alternatively readily pass the majority of the work onto the compiler by adopting the following syntax:

// Register class 'C' that implements I1, I2 and I3, all four define a method M.
ss.initClass($InterfaceExample_$c, $asm, {
    // Class member C.M()
    $m: function () { ... },
    // Explicit implementation of auto-named I1.M()
    $InterfaceExample_$I1_$m: function() { ... },
    // Implicit implementation of auto-named I2.M()
    $InterfaceExample_$I2_$m: "$m",
    // Implicit implementation of I3.M() where ScriptName was explicitly set to 'm'.
    m: "$m"
}, null, [$InterfaceExample_$I1, $InterfaceExample_$I2, $InterfaceExample_$I3]);

This would almost entirely negate the work done at runtime by ss.initClass, all it does now is replace string values which it treats as keys in the same object, therefore "$m" would be replaced with the function definition supplied to $m (it creates an alias).

The call sites would look like this

var c = new $InterfaceExample_$c();
// c.M() <-- Note this used to map to the interface script name
c.$m(); // <-- But now the class implementation script name is allowed.
// ((I1)c).m()
c.$InterfaceExample_$I1_$m(); // <-- This is an auto name, with it's own implementation.
// ((I2)c).m()
c.$InterfaceExample_$I2_$m(); // <-- As is this, but it is an alias for $m
// ((I3)c).m()
c.m(); // <-- This is I3.M()'s explicit script name, and is an alias for $m

As you can see, the main difference here is that calling a method directly on the class is distinguished from calling via an interface. Hence having a ScriptName on a class member that implements an interface implicitly is now valid (this example doesn't show that though).

I'm hoping this makes the proposal clearer to understand too.

This aliasing syntax may find other uses down the line, for example you could add a 'ScriptAlias' attribute that works like 'ScriptName' but instead of performing call-site re-writing only adds an alias into the prototype (which could be useful for interop scenarios)?

@erik-kallen
Copy link
Contributor

The reason I don't want to include the namespace and interface name in interface member names by default is because I do not like the idea that the code

I1 o = GetSomething();
o.MyMethod();

should generate

var o = getSomething();
o.$SomeNamespace_$I1_MyMethod();

Using interfaces should IMO generate code that read equally nice as code using classes. But if you want that convention you can achieve it using [ScriptName]. Sorry, but this is a wontfix.

For your (unrelated) suggestion to allow a method to implement multiple interface members with different script names, or using [ScriptName] on a method that implements an interface members, I know perfectly well how to implement it, it just needs to be done and your support of the feature has been noted. I don't consider it a terribly high-priority feature, though, since the workaround is trivial:

interface I1 { [ScriptName("m1")] void M() {} }
interface I2 { [ScriptName("m2")] void M() {} }

class C : I1, I2 {
    [ScriptName("renamed")]
    public void M() {
        Console.WriteLine("Hello, world");
    }

    void I1.M() { return M(); }
    void I2.M() { return M(); }
}

What I mean with proxy objects is that it would be possible to make code like this:

interface I1 { [ScriptName("m")] void M() {} }
interface I2 { [ScriptName("m")] void M() {} }

class C : I1, I2 {
    public void M() {
        Console.WriteLine("1");
    }

    void I2.M() {
        Console.WriteLine("2");
    }
}

...

C c = new C();
I2 i = c;
c.M();
i.M();

translate to (something similar to)

ss.initClass($Namespace_C, $asm, {
    m: function() {
        console.log('1');
    },
    $someOtherName: function() {
        console.log('2');
    }
});

...

var c = new $Namespace_C();
var i = { m: c.$someOtherName.bind(c) };
c.m();
i.m();

Among things that need to be considered for this solution are: object reference comparisons and how it works together with generics. I don't think this would even cause any runtime performance hit (although I'm not a JS performance specialist), but if it does it can most likely not be measured anyway.

@thargy
Copy link
Author

thargy commented Mar 6, 2015

Thanks for the response.

Using interfaces should IMO generate code that read equally nice as code using classes. But if you want that convention you can achieve it using [ScriptName]. Sorry, but this is a wontfix.

I genuinely don't want to irritate you, now that I understand the issue is you consider it ugly there are alternatives, for example it would be trivial to use the same collision avoidance you use for overloads (i.e. prepending a $<number>, so you'd have something like $m$3 instead of $SomeNamespace_$I1_MyMethod). I didn't suggest that approach as using the FQN is deterministic (and hence fast), already immune to collision and I felt it was informative (someone debugging can easily see what is being called). I fully understand why you'd find it ugly.

What it was called wasn't the main point, the idea was to avoid having to manually fix collision issues by modifying the interfaces manually - something which is not readily possible if you're bringing in two separate libraries you don't control.

For example, if I'm using two libraries, both of which define a requirement to supply an interface for logging, e.g.

// Interface defined in first external library
interface Library1.ILogger {
    void Log(string msg);
}

// Interface defined in second external library
interface Library2.ILogger {
    string Log(string msg);
}

// My impementation
public class Logger : Library1.ILogger, Library2.ILogger {
    public string Log(string msg) { ... }
    void Library1.ILogger.Log(string msg) { this.Log(msg); }
}

Then the current implementation is that both of these have a 'default' script name of '$log'. If I wish to implement these interfaces in the same class I can't achieve it without modifying the two interfaces which I don't own. If the default behaviour was to supply unique names (like you do for overloads) then using the workaround becomes unnecessary.

Again, I note you say this is a WontFix, so please forgive me if you feel I'm pushing the point too far.

I know perfectly well how to implement it

Honestly, no disrespect was intended. If I'm verbose it is to aid communication and I'm sorry if I've inadvertently caused offence or implied that your were unaware of how to implement the code yourself. We reread the messages several times (my team proofed with me) before posting to try and ensure they were not confrontational, and I'll try harder in future.

Thanks for explaining the proxy idea, why do you prefer doing this over call-site rewriting, i.e.

var c = new $Namespace_C();
c.m();
i.$someOtherName();

Which seems significantly easier to achieve?

Again thank you for taking the time to consider these requests.

Craig

@erik-kallen
Copy link
Contributor

it would be trivial to use the same collision avoidance you use for overloads (i.e. prepending a $, so you'd have something like $m$3 instead of $SomeNamespace_$I1_MyMethod)

Not at all. The overload name generation only needs to look at a class, its base classes and interfaces it implements. Using this naming scheme for interfaces requires analyzing all classes that implement the interface, which is complex if the interface is internal and impossible if it is public.

What it was called wasn't the main point, the idea was to avoid having to manually fix collision issues by modifying the interfaces manually - something which is not readily possible if you're bringing in two separate libraries you don't control.

If you don't own the code implementing the interfaces, chances are that the author of that code shares my view that interface methods should look like something you would name the methods if you were actually writing the JS, and has used the [ScriptName] attribute on their members to ensure that they look nice, so the problem is still there. Is your example taken from a real-world situation or is it just hypothetical? I have written a fair amount of C# during my days and apart from implementing the non-generic IEnumerable, I don't know if I have ever run into the situation you are describing in real-world code.

Honestly, no disrespect was intended,

No worries.

why do you prefer doing this over call-site rewriting, i.e. ...

because the call-site rewriting is usually not possible. If the method I2.M has the script name `m``, it must be possible to invoke that method through that name. It is possible to use the actual name of the method in the special case of the actual (runtime) type of the variable being known, but say instead we have

class C : I1, I2 {
    public void M() {
        Console.WriteLine("1");
    }

    void I2.M() {
        Console.WriteLine("2");
    }
}

class D : I2 {
    public void M() {
        Console.WriteLine("3");
    }
}

class X {
    void F(I2 i) {
        i.M();
    }

    void Main() {
        C c = new C();
        D d = new D();
        F(c);
        F(d);
    }
}

Now inside F, we can't know the runtime type of i and rewrite the method name in the call.

@erik-kallen
Copy link
Contributor

Another advantage of the proxy solution is that it could be use in situations like

interface I<T> { void M(); }
class C : I<int>, I<string> {
    void I<int>.M() {
    }

    void I<string>.M() {
    }
}

In this case both the M methods have the same script name (whatever that name is).

@erik-kallen
Copy link
Contributor

And the proxy method could also be used to solve #159

[Imported]
class SQLite3Database : IDisposable {
    [ScriptName("close")]
    public void Dispose() {}
}

...

void Dispose(IDisposable d) {
    d.Dispose();
}

...

var db = new SQLite3Database();
Dispose(db);

could generate code like

var db = new SQLite3Database();
Dispose({ dispose: db.close.bind(db) });

@thargy
Copy link
Author

thargy commented Mar 7, 2015

Not at all. The overload name generation only needs to look at a class, its base classes and interfaces it implements. Using this naming scheme for interfaces requires analyzing all classes that implement the interface, which is complex if the interface is internal and impossible if it is public.

Agreed, the only deterministic alternatives that come to mind are encoding (which would achieve minimal compression) and hashing (which risks collision), both would be uglier and less information than the FQN proposed.

and has used the [ScriptName] attribute on their members to ensure that they look nice

I'm not sure if 'looks nice' would be the main reason they've attributed the interface, in our case we'd only be adding the attributes to perform the workarounds suggested.

Is your example taken from a real-world situation or is it just hypothetical?

Over the years I've come across it relatively infrequently. A recent example that springs to mind is integrating our custom logging solution with OWIN, ELMAH, Log4Net, etc. hence I gave the log example as this is probably the most common scenario.

apart from implementing the non-generic IEnumerable,

There are far more examples of this pattern in the BCL and popular libraries. An obvious one being IObservable (which is most commonly implemented via Rx in C# and RxJS in js). Ultimately this issue came about because of the IEnumerable problems we found, but IObservable would be the next area we probably come up against.

It is possible to use the actual name of the method in the special case of the actual (runtime) type of the variable being known, but say instead we have...

The example you give is exactly the scenario we propose to fix with call-site rewriting combined with prototype member aliases. Using such an approach we can avoid the need for runtime type checking entirely.

For example, our proposal would solve your example thus (assuming no ScriptName attributes are defined on I1 and I2.M() as a ScriptName of m):

// Class C, implements I1 & I2
ss.initClass($Namespace_C, ... {
    // Class implementation - C.M()
    $m: function() {
        console.log('1');
    },
    // Implicit implementation of I1.M()
    $Namespace_I1_$m: "$m",
    // Explicit implementation of I2.M()
    m: function() {
        console.log('2');
    }
} ...);

// Class D implements I2 only, note implicit interface members are specified
ss.initClass($Namespace_D, ... {
    // Class implementation - D.M()
    $m: function() {
        console.log('3');
    },
    // Implicit implementation of I2.M()
    m: "$m"
} ...);

// Class X
ss.initClass($Namespace_X, ... {
    $f: function(i) {
        // Because all classes that implement I2 have the m method defined, and we're calling m
        // on a object of type I2 (which is known at compile time) then we can safely use m here.
        i.m();
        // If 'i' were of type I2, we'd use i.$Namespace_I1_$m
        // If 'i' were of type C, we'd use i.$m
    },
    $main:  function() {
        var c = new $Namespace_C();
        var d = new $Namespace_D();
        this.$f(c);
        this.$f(d);
    }
} ...);

(Forgive typos as I did this by hand)

Now inside F, we can't know the runtime type of i and rewrite the method name in the call.

Agreed, but it's unnecessary if we know the compiler always adds implicit and explicit interface method names to the prototype of classes. In C and D you can see this in effect.

Among things that need to be considered for this solution are: object reference comparisons and how it works together with generics. I don't think this would even cause any runtime performance hit (although I'm not a JS performance specialist), but if it does it can most likely not be measured anyway.

The main hit is the extra object allocations required to generate proxies. Without a complex caching system then the number of allocations would be significant and result in extra GC pressure in the browser.

@thargy
Copy link
Author

thargy commented Mar 7, 2015

interface I<T> { void M(); }
class C : I<int>, I<string> {
    void I<int>.M() {
    }

    void I<string>.M() {
    }
}

Is solved, without proxies, by aliases too -

ss.initClass($Namespace_$C, $asm, {
        // Note class didn't define class member ($m).
        // Exact auto name style for generics TBC
        $Namespace_$I$ss.Int32_$m: function() {
        },
        $Namespace_$I$ss.string_$m: function() {
        },
    }, null, [ss.makeGenericType($Namespace_$I$1, [ss.Int32]),
                ss.makeGenericType($Namespace_$I$1, [ss.string])]);

In fact this is one of our test cases.

This illustrates a pretty good point to. Class C doesn't implement a method M, so calling C.M() in C# would be invalid, you can only call M via one of the interfaces. This is another reason why we're arguing for separating class members from interface implementations. The current compiler would expose a $m member on the JS class.

@thargy
Copy link
Author

thargy commented Mar 7, 2015

The final issue #159, could be solved using a proxy as you suggest, as we don't own the class in question so don't create the definition (and can't then add the aliases).

However, this problem can be overcome without proxies by modifying the prototype of imported class, in this way our call-site rewriter remains unchanged:

// Route one, modify imported class prototype directly (this is psuedo code there are better
// ways of doing this!
sqlite3.Database.prototype.$System_$IDisposable_$dispose = sqlite3.Database.prototype.close;

...
Dispose = function(d) {
   d.$System_$IDisposable_$dispose();
}
...

var db = new sqlite3.Database();
Dispose(db);

I am not a js expert, so I'll leave the endless debates over prototype manipulation to one of them.

@erik-kallen
Copy link
Contributor

Unless there is a massive user cryout for interface member names to contain the name of the interface (either directly or as a hash), this will not happen because I think the ugliness of the resulting script is not worth solving this uncommon case. Sorry, but you will need to use [ScriptName]. Or, if support for quirky edge cases with no regard for how the generated script looks you might be better off using JSIL.

Your suggestion for how to fix issue #159 is exactly what needs to be done currently (and is also in that rather long thread) but it would be nice if there were better support for it.

@erik-kallen
Copy link
Contributor

Sidenote:

if you have

interface I<T> { void M(); }
class C : I<int>, I<string> {
    void I<int>.M() {
    }

    void I<string>.M() {
    }
}

class X {
    void F<T>(I<T> i) {
        i.M();
    }
}

I do not see an obvious way to implement X.F without proxies.

@thargy
Copy link
Author

thargy commented Mar 7, 2015

Unless there is a massive user cryout for interface member names

Thanks for taking the time to consider our request, I'll leave it there.

I do not see an obvious way to implement `X.F without proxies.

Agreed, this scenario normally requires runtime resolution, which can be avoided by supplying a proxy as an argument to the parameter. It can still be resolved without proxies, using our alias system, if you treat generic methods as templates that are created on first use, which is quite easy to do. This would remove the extra memory requirement of creating new objects on each use.

Again, thank you for taking the time to respond to all the questions.

@erik-kallen
Copy link
Contributor

Created issues #404 and #405 for this.

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

No branches or pull requests

2 participants