Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-38605: Make postponed evaluation of annotations default #20434

Open
wants to merge 15 commits into
base: master
from

Conversation

@isidentical
Copy link
Member

@isidentical isidentical commented May 26, 2020

[This is a draft for my public work in progress PoC, feel free to create another PR if you can successfully pass the test suite with the least breakage]
Todo:

  • add what's new entry
    `

https://bugs.python.org/issue38605

@isidentical isidentical force-pushed the isidentical:bpo-38605 branch 16 times, most recently from 99bf3de to b9d9f0a May 26, 2020
Copy link
Member

@vstinner vstinner left a comment

Overall, the whole change looks like the right approach. I will wait until the PR is not marked as draft to review it in depth. Thanks for working on this issue!

@isidentical
Copy link
Member Author

@isidentical isidentical commented May 27, 2020

Overall, the whole change looks like the right approach. I will wait until the PR is not marked as draft to review it in depth.

Thanks! As far as I'm aware there are only 2 things that currently hold me from marking this PR as ready to review, one is some silly typing failures (will look them) and the other one is issue 40794.

@isidentical isidentical force-pushed the isidentical:bpo-38605 branch 2 times, most recently from 1efa11e to 51c7556 May 27, 2020
@python python deleted a comment from isidentical May 28, 2020
@isidentical isidentical force-pushed the isidentical:bpo-38605 branch from 3af038a to 3f1abab May 29, 2020
@isidentical isidentical marked this pull request as ready for review May 29, 2020
@vstinner
Copy link
Member

@vstinner vstinner commented Oct 1, 2020

@isidentical: hey, there is now a conflict on your PR. Would you mind to try to solve it?

Copy link
Member

@gvanrossum gvanrossum left a comment

I think this is in part on me -- I started a review but never finished it. IIRC there were also some unresolved problems, but I don't know if they are still unresolved.

Anyway, here are my (very old) review comments that I never sent out.

Doc/reference/compound_stmts.rst Outdated Show resolved Hide resolved
Lib/test/test_functools.py Outdated Show resolved Hide resolved
Lib/test/test_grammar.py Outdated Show resolved Hide resolved
@isidentical
Copy link
Member Author

@isidentical isidentical commented Oct 1, 2020

I think this is in part on me -- I started a review but never finished it. IIRC there were also some unresolved problems, but I don't know if they are still unresolved.

I'm giving a rebase right now, and try to merge the latest typing changes on get_type_hints etc with current version. Please do not review until I push the latest changes (at worst tomorrow)

@isidentical isidentical force-pushed the isidentical:bpo-38605 branch from 2c4e607 to 1e2c3f8 Oct 1, 2020
@isidentical
Copy link
Member Author

@isidentical isidentical commented Oct 2, 2020

Found a different example for double-stringified sources in dataclasses (this used to work, but now it can't get parsed by the static regex in the dataclasses module since now there are extra quotes);

import typing
from dataclasses import dataclass

@dataclass
class T:
    a: "typing.ClassVar[int]"

T()

We can either handle it like how we do in the ForwardRef, or solve these all problems completely in the compiler level by doing something like this;

+++ b/Python/compile.c
@@ -2016,7 +2016,15 @@ error:
 static int
 compiler_visit_annexpr(struct compiler *c, expr_ty annotation)
 {
-    ADDOP_LOAD_CONST_NEW(c, _PyAST_ExprAsUnicode(annotation));
+    PyObject *value;
+    if (annotation->kind == Constant_kind && PyUnicode_CheckExact(annotation->v.Constant.value)) {
+        value = annotation->v.Constant.value;
+        Py_INCREF(value);
+    }
+    else {
+        value = _PyAST_ExprAsUnicode(annotation);
+    }
+    ADDOP_LOAD_CONST_NEW(c, value);
     return 1;
 }

Any suggestions @gvanrossum?

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 2, 2020

I don't think that solves it completely, does it? ISTM we still get double quotes when the annotation contains a string literal as a generic parameter (e.g., list["int"]). And it doesn't produce the same result with the future import in 3.9. It also seems quite inconsistent to say "annotations get stringified except if they are already strings" -- this makes it impossible to reconstruct the original annotation by un-stringifying.

I'd rather fix this in dataclasses.py and backport the fix to 3.9.1 (since it's clearly too late for 3.9.0 :-).

@isidentical isidentical requested a review from gvanrossum Oct 3, 2020
Copy link
Member

@gvanrossum gvanrossum left a comment

I worry that people do introspection on dataclasses and will be confused by finding strings instead of type objects. But I don't know what else to do. I somehow think not too many people outside Facebook have been using from __future__ import annotations... :-(

I also worry about the change in behavior in the inspect module. But I think you're over the hump and we can soon land this...

Doc/library/inspect.rst Outdated Show resolved Hide resolved
Doc/reference/compound_stmts.rst Outdated Show resolved Hide resolved
Lib/dataclasses.py Show resolved Hide resolved
Lib/test/test_dataclasses.py Outdated Show resolved Hide resolved
Lib/dataclasses.py Outdated Show resolved Hide resolved
Lib/test/test_functools.py Show resolved Hide resolved
Lib/test/test_inspect.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
@isidentical
Copy link
Member Author

@isidentical isidentical commented Oct 4, 2020

As a side effect, this now folds into Optional[int]

@dataclass
class C:
    x: Union[int, type(None)] = None
- C.__doc__ == C(x:'Union[int, type(None)]'=None)
+ C.__doc__ == C(x:Optional[int]=None)
isidentical added 2 commits Oct 4, 2020
Lib/dataclasses.py Show resolved Hide resolved
Lib/dataclasses.py Show resolved Hide resolved
except Exception:
# First, try to use the get_type_hints to resolve
# annotations. But for keeping the behavior intact
# if there was a problem with that (like the namespace
# can't resolve some annotation) continue to use
# string annotations
return func.__annotations__
Comment on lines +2128 to +2134

This comment has been minimized.

@gvanrossum

gvanrossum Oct 4, 2020
Member

I worry that that exception is going to mask errors (and e.g. cause us to update tests from a: int to a: 'int').

I wonder if it would be better to just return {} in this case (given that we don't want to bubble up the error)?

Also, catching Exception feels wrong -- but I do agree that there are lots of different errors you could get (I managed to trigger TypeError, AttributeError, NameError, SyntaxError, IndentationError, IndentationError -- so who knows what else it could raise. (Just mumbling to myself here. :-)

This comment has been minimized.

@isidentical

isidentical Oct 4, 2020
Author Member

Actually, there is not much we can do for the exceptions since there is a code path that is going to eval.

(.venv) (Python 3.10.0a0) [ 12:31ÖÖ ]  [ isidentical@desktop:~/cpython/cpython(bpo-38605✗) ]
 $ python t.py  
(a: 'b()')
(.venv) (Python 3.10.0a0) [ 12:31ÖÖ ]  [ isidentical@desktop:~/cpython/cpython(bpo-38605✗) ]
 $ ./python t.py
Traceback (most recent call last):
  File "/home/isidentical/cpython/cpython/t.py", line 4, in <module>
    print(inspect.signature(x))
  File "/home/isidentical/cpython/cpython/Lib/inspect.py", line 3133, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped)
  File "/home/isidentical/cpython/cpython/Lib/inspect.py", line 2882, in from_callable
    return _signature_from_callable(obj, sigcls=cls,
  File "/home/isidentical/cpython/cpython/Lib/inspect.py", line 2333, in _signature_from_callable
    return _signature_from_function(sigcls, obj,
  File "/home/isidentical/cpython/cpython/Lib/inspect.py", line 2178, in _signature_from_function
    annotations = _get_type_hints(func)
  File "/home/isidentical/cpython/cpython/Lib/inspect.py", line 2127, in _get_type_hints
    return typing.get_type_hints(func)
  File "/home/isidentical/cpython/cpython/Lib/typing.py", line 1420, in get_type_hints
    value = _eval_type(value, globalns, localns)
  File "/home/isidentical/cpython/cpython/Lib/typing.py", line 255, in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
  File "/home/isidentical/cpython/cpython/Lib/typing.py", line 501, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
  File "/home/isidentical/cpython/cpython/t.py", line 1, in b
    def b(): 1/0 
ZeroDivisionError: division by zero
(.venv) (Python 3.10.0a0) [ 12:31ÖÖ ]  [ isidentical@desktop:~/cpython/cpython(bpo-38605✗) ]
 $ cat t.py
def b(): 1/0 
def x(a: 'b()'): pass
import inspect
print(inspect.signature(x))

This comment has been minimized.

@gvanrossum

gvanrossum Oct 4, 2020
Member

But I still think maybe we should return {} if it raises.

This comment has been minimized.

@isidentical

isidentical Oct 5, 2020
Author Member

This is a bit problematic when we are in a different context than the module, like this;

import inspect, typing

def foo():
    class F: ...
    def foo(bar: F): ...
    print(inspect.signature(foo))

foo()

This would be normally resolved when we call get_type_hints with locals() but we can't do it (at least not without changing the API / or doing some frame-level dark magic). (that is from a test case of ours).

This comment has been minimized.

@gvanrossum

gvanrossum Oct 5, 2020
Member

(AFK) Does inspect.signature() have a way to pass a namespace?

This comment has been minimized.

@isidentical

isidentical Oct 5, 2020
Author Member

Does inspect.signature() have a way to pass a namespace?

Apparently not

Lib/test/test_functools.py Show resolved Hide resolved
@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 4, 2020

As a side effect, this now folds into Optional[int]

@dataclass
class C:
    x: Union[int, type(None)] = None
- C.__doc__ == C(x:'Union[int, type(None)]'=None)
+ C.__doc__ == C(x:Optional[int]=None)

But note that type(None) is invalid according to PEP 484 -- it should just be None.

@isidentical
Copy link
Member Author

@isidentical isidentical commented Oct 4, 2020

But note that type(None) is invalid according to PEP 484 -- it should just be None.

I just copied over the test case, my point was we are changing the shape of a declaration;

>>> from typing import *
>>> def x(a: Union[int, None]): pass
... 
>>> inspect.signature(x)
<Signature (a: Optional[int])>

But it is not much of a big deal, I guess, just wanted to mention about it

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 4, 2020

But note that type(None) is invalid according to PEP 484 -- it should just be None.

I just copied over the test case, my point was we are changing the shape of a declaration;

>>> from typing import *
>>> def x(a: Union[int, None]): pass
... 
>>> inspect.signature(x)
<Signature (a: Optional[int])>

But it is not much of a big deal, I guess, just wanted to mention about it

Hm, your comment was not linked to any particular line of code.

FWIW that behavior is built into typing.Union:

>>> import typing
>>> typing.Union[int, None]
typing.Optional[int]
>>> 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.