Support instance member overriding a static member with union types #48
Conversation
niloc132
force-pushed
the
overloaded-methods
branch
from
65894de to
b9c348b
Compare
|
Are there changes or improved tests you'd like to see in this? |
|
Julien is OoO for 2 weeks. I recommend pinging him again when he comes back. |
|
Thanks, will do! |
|
Hey guys, checking in again on this, other issues filed - anything you'd like to see in this changeset to get it ready, or a better approach to handle this issue? |
| function Foo() {} | ||
|
|
||
| Foo.noArgMethod = function() {}; | ||
| Foo.prototype.noArgMethod = function() {}; |
There was a problem hiding this comment.
You can remove these two line, we already have these tests in simpleclass package
There was a problem hiding this comment.
Sounds good - my thinking was to have them all be covered here as well, so that one didn't have to scan all test packages to find other overload tests, but your point makes good sense too.
| /** | ||
| * @type {string|number} | ||
| */ | ||
| Foo.prototype.unionProperty; |
There was a problem hiding this comment.
I would put all test with union type into the uniontype test package.
There was a problem hiding this comment.
Will do, I wasn't sure if this justified its own test package or not.
| /** | ||
| * @type {function():void} | ||
| */ | ||
| Foo.prototype.functionProperty; |
There was a problem hiding this comment.
same here, move the tests in the function type test package
There was a problem hiding this comment.
I think we don;t need these tests for this cl.
| @@ -80,12 +80,18 @@ public void applyTo(Program program) { | |||
|
|
|||
| @Override | |||
| public boolean shouldProcessField(Field field) { | |||
| if (field.isStatic()) { | |||
| currentNameStack.push("Static"); | |||
| } | |||
There was a problem hiding this comment.
I need to think more about that. This is a breaking change. every static fields/methods with an union type will see a change in the name of their UnionType helper.
There was a problem hiding this comment.
Agreed, I don't love it either, but was trying to take my cue from the entity.setName(name + "_STATIC"); line in MembersClassCleaner, which at least only adds the suffix if necessary. I think was a reason I had to put this in the visitor instead of the loop at the end, but I've forgotten now what it was, I'll try it again to see if I can remember, or if I can move to that loop and only change already-broken code, rather than modify all static fields, methods.
There was a problem hiding this comment.
The case I'm specifically working on here is that the protobuf impl for JS offers instance and static methods with the same names, and the same overloads, so the exact same union gets generated twice. In a specific case like that, it could even be possible to observe that the union and the method name are the same, so share the union type rather than generate it again.
There was a problem hiding this comment.
I changed this code a bit - it isn't pretty, but hoping it touches the basics of what you were thinking of? In short, if the field is static, and there is another field with the same name, then we need the Static suffix - naturally this won't affect any existing code, since it would have been affected by this bug.
For methods it is slightly more complicated, we need to only test the non-overlay methods, since those just call the one "real" method, which takes the union type.
I havent updated tests yet, but presently all existing and new tests pass - once we settle on the approach I'll get those in line with what you are requesting there too?
There was a problem hiding this comment.
I think we should take another approach. The MembersClassCleaner is doing too much thing: it's renaming methods/fields, removing constructors/methods or fields and modifying method's generics. We should split that in different visitors: I think at least two in order to modify the method generics in a separate visitor.
The visitor that will rename class members can be run before the UnionTypeMethodParameterHandler and the one that modify method's generics need to be called after the FixReferencesToDuplicatedTypes visitor.
I think it will solve all the issues.
There was a problem hiding this comment.
Okay, I'll close this for now - it will probably be a few months until I have something to show here, I'm going on personal leave sometime in the next week and will not be in a position any more to take on work like this.
There was a problem hiding this comment.
No problem and thanks your patch and opening the issue. I can take over the work. Enjoy your leave!!
There was a problem hiding this comment.
Hi, checking back on this, are you still able to resolve this some other way?
Proposed fix to disambiguate the generated union type for a static and instance member with the same name (and for a method, with the same param name). A nicer fix might be where the enclosing type only takes a single union type for the given method name plus arg name plus types, so that it could be reused, but this is not a general fix.
It could also perhaps be possible to reorder
MembersClassCleanerandUnionTypeHelperTypeCreatorinVisitorHelper(so that_STATICis already appended and could be used when generating the union type), but there seem to be other dependencies on the order as it is.Fixes #46
Does not address #47.