-
Notifications
You must be signed in to change notification settings - Fork 74
Support explicit Interface implementation #397
Comments
I don't really understand what you mean. It should already be possible to do explicit implementation, like
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:
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:
This issue will be fixed in version 3.0 (Roslyn-based). |
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 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 In essence we are prototyping the following changes:
These will occur automatically with no need to add attributes. If you add a 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 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() { ... }
} |
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 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. |
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 Your fourth example is invalid, there is no method |
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.
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).
Agreed, it was only included for completeness, not to imply it doesn't currently work; sorry if that was confusing.
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).
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.
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 What we're proposing is to reconsider this and instead view At that moment when you call In our proposal calling 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 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? |
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. |
We have confirmed all the information is present to modify // 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 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)? |
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
should generate
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 For your (unrelated) suggestion to allow a method to implement multiple interface members with different script names, or using
What I mean with proxy objects is that it would be possible to make code like this:
translate to (something similar to)
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. |
Thanks for the response.
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 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.
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 |
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.
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
No worries.
because the call-site rewriting is usually not possible. If the method
Now inside |
Another advantage of the proxy solution is that it could be use in situations like
In this case both the |
And the proxy method could also be used to solve #159
could generate code like
|
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.
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.
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.
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.
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 // 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)
Agreed, but it's unnecessary if we know the compiler always adds implicit and explicit interface method names to the prototype of classes. In
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. |
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 |
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. |
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. |
Sidenote: if you have
I do not see an obvious way to implement |
Thanks for taking the time to consider our request, I'll leave it there.
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. |
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:
Will produce the following:
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:
Compiles to:
However, when accessing a member on an object typed (at compile time) as IFoo, the compiler can instead write out:
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 asss.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:
We can now compile this to:
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
The text was updated successfully, but these errors were encountered: