Skip to content

push block label sym owner for const and static #24085

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

push block label sym owner for const and static #24085

wants to merge 4 commits into from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 8, 2024 •
edited
Loading

fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803, refs nim-lang/RFCs#276

Based off of #24084 for now but maybe doesn't need it

Another option is to reuse the instantiatedFrom field

@metagn
Copy link
Collaborator Author

metagn commented Sep 8, 2024 •
edited
Loading

As expected the compiler doesn't like that the owner of a symbol needs skips to get to the owning proc, we could add the skips in the required places but that might hurt performance and would take a lot of effort.

For skVar and skLet we have the advantage that they will never use the s.instantiatedFrom symbol, so we can track the "owner" in another field in PContext and give that to instantiatedFrom. Unfortunately skTemp symbols also use sfFromGeneric for some reason (for at least 15 years) so I'm not sure if that would trip the compiler up in some places (seems to just be used to skip semming though). Maybe it already trips the compiler up for skVar and skLet too.

I'll test using instantiatedFrom for now, if it's not considered too hacky I'll polish it.

@metagn
Copy link
Collaborator Author

metagn commented Feb 7, 2025 •
edited
Loading

Another option might be to track the introduced variables in vmgen and only allow them to be used

Edit: Done in #24674

metagn added a commit to metagn/Nim that referenced this pull request Feb 8, 2025
@metagn
track introduced locals in vmgen for eval check
28e2411 
fixes nim-lang#8758, fixes nim-lang#10828, fixes nim-lang#12172, fixes nim-lang#21610, fixes nim-lang#23803, succeeds nim-lang#24085
@metagn metagn mentioned this pull request Feb 8, 2025
Merged
@metagn metagn closed this Feb 8, 2025
Araq pushed a commit that referenced this pull request Feb 14, 2025
@metagn
track introduced locals in vmgen for eval check (#24674)
a5cc33c 
fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803,
fixes #24633, fixes #24634, succeeds #24085

We simply track the symbol ID of every traversed `var`/`let` definition
in `vmgen`, then these symbols are always considered evaluable in the
current `vmgen` context. The set of symbols is reset before every
generation, but both tests worked properly without doing this including
the nested `const`, so maybe it's already done in some way I'm not
seeing.
narimiran pushed a commit that referenced this pull request Feb 17, 2025
@metagn @narimiran
track introduced locals in vmgen for eval check (#24674)
57f84c5 
fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803,
fixes #24633, fixes #24634, succeeds #24085

We simply track the symbol ID of every traversed `var`/`let` definition
in `vmgen`, then these symbols are always considered evaluable in the
current `vmgen` context. The set of symbols is reset before every
generation, but both tests worked properly without doing this including
the nested `const`, so maybe it's already done in some way I'm not
seeing.

(cherry picked from commit a5cc33c)
narimiran pushed a commit that referenced this pull request Mar 3, 2025
@metagn @narimiran
track introduced locals in vmgen for eval check (#24674)
bcecce8 
fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803,
fixes #24633, fixes #24634, succeeds #24085

We simply track the symbol ID of every traversed `var`/`let` definition
in `vmgen`, then these symbols are always considered evaluable in the
current `vmgen` context. The set of symbols is reset before every
generation, but both tests worked properly without doing this including
the nested `const`, so maybe it's already done in some way I'm not
seeing.

(cherry picked from commit a5cc33c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@metagn