reflect: implement NumMethod and Implements #907
Conversation
| panic("reflect: non-interface type passed to Type.Implements") | ||
| } | ||
| return u.AssignableTo(t) | ||
| if u.NumMethod() == 0 { |
There was a problem hiding this comment.
This is subtly incorrect, as an interface can include unexported methods. I am not sure though whether there is an easy way to solve this right now. If you wish to leave this for further work, consider adding a comment about it.
There was a problem hiding this comment.
Hmm this might be a bold thing to do but what if we reverted to the old (incorrect) behavior? It would make the implementation of Implements a lot easier, at least as long as we don't need to fully support it. The limited implementation here should be enough for packages like encoding/json.
There was a problem hiding this comment.
By which you mean, the same behavior as Go currently has? I would agree with that, lets implement the same bugs 🤡
There was a problem hiding this comment.
Yes, the same buggy behavior (to be bug-compatible 😉). Reading the bug report the final decision on whether the docs or the implementation should be fixed does not seem to be made yet.
There was a problem hiding this comment.
Fixing this is slightly less trivial than I expected: I discovered that anonymous interface types (like var x interface{ F() }) do not have the correct NumMethod.
There was a problem hiding this comment.
What is the status with this one, @aykevl ?
There was a problem hiding this comment.
I need to look into this further, haven't done that yet.
There was a problem hiding this comment.
Reminder that we still need to resolve this. 😸
These two functions are needed by the encoding/json package.
aykevl
force-pushed
the
reflect-nummethods
branch
from
dd5298f to
55d5f1c
Compare
|
I have updated this branch to be bug-compatible with Go but now it isn't complete enough for encoding/json, so this will need a bit more work. |
|
I have implemented Implements in a slightly different way (at compile time). This PR might still be useful one day, but for now I've solved it in a different way. Closing, for now. |
These two functions are needed by the encoding/json package.