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

[red-knot] remove layer of indirection from SymbolTable #11132

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Apr 24, 2024

Summary

Now that symbol definitions don't hold direct references to AST nodes, and thus don't have a lifetime bound, there's no longer any reason to separate the "core SymbolTable" from the definitions. All of the symbol table, including definitions, can be cached together, and it all needs to be invalidated together if the module AST changes. So let's simplify this and have fewer structs.

Test Plan

Existing tests.

@carljm carljm requested a review from MichaReiser April 24, 2024 15:59
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Makes sense

@carljm carljm merged commit 5ac0c3c into red-knot Apr 24, 2024
14 checks passed
@carljm carljm deleted the cjm/red-knot/symbols-types branch April 24, 2024 16:02
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser pushed a commit that referenced this pull request Apr 25, 2024
## Summary

Now that symbol definitions don't hold direct references to AST nodes,
and thus don't have a lifetime bound, there's no longer any reason to
separate the "core SymbolTable" from the definitions. All of the symbol
table, including definitions, can be cached together, and it all needs
to be invalidated together if the module AST changes. So let's simplify
this and have fewer structs.

## Test Plan

Existing tests.
@carljm carljm added the internal An internal refactor or improvement label Apr 26, 2024
MichaReiser pushed a commit that referenced this pull request Apr 27, 2024
## Summary

Now that symbol definitions don't hold direct references to AST nodes,
and thus don't have a lifetime bound, there's no longer any reason to
separate the "core SymbolTable" from the definitions. All of the symbol
table, including definitions, can be cached together, and it all needs
to be invalidated together if the module AST changes. So let's simplify
this and have fewer structs.

## Test Plan

Existing tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants