Implement variable resolution and global environment management#63
Implement variable resolution and global environment management#63ryan-gang merged 16 commits intoresolving-bindingfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request makes multiple updates across the codebase. The Makefile test targets have been adjusted and expanded. The interpreter now includes a semantic resolution phase before evaluation, with improved error handling. Changes in the environment and function modules add methods and a locals field to support variable scoping. Additionally, a new resolver module and a semantic error handling file have been introduced, while test cases for resolving and function behavior have been updated across several fixture files and test programs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RunAPI
participant Resolver
participant Interpreter
participant Environment
Client->>RunAPI: Execute parsed statements
RunAPI->>Resolver: Resolve(statements)
Resolver-->>RunAPI: Return locals (or error)
alt Semantic Error Occurs
RunAPI-->>Client: Return exit code 65 with error
else
RunAPI->>Environment: Create/use global environment
RunAPI->>Interpreter: Interpret(statements, env, locals)
Interpreter-->>RunAPI: Execution result
RunAPI-->>Client: Return output
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ryan-gang
force-pushed
the
resolving-golox
branch
2 times, most recently
from
67ff6ff to
1e616c6
Compare
- Introduce GlobalEnv and globals variables for managing global environment - Modify InitializeNativeFunctions to use GlobalEnv - Add ResetGlobalEnv method to restore original global environment reference
- Modify Eval function to accept Locals parameter for resolving variable scope - Add support for resolving variables from local and global environments - Update function signatures to include Locals parameter - Implement environment lookup and assignment based on resolved distance
- Add Resolver struct to perform static name resolution - Implement recursive resolution for different AST node types - Support scope tracking, variable declaration, and local variable resolution - Add error handling for variable redeclaration and self-referencing initializers
- Update UserFunction to store Locals for resolving variable scope - Modify Call method to pass Locals to Eval during function execution - Update Run API to pass resolved locals to Interpret method - Add semantic error handling in Run API
- Create semantic_error.go with error reporting functions - Introduce PrintSemanticError for reporting errors to stderr - Add MakeSemanticError for creating semantic error instances - Include HadSemanticError flag to track semantic error state
Modify the error checking logic to ensure semantic errors are properly handled when resolving statements
…ution - Modify NewUserFunction to accept Locals parameter - Update Run API to create global environment before interpreting - Add debug print statements for semantic error handling - Prepare for more robust local variable resolution
- Add debug print statements in Interpret and Eval methods for troubleshooting - Modify Interpret method to accept pre-created environment instead of creating a new one - Update method to preserve and restore GlobalEnv during interpretation - Include additional logging for error tracking and method flow
- Add FunctionType enum to track function context during resolution - Implement function type tracking in Resolver to support return statement validation - Add checks to prevent return statements in top-level code - Include debug print statements for function resolution process
Add HadSemanticError to the list of error flags being reset, ensuring a clean slate for error tracking across different runs
- Set HadSemanticError flag when semantic errors occur - Add more precise error messages for variable declaration and initialization - Include debug print statements for troubleshooting variable resolution
Add test_functions_w_jlox and test_resolving_w_jlox to the test_all target, ensuring comprehensive test coverage for recent language feature implementations
Add test cases for resolving implementation in different stages of the jlox interpreter, covering in-progress and completed resolving scenarios
ryan-gang
force-pushed
the
resolving-golox
branch
from
1e616c6 to
21cfe73
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
internal/lox/functions.go (1)
40-40: Consider referencing Locals by pointer
TheTODOsuggests using a pointer toLocals. This could avoid accidental copies and simplify scope management, especially if future changes require mutating the local variable map.internal/lox/globals.go (2)
7-10: Beware shared global state
Using a single global environment (GlobalEnv) might introduce concurrency or re-initialization challenges if multiple runs occur in parallel. Consider concurrency-safe usage or separate env instances if needed.
11-11: Parameter removal trade-off
RemovingenvfromInitializeNativeFunctionsreduces complexity but also flexibility for test scenarios that might require custom environments.internal/lox/api/run_api.go (1)
27-27: Single exit code for parse & semantic errors
Using65for both parse and semantic errors unifies error handling but can limit nuanced reporting. If needed, consider distinct exit codes for deeper insight into failures.internal/lox/semantic_error.go (1)
20-21: Consider documentation for global state implications.While the variable name is descriptive, you may want to add a comment about how this global state is managed across multiple error scenarios and whether it gets reset between runs.
internal/lox/environment.go (1)
75-78: Ensure type consistency across function signatures.The
AssignAtmethod usesanyas a parameter type, while other methods in this file useinterface{}. Whileanyis a Go 1.18+ alias forinterface{}, maintaining consistency would improve code readability.-func (e *Environment) AssignAt(distance int, name Token, value any) error { +func (e *Environment) AssignAt(distance int, name Token, value interface{}) error {internal/test_helpers/fixtures/pass_resolving_final (1)
1-1: Consider adding a toggle for debug mode instead of a hard-coded flag.The line
Debug = truesuggests that debug output is permanently enabled. Allowing debug mode to be set via an environment variable or command-line argument makes it easier to switch on or off in different environments without modifying the file.internal/lox/interpreter.go (1)
48-120: Logical extension to pass Locals into Eval.By threading
localsthrough all expression evaluations, you correctly support scoped name resolution. This is essential for the new resolver. Just watch out for any performance overhead in large codebases with deeply nested expressions. Consider short-circuit logic or caching expansions if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Makefile(2 hunks)internal/lox/api/run_api.go(1 hunks)internal/lox/environment.go(2 hunks)internal/lox/functions.go(2 hunks)internal/lox/globals.go(2 hunks)internal/lox/interpreter.go(9 hunks)internal/lox/resolver.go(1 hunks)internal/lox/semantic_error.go(1 hunks)internal/stages_test.go(1 hunks)internal/test_helpers/fixtures/pass_functions(1 hunks)internal/test_helpers/fixtures/pass_functions_final(1 hunks)internal/test_helpers/fixtures/pass_resolving(1 hunks)internal/test_helpers/fixtures/pass_resolving_final(1 hunks)test_programs/f9/2.lox(1 hunks)
🔇 Additional comments (31)
test_programs/f9/2.lox (1)
21-21: Change looks good - using explicit value instead of placeholder.The change from a placeholder
<<RANDOM_INTEGER_1>>to an explicit value of75is appropriate for a test case. Since the threshold is defined as50(line 7), this value is beyond the threshold and will returnfalsewithout deep recursion, making it a good test case.internal/test_helpers/fixtures/pass_functions_final (1)
638-638: Test fixture consistently updated to match the test program.The test fixture has been updated to use
75as the input value forisEven()function, maintaining consistency with the changes made in the test program file.internal/test_helpers/fixtures/pass_functions (1)
638-638: Consistent test value updated in all related files.The input value has been consistently updated to
75across all test fixtures and test programs, ensuring coherent test behavior. The output in line 641 correctly shows "false" as expected, since 75 > threshold (50).Makefile (2)
156-156: Smooth transition to new chapter directory
PointingCODECRAFTERS_REPOSITORY_DIRtochap11_resolvingeffectively aligns with the newly introduced resolving tests. This helps ensure the correct set of fixtures is used for resolution.
168-168: Comprehensive test coverage
Includingtest_resolving_w_jloxintest_allensures that the new resolution tests won't be overlooked.internal/lox/functions.go (2)
44-49: Verify invocations of the new constructor
Ensure all calls toNewUserFunctioncorrectly pass thelocalsparameter. Missing updates could lead to inconsistent scope handling.
62-62: Local variable resolution
CallingEvalwithu.Localsensures statements use the correct local scope. This appropriately aligns with the new resolution logic.internal/lox/globals.go (1)
22-25: Partial environment reset
ResetGlobalEnvreassignsGlobalEnvto the initialglobalsreference but doesn’t reinstantiate it. If user code modifiesglobals, a full re-initialization might be needed for a clean slate.internal/lox/api/run_api.go (2)
18-21: Semantic resolution handling
Introducinglox.Resolvebefore interpretation is a solid approach to ensure variable scope validation. The early return on semantic errors clearly signals resolution issues.
23-24: Isolated environment per run
Creating a new global environment each time avoids state leakage between executions. This is beneficial for consistent testing and prevents cross-run interference.internal/lox/semantic_error.go (2)
8-12: Clean implementation of semantic error reporting.The
PrintSemanticErrorfunction provides a clear way to report semantic errors to stderr while updating the global error flag.
14-18: Well-designed semantic error creation function.The
MakeSemanticErrorfunction creates and returns properly formatted error objects, maintaining consistency with error handling patterns.internal/stages_test.go (1)
78-91: Test cases for resolving functionality well structured.The test cases for both in-progress and completed resolving stages follow the established pattern in the codebase. This consistency makes the test suite easier to maintain and understand.
internal/lox/environment.go (2)
49-52: Effective implementation of hierarchical scope lookup.The
GetAtmethod elegantly uses theAncestormethod to navigate to the appropriate environment before performing the lookup.
54-61: Well-designed environment navigation.The
Ancestormethod efficiently traverses the environment chain using idiomatic Go with a range loop over the distance value.internal/test_helpers/fixtures/pass_resolving (4)
1-96: Comprehensive test coverage for variable resolution scenarios.The test suite for Stage #7 covers important variable resolution scenarios including closure behavior, variable shadowing, and nested function scopes. These tests will help ensure the resolver correctly implements lexical scoping.
155-173: Good error handling tests for variable initialization.Testing the error case where a local variable is referenced in its own initializer is an important semantic check that prevents subtle bugs.
191-195: Appropriate validation of return statement restrictions.Testing that return statements are not allowed in top-level code ensures proper function boundary enforcement, which is essential for predictable language behavior.
257-305: Thorough validation of variable redeclaration rules.The tests comprehensively cover various scenarios of variable redeclaration, including local scope redeclaration, parameter redeclaration, and function parameter duplication.
internal/test_helpers/fixtures/pass_resolving_final (3)
3-20: Good comprehensive test for variable shadowing in nested scopes.The test logs and expected output effectively cover how
outerandinnervariables are accessed within nested functions. This approach ensures that the scoping rules are thoroughly checked. Nice work capturing both the initial and redeclared variable behaviors.
72-98: Excellent illustration of closures by returning a function.This test shows that the function returned by
makeCounter()continues to reference and modify thecountvariable declared in an outer scope. This confirms that closures are working correctly. No concerns here.
254-306: Thorough validation of redeclaration errors.These tests confirm that redeclaring variables within the same scope correctly triggers an error. The fixture also checks for parameter name collisions and re-declarations of the same variable in short succession. This ensures robust error handling.
internal/lox/interpreter.go (4)
21-21: Verify that passing an empty Locals{} won't break external usage.Using
Locals{}by default inBasicInterpretis fine, provided that any references to local variable resolutions outside minimal test expressions handle this scenario gracefully. Consider verifying that no calls rely on a non-emptylocals.
32-45: GlobalEnv override may mask errors in nested calls.Saving the current
GlobalEnv, overriding it, and then restoring it is convenient but can introduce unintentional side effects if future code depends on the original environment mid-interpretation. Ensure that no concurrency or parallel evaluations attempt to read fromGlobalEnvwhile it’s swapped.
166-296: Proper local vs. global variable handling.Lines for variable reads (
Variable) and writes (Assign) correctly checklocals[n]first, then fall back toGlobalEnv. This order ensures locals take precedence, aligning with typical scoping rules. Great job maintaining clarity in these conditionals.
345-345: Confirm that resetting HadSemanticError won't mask subsequent semantic errors.Clearing
HadSemanticErroris appropriate before a fresh interpretation run. Just ensure that any semantic issues discovered afterward are still being captured and reported accurately.internal/lox/resolver.go (5)
1-9: Concise introduction for resolving local scopes.Defining
Localsasmap[Expr]intis straightforward and keeps the resolution overhead minimal. This helps maintain an efficient lookup for scoping during interpretation. No concerns here.
13-19: Return both the Locals map and the resolution error.The
Resolvefunction's return signature is intuitive, ensuring callers receive both the computed scope data and any semantic errors that arise. This design is consistent and improves the clarity of error handling in the interpreter.
44-52: Error handling in variable declarations.Declaring a variable before initialization, then immediately resolving its initializer prevents self-references. The current approach effectively catches “read in own initializer” scenarios. Nicely done.
67-74: Well-structured resolver for function declarations.By declaring and defining the function name, then resolving its body in a new scope, you ensure all function parameters and local variables are accounted for. This is a clean approach that avoids premature access to partially-defined functions.
177-184: Local resolution logic is straightforward and efficient.Iterating scopes from the innermost outward provides the correct distance for environment lookups. This pattern integrates seamlessly with the updated interpreter, ensuring correct variable references in nested scopes.
ryan-gang
force-pushed
the
resolving-golox
branch
from
7b196cb to
21cfe73
Compare
|
This PR implements resolving and binding in our internal golox. |
1 participant
Introduce methods for resolving local and global variables, manage global environment, and enhance semantic error handling. This update improves the interpreter's ability to handle variable scopes and ensures proper error reporting during name resolution.
Summary by CodeRabbit
New Features
Tests