🐛 Fix Mypy plugin crash on variadic arguments #525
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These changes add comments to the `get_reducers_type` function annotating the purpose and meaning of various sections of the function. This will help future newcomers understand the logic used to compute more specific types for model-combining functions.
These changes add a regression test checking whether or not type-checking succeeds when applied to a file with variadic arguments passed to a Thinc API function.
These changes fix a crash that would occur in the Thinc Mypy plugin whenever a Thinc API function was called with variadic arguments. Previously the Thinc Mypy plugin would treat any argument with a generic type as a `Model` instance and attempt to infer a more specific type for the return type of the function call. However, the Thinc Mypy plugin was intended to only be used on functions that exclusively accept and return `Model` instances. Since the plugin accepted overly broad input, it would sometimes end up trying to access the second generic type argument for a generic type instance with only a single argument. This resulted in the plugin crashing. This specifically manifested itself whenever variadic arguments were passed to a Thinc API function. Mypy passes the type of variadic arguments as `List[Model[InT, OutT]]`. The Thinc Mypy plugin would look at this type and attempt to treat it as an unwrapped `Model` type since the plugin would allow any generic type as an argument type annotation. Later on the plugin would attempt to retrieve the output type of the "Model". Since `List` accepts only a single generic argument, attempting to access `type.args[1]` would cause the crash. These changes fix this by restricting the input on which the Thinc Mypy plugin operates. Specifically each argument and the return type must: * Be a generic type `Instance`, * Refer to the `thinc.model.Model` type and * Have two generic type arguments Notably these restrictions prevent the Thinc Mypy plugin from working on subclasses of `thinc.model.Model`. While this plugin previously had some support for `Model` subclasses, this behavior was not documented and unintentional. `Model` subclasses were only properly handled if their first two generic type arguments had the same meaning as `Model`'s generic type arguments. Support for `Model` subclasses may be restored in the future through the use of Mypy's `map_instance_to_supertype` function. I just haven't figured out how to use it yet 😅 Fixes explosion#519
|
Thanks for the PR! I'll go ahead and try to fix the python version compat issues from our end. |
svlandeg
added
feat / types
svlandeg
approved these changes
Aug 30, 2021
There was a problem hiding this comment.
Hi @connorbrinton,
This all looks pretty good, I particularly appreciate the detailed explanations, code comments and additional unit test :-) I made a few small nitpicking suggestions for some of the comments, as it makes sense to avoid statements with "I/my" in the code base, as you don't know later on who wrote what (and it shouldn't really matter either).
Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
|
@svlandeg Thanks for those suggestions, they've been committed 🙂 |
|
We'll get this released as part of 8.0.9 so we can continue with explosion/spaCy#8773! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes fix a crash that would occur in the Thinc Mypy plugin whenever a Thinc API function was called with variadic arguments.
Previously the Thinc Mypy plugin would treat any argument with a generic type as a
Modelinstance and attempt to infer a more specific type for the return type of the function call. However, the Thinc Mypy plugin was intended to only be used on functions that exclusively accept and returnModelinstances. Since the plugin accepted overly broad input, it would sometimes end up trying to access the second generic type argument for a generic type instance with only a single argument. This resulted in the plugin crashing.This specifically manifested itself whenever variadic arguments were passed to a Thinc API function. Mypy passes the type of variadic arguments as
List[Model[InT, OutT]]. The Thinc Mypy plugin would look at this type and attempt to treat it as an unwrappedModeltype since the plugin would allow any generic type as an argument type annotation. Later on the plugin would attempt to retrieve the output type of the "Model". SinceListaccepts only a single generic argument, attempting to accesstype.args[1]would cause the crash.These changes fix this by restricting the input on which the Thinc Mypy plugin operates. Specifically each argument and the return type must:
Instance,thinc.model.Modeltype andNotably these restrictions prevent the Thinc Mypy plugin from working on subclasses of
thinc.model.Model. While this plugin previously had some support forModelsubclasses, this behavior was not documented and unintentional.Modelsubclasses were only properly handled if their first two generic type arguments had the same meaning asModel's generic type arguments.Support for
Modelsubclasses may be restored in the future through the use of Mypy'smap_instance_to_supertypefunction. I just haven't figured out how to use it yet 😅Fixes #519