Conversation
Signed-off-by: WolfGangS <flamin2k8@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds switch statement preprocessing support for LSL scripts, converting switch(expr) { case v: { ... } default: { ... } } syntax into equivalent if/jump/label patterns compatible with LSL's execution model (matching Firestorm's preprocessor behavior). It also includes a significant refactor passing LanguageLexerConfig objects top-down instead of ScriptLanguage strings throughout the preprocessor pipeline.
Changes:
- New
switchdirective handler inparser.tsthat parses case blocks and emits LSLif/jump/@labelequivalents, plus a deterministicRNGclass for unique label generation. - API refactor across
Parser,ConditionalProcessor,IncludeProcessorto acceptLanguageLexerConfiginstead ofScriptLanguagestring;MacroProcessorconstructor drops its language parameter;getLanguageConfig()now returns astructuredClone. - New configuration key
PreprocessorLSLSwitchStatements,package.jsonsetting, test workspace files, and comprehensive test updates across all test files.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/shared/parser.ts |
Core switch directive implementation, RNG class, parseToken refactor, new helper methods |
src/shared/lexer.ts |
Added name field to configs, split LanguageLexerConfig/CustomLanguageLexerConfig, getLanguageConfig uses structuredClone, removed overloaded constructor |
src/shared/lexingpreprocessor.ts |
Updated process() signature to accept LanguageLexerConfig |
src/shared/diagnostics.ts |
New DiagnosticError class for throw-based error propagation |
src/shared/macroprocessor.ts |
Removed language parameter from constructor |
src/shared/conditionalprocessor.ts |
Accepts LanguageLexerConfig instead of ScriptLanguage |
src/shared/includeprocessor.ts |
Accepts LanguageLexerConfig instead of ScriptLanguage |
src/scriptsync.ts |
New getLanuageConfig() helper that conditionally adds switch to directive keywords |
src/synchservice.ts |
Use getLanguageConfig() instead of direct LANGUAGE_CONFIGS access |
src/interfaces/configinterface.ts |
New PreprocessorLSLSwitchStatements config key |
package.json |
New VSCode setting for switch statement support |
src/test/suite/*.test.ts |
Updated all tests to use getLanguageConfig() for typed configs |
src/test/workspace/set_1/test_switch*.lsl |
Test input and expected output for switch preprocessing |
doc/preprocessor-guide.md |
Documentation for the new switch statement feature |
You can also share your feedback on Copilot code review. Take the survey.
| break; | ||
| } | ||
| } | ||
| parser.parseToken(current); |
There was a problem hiding this comment.
parser.parseToken(current) is called without await at line 987, inside consumeCaseBlock. Since parseToken is async and may internally call await this.handleDirective(token), the Promise returned by parseToken is silently discarded. This means any async directives (e.g., #include) inside a case body will not be awaited, potentially leading to incorrect ordering, missing output, or unhandled errors. This call must be awaited: await parser.parseToken(current);.
| class RNG { | ||
| private state: number; | ||
| constructor(seed: number = 9863369152) { | ||
| // Ensure seed is treated as a 32-bit signed integer | ||
| this.state = seed | 0; | ||
| } | ||
| public next(): number { | ||
| // Xorshift* algorithm (32-bit integer arithmetic) | ||
| let x = this.state; | ||
| x ^= x << 13; | ||
| x ^= x >> 17; | ||
| x ^= x << 5; | ||
| // Clamp back to 32-bit signed integer | ||
| x |= 0; | ||
| this.state = x; | ||
| return x & 0x7FFFFFFF; | ||
| } | ||
|
|
||
| public nextHex(): string { | ||
| return this.next().toString(16).padStart(8, '0'); | ||
| } | ||
| } |
There was a problem hiding this comment.
The RNG class is declared outside the Parser class (after the closing } on line 2167) but before the //#endregion marker. While this works as a module-private class in TypeScript, the RNG class is logically an implementation detail of ParserState/Parser and is declared in an awkward location between the Parser class body and its #endregion comment. It should either be moved inside the module scope before the Parser class or be a proper private inner implementation. Additionally, the checkTokenOfType method and peek method added in this PR are never called anywhere in the codebase — they are dead code that should be removed or used.
| console.error("======RESULT CONTENT======"); | ||
| console.error(result.content); | ||
| console.error("======END RESULT CONTENT======"); | ||
|
|
There was a problem hiding this comment.
There are two leftover debug console.error statements at lines 1678-1680 of the test file that print ======RESULT CONTENT====== and the full result content on every test run. These should be removed before merging.
| console.error("======RESULT CONTENT======"); | |
| console.error(result.content); | |
| console.error("======END RESULT CONTENT======"); | |
| import { getLanguageConfig, Lexer } from '../../shared/lexer'; | ||
| import { DiagnosticCollector, DiagnosticSeverity, ErrorCodes } from '../../shared/diagnostics'; | ||
| import { normalizePath, NormalizedPath } from '../../interfaces/hostinterface'; | ||
| import { get } from 'http'; |
There was a problem hiding this comment.
There is an unused import import { get } from 'http'; on line 11. This appears to be an accidental leftover that should be removed.
| import { get } from 'http'; |
| }); | ||
| }); | ||
|
|
||
| test('should allow switch as identifier if switch statments not enabled for lsl preproc', async () => { |
There was a problem hiding this comment.
Spelling error: "statments" should be "statements" in the test name 'should allow switch as identifier if switch statments not enabled for lsl preproc'. This same misspelling also appears in the duplicate test name at line 1642.
| /** Require state (for SLua require() directives) - only present when require is supported */ | ||
| requireState?: RequireState; | ||
| /** Set of unique identifiers to use for naming things like switch/loop jumps */ | ||
| uniqueidentifiers: Set<string>; |
There was a problem hiding this comment.
The uniqueidentifiers field in ParserState uses a lowercase 'i' with no separator, while the rest of the codebase uses camelCase for multi-word identifiers (e.g., requireState, includeState). The field name should be uniqueIdentifiers (camelCase) to match the established naming convention.
| uniqueidentifiers: Set<string>; | |
| uniqueIdentifiers: Set<string>; |
| return finalContent; | ||
| } | ||
|
|
||
| private getLanuageConfig(): LanguageLexerConfig { |
There was a problem hiding this comment.
The method getLanuageConfig is a typo — it should be getLanguageConfig (missing 'g' in 'Language'). This typo appears in both the method name definition and all three call sites in scriptsync.ts.
| private getLanuageConfig(): LanguageLexerConfig { | |
| private getLanguageConfig(): LanguageLexerConfig { |
|
|
||
| this.macros.clearNonSystemMacros(); | ||
| const languageConfig = this.getLanuageConfig(); | ||
| console.error("LANG CONFIG", languageConfig); |
There was a problem hiding this comment.
There is a leftover debug console.error("LANG CONFIG", languageConfig) statement at line 402 that logs the full language config object to stderr on every preprocessing run. This should be removed before merging.
| console.error("LANG CONFIG", languageConfig); |
| assert.strictEqual(result.issues.length, 1, 'Should have 1 issue'); | ||
| }); | ||
|
|
||
| test('should allow switch as identifier if switch statments not enabled for lsl preproc', async () => { |
There was a problem hiding this comment.
The test at line 1642 has the wrong name: it is titled 'should allow switch as identifier if switch statments not enabled for lsl preproc' but its body actually tests the opposite behavior — that switch is processed as a statement when lslLanguageConfigWithSwitch is used. This is a duplicate name of the test at line 1598, and the name does not match the intent of the test. The name should reflect that switch statements are correctly expanded when switch support is enabled.
| test('should allow switch as identifier if switch statments not enabled for lsl preproc', async () => { | |
| test('should expand switch statement when switch statements enabled for lsl preproc', async () => { |
| import { Lexer } from '../../shared/lexer'; | ||
| import { getLanguageConfig, Lexer } from '../../shared/lexer'; | ||
| import { HostInterface, NormalizedPath, normalizePath } from '../../interfaces/hostinterface'; | ||
| import { get } from 'http'; |
There was a problem hiding this comment.
There is an unused import import { get } from 'http'; on line 18. This appears to be an accidental leftover that should be removed.
| import { get } from 'http'; |
Add Switch statement support a la firestorm preprocessor
Closes #38
This also include a fairly large refactor to make most of the parsers lexers and processors, use 1 language config passed in from the top down, rather than each fetching their own copy.
I did this so that the config switch for enabling
switchstatements can be checked once at the start, and then haveswitchadded to the lsl language config for that run.