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

wgsl-in: Implement lexical scopes #2024

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

JCapucho
Copy link
Collaborator

@JCapucho JCapucho commented Aug 6, 2022

Previously the wgsl frontend wasn't aware of lexical scopes causing all variables and named expressions to share a single function scope, this meant that if a variable was defined in a block with the same name as a variable in the function body, the variable in the function body would be lost and exiting the block all references to the variable in the function body would be replaced with the variable of the block.

This PR fixes that in two commits, the first commit introduces a new frontend agnostic type SymbolTable that will be responsible for managing lexical scopes and performing variable lookups, this type is only used in wgsl right now but I intend to change glsl's internal symbol table for it, the second commit wires it up to the wgsl frontend and inserts the proper push/pop of lexical scopes according to the wgsl spec.

A snapshot test is also included to ensure all lexical scopes are working.

Fixes #2021

@jimblandy
Copy link
Member

jimblandy commented Sep 2, 2022

This isn't really a problem with this PR, but one concern I have is that the WGSL Parser type already has functions named push_scope and pop_scope. Those do something entirely different from SymbolTable's methods - they're purely for error checking - but it's confusing to have both of these flying around.

But I think this PR's use of the term 'scope' is more apt. Once it's merged, we should rename naga::front::wgsl::Scope to Rule (since it indicates which rule of the grammar we are currently parsing) with corresponding changes to the related functions and variables.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This looks great - thank you!

I've put a bunch of copy-edits here: https://github.com/jimblandy/naga/tree/copy-edit
If they look good, then smoosh them into your commit before landing.
EDIT: I wanted to merge immediately, so I took care of this.

Adds a new type `SymbolTable` to allow sharing code between the
frontends related to variable name lookup and lexical scope management,
this way improvements in variable lookup can be shared among all
frontends.
Previously the wgsl frontend wasn't aware of lexical scopes causing all
variables and named expressions to share a single function scope, this
meant that if a variable was defined in a block with the same name as a
variable in the function body, the variable in the function body would
be lost and exiting the block all references to the variable in the
function body would be replaced with the variable of the block.

This commit fixes that by using the previously introduced `SymbolTable`
to track the lexical and perform the variable lookups, scopes are pushed
and popped as defined in the wgsl specification.
@jimblandy jimblandy merged commit d64d78f into gfx-rs:master Sep 2, 2022
@JCapucho JCapucho deleted the wgsl-variable-scopes branch September 3, 2022 15:29
@JCapucho
Copy link
Collaborator Author

JCapucho commented Sep 3, 2022

This looks great - thank you!

I've put a bunch of copy-edits here: https://github.com/jimblandy/naga/tree/copy-edit If they look good, then smoosh them into your commit before landing. EDIT: I wanted to merge immediately, so I took care of this.

@jimblandy if it's just some typos you don't even need to ask, thanks for fixing them.

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.

[wgsl-in] let declarations from different scopes merged?
2 participants