Skip to content

Suppress NameError for undefined names in annotations#3

Closed
JelleZijlstra wants to merge 8 commits into
larryhastings:co_annotationsfrom
JelleZijlstra:annotname
Closed

Suppress NameError for undefined names in annotations#3
JelleZijlstra wants to merge 8 commits into
larryhastings:co_annotationsfrom
JelleZijlstra:annotname

Conversation

@JelleZijlstra

Copy link
Copy Markdown
Contributor

A possible proof-of-concept fix for #1. I'm sure there's some subtlety I missed.

Comment thread Lib/test/test_types.py
def test_annotation_name(self):
an = types.AnnotationName("x")
self.assertEqual(repr(an), "<unresolved name: x>")
# TODO for some reason __name__ doesn't exist if I don't do this first

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really weird to me: somehow .__name__ only exists on AnnotationName objects after I've done something like AnnotationName.__dict__. My implementation mostly followed that of GenericAlias, which doesn't have this problem. I'll stare at it some more, but maybe someone who knows more about CPython internals will know the reason.

Comment thread Lib/types.py

AnnotationName = type(f.__annotations__["x"])
del f

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be moved up to use the function _f() on line 12. Then both types could be extracted from one function instead of two.

@larryhastings

Copy link
Copy Markdown
Owner

I appreciate you writing this PR, but unfortunately I don't like this. My goal in designing PEP 649 was to take stock semantics and time-shift the evaluation of the annotations, adding as little opinion as possible. I think catching NameErrors and substituting another object is too opinionated for a language-level feature.

If I'm reading it correctly, this PR would suppress NameError by replacing the failed value with a new AnnotationName object. It wouldn't handle any other exceptions (e.g. ValueError).

Also, as currently implemented it doesn't permit getting attributes or indexing into the AnnotationName--if o is the object we failed to look up, o.value and o[1] would raise exceptions. AnnotationName could be extended with __getattr__ and __getitem__, but this makes it even more opinionated.

Also, the PR only adds LOAD_ANNOTATION_GLOBAL; it would presumably also need LOAD_ANNOTATION_NAME and LOAD_ANNOTATION_CLASSDEREF. (I don't think we'd need LOAD_ANNOTATION_LOCAL.)

I've mentioned this idea in PEP 649, so I assume the Steering Committee is aware of it. If they accept PEP 649 but stipulate that it must implement this or similar functionality, naturally I will revisit your PR then.

Again, thank you for proposing this; it's certainly not my goal to belittle your proposal. I appreciate your efforts to improve PEP 649.

@JelleZijlstra

Copy link
Copy Markdown
Contributor Author

Thanks, and no worries! It's certainly a more ambitious change to semantics than PEP 649 as it stands. The problems you bring up (missing opcodes, missing subscript functionality) can be fixed, but as you say they would make the change even more involved. Let's close this for now and revisit it if the SC turns out to be interested in this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants