Improve type detection for annotations #3556
Conversation
| """Get non-vararg parameters of `func` as an ordered dict.""" | ||
| var_param_kinds = (inspect.Parameter.VAR_POSITIONAL, inspect.Parameter.VAR_KEYWORD) | ||
| try: | ||
| params = list(get_signature(func).parameters.values()) | ||
| params = list(get_signature(func, eval_str=eval_str).parameters.values()) | ||
| except Exception: |
There was a problem hiding this comment.
| except Exception: | |
| except Exception: | |
| if eval_str: | |
| return _get_params(func) |
Evaluating string annotations can fail due to syntax errors, name resolution, or whatever arbitrary code execution we invoked failing; so in those cases I'd rather fall back to unevaluated annotations (above) or switching to annotate=False for that argument. I trust your judgement as to which is preferable.
In either case, a test demonstrating that would be nice - say f(a: "invalid ::: syntax", b: "1/0").
|
| else: | ||
| sig = inspect.signature(target, follow_wrapped=follow_wrapped) |
There was a problem hiding this comment.
Nice solution 🙂
Just needs a # pragma: no cover comment on one of these lines - we configure coverage to ignore branching on sys.version, but that doesn't cover the else:.
| try: | ||
| params = _get_params(func, eval_str=True) | ||
| except Exception: | ||
| # don't add parameters if the annotations could not be evaluated | ||
| pass |
There was a problem hiding this comment.
I think we need to handle this within the _get_params function, because an error due to eval_str=True will instead trigger the docstring-based fallback, and potentially lead to a wrong signature being returned!
We can get the signature as described above, and then discard any annotations that are strings at this stage.
There was a problem hiding this comment.
I've now had a closer look at the _get_params function. As it turns out, the fallbacks will be ignored anyway, because they don't set the annotation property of the inspect.Parameter. I've refactored the code to call the get_signature directly.
…ks dont have any annotations
|
Fantastic work - thanks again! |
|
@ThunderKey, I've sent you an invitation to the Hypothesis team - there's no obligation attached, but we'd love it if you wanted to stick around, write PRs or review code as the mood takes you, or just generally remain associated with the project. The contributor |
|
Awesome, thank you @Zac-HD for the invitation! I'll have a look at the guides :) |
…detection-for-annotations Improve type detection for annotations
When running the ghostwriter with type annotations (#3548) on bigger projects I've detected the following issues:
Optionalwith a single type argument got translated toOptional[int, None], which does not work.List[int | str]) got translated toList[types.UnionType[int, str]].types.UnionTypedoes not support generics and this does therefore not work.from __future__ import annotationsmarker is used, the annotations are strings and not types. I've added a patch that evaluates those annotations in Python 3.10 and newer. We could add a patch for earlier versions by recreating theeval_strargument, but I'm not sure this edge case of an optional feature is worth the added complexity: https://github.com/python/cpython/blob/498598e8c2d64232d26c075de87c513415176bbf/Lib/inspect.py#L260:#L280Sorry that I did not detect those issues earlier. Now I've tested it on multiple projects and it seems more stable.