Module#21
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the intermediate representation (IR) structure by introducing a SlangModule abstraction to better organize top-level code and functions.
Key Changes:
- Introduced
SlangModuleto wrap top-level functions and inline functions, with a synthetic__module__main__function containing top-level statements - Updated the interpreter, CFG builder, and test infrastructure to work with the new module-based structure
- Modified test snapshots to show pretty-printed output instead of raw AST structure
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/kotlin/slang/hlir/Ast.kt | Added SlangModule data class and updated ProgramUnit to contain a list of modules instead of statements |
| src/main/kotlin/slang/hlir/AstBuilder.kt | Modified visitCompilationUnit to wrap top-level code in a SlangModule with a synthetic __module__main__ function |
| src/main/kotlin/slang/runtime/Interpreter.kt | Updated interpreter to register module functions and execute the synthetic module main body |
| src/main/kotlin/slang/hlir/ControlFlowGraph.kt | Enhanced CFG builder to extract and build CFG for the synthetic module main function, with improved break/continue handling |
| src/test/kotlin/DataflowAnalysisTest.kt | Updated to access functions through the new module structure using flatMap { it.functions } |
| src/test/kotlin/IRBuilderTests.kt | Changed to use prettyPrint() instead of toString(), but discards error messages in the error case |
| src/main/kotlin/slang/ui/AstVisualizer.kt | Added tree node rendering for SlangModule in the AST visualizer |
| build.gradle.kts | Modified approveSnapshots task to require a test path parameter instead of a commit flag |
| scripts/approve_snapshots.sh | Simplified script to rename files in a specified directory instead of git operations |
| src/test/kotlin/IRBuilderTests.testCase*.approved.txt | Updated snapshot files to show pretty-printed IR instead of raw AST representation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val topLevelInlineFuncs = stmts.filterIsInstance<Stmt.ExprStmt>().map { it.expr } | ||
| .filterIsInstance<Expr.InlinedFunction>() | ||
| val topLevelfuncs = stmts.filterIsInstance<Stmt.Function>() | ||
| val slangModule = SlangModule(listOf(moduleMain) + topLevelfuncs, topLevelInlineFuncs ) |
There was a problem hiding this comment.
[nitpick] Extra space before closing parenthesis. The code has topLevelInlineFuncs ) but should be topLevelInlineFuncs) for consistent formatting.
| val slangModule = SlangModule(listOf(moduleMain) + topLevelfuncs, topLevelInlineFuncs ) | |
| val slangModule = SlangModule(listOf(moduleMain) + topLevelfuncs, topLevelInlineFuncs) |
| val stmt: List<SlangModule>, | ||
| ) : SlastNode() | ||
|
|
||
| data class SlangModule(val functions : List<Stmt.Function>, val inlinedFuncs : List<Expr.InlinedFunction>) : SlastNode() |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing in property declaration. There's an extra space in val functions : List<Stmt.Function> - should be val functions: List<Stmt.Function> (no space before the colon). This is inconsistent with Kotlin style conventions.
| data class SlangModule(val functions : List<Stmt.Function>, val inlinedFuncs : List<Expr.InlinedFunction>) : SlastNode() | |
| data class SlangModule(val functions: List<Stmt.Function>, val inlinedFuncs : List<Expr.InlinedFunction>) : SlastNode() |
| val stmt: List<SlangModule>, | ||
| ) : SlastNode() | ||
|
|
||
| data class SlangModule(val functions : List<Stmt.Function>, val inlinedFuncs : List<Expr.InlinedFunction>) : SlastNode() |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing in property declaration. There are extra spaces in val inlinedFuncs : List<Expr.InlinedFunction> - should be val inlinedFuncs: List<Expr.InlinedFunction> (no spaces before the colon). This is inconsistent with Kotlin style conventions.
| data class SlangModule(val functions : List<Stmt.Function>, val inlinedFuncs : List<Expr.InlinedFunction>) : SlastNode() | |
| data class SlangModule(val functions : List<Stmt.Function>, val inlinedFuncs: List<Expr.InlinedFunction>) : SlastNode() |
| {it.prettyPrint(0)}, | ||
| {""} |
There was a problem hiding this comment.
Error messages are being silently discarded. When the HLIR transformation fails (returns Result.Err), this code returns an empty string "", losing all error information. This causes tests like testCase8 (which expects error output about variable redeclaration) to produce empty output instead of showing the actual errors. Consider returning a formatted error message instead, e.g., {errors -> errors.joinToString("\n") { it.toString() }}.
| {it.prettyPrint(0)}, | |
| {""} | |
| { it.prettyPrint(0) }, | |
| { errors -> errors.joinToString("\n") { it.toString() } } |
|
This pull request refactors the internal representation of programs in the Slang language compiler and runtime. The main change is the introduction of a new
SlangModuletype to encapsulate top-level functions and inlined functions, along with a synthetic__module__main__function that contains all top-level statements. This improves modularity and prepares the codebase for future multi-module support. Several components—AST, CFG builder, interpreter, and UI—are updated to work with this new structure, and tests are adjusted to match the new output format.Core IR and AST changes
SlangModuleclass to group top-level functions and inlined functions, and updatedProgramUnitto hold a list of modules instead of statements. The IR builder now wraps top-level statements into a synthetic__module__main__function inside a module. [1] [2]Control Flow Graph (CFG) and Interpreter updates
Test and snapshot updates
Miscellaneous
Ast.ktand added necessary imports for module support in the UI. [1] [2]