#10276 Correct type hints for IHostnameResolver.resolveHostName #11589
Conversation
src/twisted/internet/_resolver.py
Outdated
| if TYPE_CHECKING: | ||
| from twisted.python.runtime import platform | ||
|
|
||
| if platform.supportsThreads(): |
There was a problem hiding this comment.
What does it mean to have an executable condition like this inside an if TYPE_CHECKING block, where the code will definitely never run?
There was a problem hiding this comment.
I think it works fine, I believe anything inside of TYPE_CHECKING is just ignored when CPython executes the code (hence why ThreadPool is quoted below). mypy seems to be OK with it, but I'm not sure if that actually executes the module (or how it picks up on what this is doing).
Note that this conditional is copied from twisted.internet.interfaces:
|
|
||
| # Make sure it's passed down as a native str, to maintain the interface | ||
| hostName = nativeString(hostName) | ||
| hostName = nativeString(hostName_bytes) | ||
|
|
||
| resolution = HostResolution(hostName) | ||
| resolutionReceiver.resolutionBegan(resolution) | ||
| onAddress = self._simpleResolver.getHostByName(hostName) |
There was a problem hiding this comment.
This line now doesn't match the interface for IResolverSimple which states that the timeout parameter is required. I'm not sure if I should:
- Pass the default timeout value here.
- Update the interface to reflect that a default value exists.
Any thoughts on what to do here?
(There's a similar error related to IHostnameResolver.resolveHostName on line 348.)
There was a problem hiding this comment.
We chatted a bit about this in #twisted:
12:04:11 PM - clokep: Not sure how to get feedback on an ongoing PR, but I left a question at https://github.com/twisted/twisted/pull/11589#discussion_r961057512 -- the tl;dr is Twisted internally seems to assume that implementations of an interface have a default value which the interface doesn't show. What's the proper solution to this?
12:04:37 PM - clokep: (To add the default to the interface or the provide the defaults in the caller or something else?)
12:07:49 PM - jean-paul[m]: The goal should usually be to fix the mismatch without breaking any code. Sometimes that's hard, maybe even hard enough that it's not worth doing, but that's the goal that should guide choices.
12:08:35 PM - jean-paul[m]: Since it's basically a point of extension, we should expect developers are implementing the interface as currently defined.
12:09:48 PM - jean-paul[m]: Buuuut if they did that, their implementation wouldn't actually work with Twisted, right? Because Twisted calls the method without a value for that parameter.
12:10:28 PM - jean-paul[m]: So from that perspective, the interface is wrong and should be updated to have a default - reflecting real usage by Twisted.
12:11:26 PM - jean-paul[m]: You could also look at it from the opposite perspective - someone wrote code that expects implementations of the interface and then uses it according to what the interface says. Fortunately, this means those implementations would already be passing a value for `timeout` since the interface says you have to.
12:11:40 PM - jean-paul[m]: So this code also wouldn't be broken by adding a default to the interface.
12:12:08 PM - jean-paul[m]: I don't think that's a complete case analysis but it's probably the major points. And I don't think this interface is an extremely popular extension point anyway.
12:12:21 PM - clokep: Right, my inclination was that adding a default to the interface is the "least" breaking thing to do.
12:12:40 PM - jean-paul[m]: That seems right to me.
12:13:01 PM - clokep: Awesome. Thanks for the help! Will try to finish that up "soon"!
12:13:23 PM - clokep: (I think connectTCP has a similar issue with parameters, will need to check the Synapse codebase again for ignores.)
12:15:00 PM - graingert[m]: I think the policy on type annotations is that they may break at any time right now
12:15:16 PM - graingert[m]: I can't find the docs on that
12:15:17 PM - graingert[m]: But they're currently provisional
12:15:42 PM - clokep: Yeah, I'm not really talking about the type annotations breaking -- I'm talking about type annotations finding bugs in old code. 😁
12:17:13 PM - graingert[m]: Ah right
The tl;dr is that option 2 from above seems more reasonable in this case (to avoid breaking code).
| @@ -747,7 +751,7 @@ def ebLog(error): | |||
| d.addCallback(cbDeliver) | |||
| d.addErrback(ebLog) | |||
| d.addBoth(lambda ignored: resolutionReceiver.resolutionComplete()) | |||
| return resolutionReceiver | |||
| return resolution | |||
There was a problem hiding this comment.
This changes the return value here, but it is never used:
twisted/src/twisted/internet/endpoints.py
Lines 959 to 961 in 6a8a70b
There was a problem hiding this comment.
Hm... might not be used by Twisted own code, but this looks like public API, so it might be used by someone else.
But I agree that the change makes more sense... since the old code just returns an input argument.
But this needs to follow the deprecations process... or ask for an exception
Do we still need @rtype: L{IResolutionReceiver} in the docstring ?
same for @type resolutionReceiver: L{IResolutionReceiver} ?
There was a problem hiding this comment.
Hm... might not be used by Twisted own code, but this looks like public API, so it might be used by someone else.
But I agree that the change makes more sense... since the old code just returns an input argument.
I don't think I was clear above -- this implementation (i.e. _nameResolver which gets set from a private class also in this file) returns the wrong type, which is never used. I don't see anyway for this to leak out to the public API. This is an internal-only change.
clokep
changed the title
clokep
force-pushed
the
resolver-types
branch
from
bc364f8
to
570f789
Compare
clokep
changed the title
clokep
changed the title
This corrects the type hints on IHostnameResolver.resolveHostName: * addressTypes is a sequence of IAddress types, not IAddress instances. * The return value should be IHostResolution, not IResolutionReceiver. Ports matrix-org/synapse#11328.
clokep
force-pushed
the
resolver-types
branch
from
570f789
to
8e3be61
Compare
|
please review |
Why? Just asking. |
Because the type hint was incorrect when added. The implementation of The incorrect documentation than got ported into an incorrect type hint in #1430. (I can't seem to deep link easily into that PR unfortunately.) When the endpoint resolver was added it seems like it was added by following the (incorrect) documentation: Luckily the twisted/src/twisted/internet/endpoints.py Lines 955 to 957 in 19a9a4c In summary, what we have is:
I'm asserting that the best way to not break things is to:
Honestly: this is the only way I can see us not breaking code. But it might (honestly it likely will) break type hints for anyone that is implementing As a final thought, I'm fairly certain the initial docstring was wrong (and the implementations are correct) since |
| """ | ||
| Create a L{HostResolution} with the given name. | ||
| """ | ||
| self.name = name | ||
|
|
||
| def cancel(self): | ||
| def cancel(self) -> NoReturn: |
There was a problem hiding this comment.
Hmm. IMHO this should really be -> None, to match the interface.
(And this is not this change's problem, but putting "@implementer on a superclass that does not in fact implement the interface, and just promotes a static type-check error into a dynamic NotImplementedError is an antipattern, let's do less of this in the future.)
There was a problem hiding this comment.
Good catch. I'm surprised mypy allowed this in the first place.
| [mypy-twisted.internet._resolver] | ||
| allow_untyped_defs = True | ||
| check_untyped_defs = False | ||
|
|
||
| [mypy-twisted.internet._signals] |
|
Hopefully that trivial import conflict resolution will not make CI mad, but if I flubbed it anybody else can feel free to fix it up for merge. |
Head branch was pushed to by a user without write access
Co-authored-by: Glyph <code@glyph.im>
|
Thanks for the review! 🎉 |
Scope and purpose
Fixes #10276
Adds missing type hints to
twisted.internet._resolver, as part of this it updates the type hints forIHostnameResolver.resolveHostNameto be correct:addressTypesis a sequence ofIAddresstypes, notIAddressinstances.IHostResolution, notIResolutionReceiver.Ports matrix-org/synapse#11328.
Contributor Checklist:
This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.
Below is a non-exaustive list (as a reminder):
#character for GitHub auto-links).please reviewwords.Our bot will trigger the review process, by applying the pending review label
and requesting a review from the Twisted dev team.