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

fix: support local refs natively in future annotation mode #3701

Closed
wants to merge 2 commits into from

Conversation

henryiii
Copy link

Change Summary

This is trying out the suggestion in #2678. When the class is instantiated, type annotations are resolved using the enclosing scopes, enabling locally defined types to be used.

Performance should be checked. This should only affect class creation, though - not usage. This could be optimized, perhaps, by trying to resolve first, maybe with the inmost locals, and then trying again with the full locals construction only if some annotations remain unresolved.

Related issue number

Improves the situation in #2678 by supporting local type annotation, or annotations in an enclosing scope, but not global.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
pydantic/main.py Outdated
Comment on lines 293 to 304

localns = {}

# Pull in the currently defined local namespace and any namespaces above
if hasattr(sys, "_getframe"):
frame = sys._getframe(1)
while frame is not None:
for k, v in frame.f_locals.items():
if k not in localns:
localns[k] = v
frame = frame.f_back

Copy link
Author

Choose a reason for hiding this comment

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

This is the "all enclosing scopes version" - if you are only worried about adding local definitions, at least as first, this simpler version would suffice:

Suggested change
localns = {}
# Pull in the currently defined local namespace and any namespaces above
if hasattr(sys, "_getframe"):
frame = sys._getframe(1)
while frame is not None:
for k, v in frame.f_locals.items():
if k not in localns:
localns[k] = v
frame = frame.f_back
localns = sys._getframe(1).f_locals if hasattr(sys, "_getframe") else {}

Also, this doesn't handle classes defined directly within classes, which might have been important too? That could be added as well by including the class namespace (new_namespace above).

Copy link
Contributor

Choose a reason for hiding this comment

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

frame.f_back goes to the calling function. This isn't 'enclosing' scopes, it's all scopes being called on the stack, which is very likely to cause problems as it may use completely unrelated names.

I gave a suggestion for one way to choose which frame to get locals from here: #2678 (comment)

Also, this doesn't handle classes defined directly within classes

I'm not 100% sure what you have in mind, but inner classes don't have direct access to the enclosing class scope:

class A:
    x = 1
    class B:
        print(x)  # NameError

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right, going "up" the stack would be too much. Getting the immediate locals, though, would help quite a bit with most common issues.

I was assuming the issue with typing in classes looked more like this:

class A:
    class B: pass

    x: B

And you want to resolve B from A.

@henryiii henryiii force-pushed the henryiii/fix/localdef branch 2 times, most recently from 41b3a1b to 8179654 Compare February 4, 2022 21:57
@samuelcolvin
Copy link
Member

thanks so much, Interesting idea.

I'll refer back to this when we have a decision on what's happening with python.

@samuelcolvin samuelcolvin added the deferred deferred until future release or until something else gets done label Apr 2, 2022
@samuelcolvin
Copy link
Member

this is fixed on main where we look at the parent frame for annotations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred deferred until future release or until something else gets done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants