Skip to content

Update documentation of SymbolTableNode #4080

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

Merged
merged 3 commits into from
Oct 9, 2017

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Oct 9, 2017

No description provided.

mypy/nodes.py Outdated
@@ -2262,6 +2296,8 @@ class SymbolTableNode:
# for other nodes, optionally the name of the referenced object.
cross_ref = None # type: Optional[str]
# Was this node created by normalіze_type_alias?
#
# TODO: Write a better comment, this is confusing
Copy link
Member

Choose a reason for hiding this comment

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

This one distinguishes typing.List and builtins.list (sorry this comment is my fault)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the hint! I'll update the comment.

are only used for them ('type_override', 'alias_tvars' at least).
"""
# TODO: This is a mess. Refactor!
# TODO: Describe how type aliases work.
Copy link
Member

Choose a reason for hiding this comment

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

Type aliases are quite complicated (due to several corner cases). I tried to make some simplifications recently. It seems to me that we can introduce a dedicated SybmolNode (like Var or TypeInfo) and deprecate type_override and friends and just point .node to this new symbol node. This is major refactoring, but I think it may help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's what I was thinking about. I'd also like to deprecate the special handling of module references and other special node kinds so that we could remove the kind attribute and replace it with a scope attribute, with only 3 possible values, corresponding to LDEF, GDEF and MDEF. Not sure how feasible this would be. It would be quite a big refactoring in any case.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to open an issue to track this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #4082.

The most fundamental attributes are 'kind' and 'node'. The 'node'
attribute defines the AST node that the name refers to.

For many bindings, including those targeting variables, functions
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it will be helpful to note that this is a bit similar to how CPython symbol tables work. There are no .node, only .kind (and there are more than three kinds however). These kinds are used to emit correct lookup opcodes, so that the right .node will be found by the interpreter at runtime (I know this comparison is vague).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm talking about CPython implementation details might not be helpful, as many readers likely aren't familiar with them.

@ilevkivskyi ilevkivskyi merged commit 5d25713 into master Oct 9, 2017
@ilevkivskyi ilevkivskyi deleted the refactor-symtable-comments branch October 9, 2017 15:05
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.

2 participants