InternPool: preserve indices of builtin types when resolved #16815
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mlugg
force-pushed
the
internpool-same-resolved-index
branch
2 times, most recently
from
3834050 to
9262f08
Compare
This type not being resolved was a bug which was being triggered by the next commit.
Some builtin types have a special InternPool index (e.g. `.type_info_type`) so that AstGen can refer to them before semantic analysis. Unfortunately, this previously led to a second index existing to refer to the type once it was resolved, complicating Sema by having the concept of an "unresolved" type index. This change makes Sema modify these InternPool indices in-place to contain the expanded representation when resolved. The analysis of the corresponding decls is caught in `Module.semaDecl`, and a field is set on Sema telling it which index to place struct/union/enum types at. This system could break if `std.builtin` contained complex decls which evaluate multiple struct types, but this will be caught by the assertions in `InternPool.resolveBuiltinType`. The AstGen result types which were disabled in 6917a8c have been re-enabled. Resolves: ziglang#16603
mlugg
force-pushed
the
internpool-same-resolved-index
branch
from
8fd0323 to
083ee8e
Compare
andrewrk
approved these changes
Aug 15, 2023
Member
andrewrk
left a comment
andrewrk
left a comment
There was a problem hiding this comment.
Nice work. I think this came out to a pretty reasonable patch.
As a follow-up it might be nice to explore eliminating those enum tags from SimpleType and instead setting up the enum/struct/union in an unresolved state. This way it does not end up creating a dummy index that ultimately just gets copied over to the real thing. I don't think that exploration needs to block this PR though.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some builtin types have a special InternPool index (e.g.
.type_info_type) so that AstGen can refer to them before semantic analysis. Unfortunately, this previously led to a second index existing to refer to the type once it was resolved, complicating Sema by having the concept of an "unresolved" type index.This change makes Sema modify these InternPool indices in-place to contain the expanded representation when resolved. The analysis of the corresponding decls is caught in
Module.semaDecl, and a field is set on Sema telling it which index to place struct/union/enum types at. This system could break ifstd.builtincontained complex decls which evaluate multiple struct types, but this will be caught by the assertions inInternPool.resolveBuiltinType.The AstGen result types which were disabled in 6917a8c have been re-enabled.
Resolves: #16603