-
Notifications
You must be signed in to change notification settings - Fork 661
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
Refactored SymbolTable and related changes. #28488
Conversation
8810f56
to
aeabc3a
Compare
@@ -1008,6 +1009,6 @@ impl<'a, N: Network> ExpressionVisitor<'a> for TypeChecker<'a, N> { | |||
} | |||
|
|||
fn visit_unit(&mut self, _input: &'a UnitExpression, _additional: &Self::AdditionalInput) -> Self::Output { | |||
unreachable!("Unit expressions should not exist in normal code"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe unit expression should not exist at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They actually do exist (not explicitly specified by the user) in return
statements. At least for now I'll leave this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the design of the new symbol table and it greatly simplifies scope management, while addressing a number of issues. I left a few comments and nits, otherwise LGTM!
1d47458
to
6673a94
Compare
The SymbolTable design is now a little simpler, and local scopes don't have tables to store functions, structs, etc, since those are only globals anyway. Remove the symbol table json code. Revise code catching assignments in conditionals in async functions. Previously, code like this: if x { let y: u8 = 1u8; { y = 2u8; } } would trigger the error, but it should actually pass. Can use locators for local records and mappings Remove the N type parameter in TypeChecker. Instead, we have a `NetworkLimits` struct which contains the values the TypeChecker will actually use. This cuts down on code bloat a bit.
6673a94
to
dcba01c
Compare
The SymbolTable design is now a little simpler, and local scopes don't have tables to store functions, structs, etc, since those are only globals anyway.
With this PR we:
would trigger the error, but it should actually pass.
NetworkLimits
struct which contains the values the TypeChecker will actually use. This cuts down on code bloat a bit.