Opt ShaderLab module code #2419
Conversation
WalkthroughThe changes in this pull request involve modifications across several files in the shader-lab package. Key updates include the addition of new methods, such as Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2419 +/- ##
===========================================
+ Coverage 67.86% 67.90% +0.03%
===========================================
Files 907 908 +1
Lines 94311 94362 +51
Branches 7890 7917 +27
===========================================
+ Hits 64001 64073 +72
+ Misses 30060 30039 -21
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
packages/shader-lab/src/preprocessor/PpScanner.ts (2)
Line range hint
301-316: Maintain consistency in advance method usage.While most of the file has been updated to use
_advance(), this method still usesadvance(2)for skipping comment markers. Consider updating these calls for consistency with the rest of the changes.- this.advance(2); + this._advance(2);
Line range hint
1-316: Overall consistent refactoring with one suggestion.The changes successfully refactor most scanner advancement calls to use
_advance(), improving code consistency. However, there's one remaining instance in_skipCommentsthat should be updated for complete consistency.Consider adding tests to verify that the scanner advancement behavior remains unchanged after these refactoring changes.
Would you like me to help generate test cases for the scanner advancement behavior?
packages/shader-lab/src/lexer/Lexer.ts (2)
Line range hint
286-294: Add safety checks and escape sequence support to string scanningThe string constant scanning has several potential issues:
- No handling of escape sequences (\n, \t, etc.)
- Could enter infinite loop on unterminated strings
- No maximum string length validation
Consider adding these safety features:
private _scanStringConst() { const start = this._getPosition(); const buffer: string[] = []; + const maxLength = 65536; // Or other appropriate limit while (this.getCurChar() !== '"') { + if (buffer.length >= maxLength) { + this.throwError(this._getPosition(), "String constant exceeds maximum length"); + } + if (this.isEnd()) { + this.throwError(this._getPosition(), "Unterminated string constant"); + } + if (this.getCurChar() === '\\') { + this._advance(1); + buffer.push(this._handleEscapeSequence()); + continue; + } buffer.push(this.getCurChar()); - this._advance(); + this._advance(1); } - this._advance(); + this._advance(1); const range = ShaderLab.createRange(start, this._getPosition()); const token = BaseToken.pool.get(); token.set(ETokenType.STRING_CONST, buffer.join(""), range); return token; } + private _handleEscapeSequence(): string { + const char = this.getCurChar(); + switch (char) { + case 'n': return '\n'; + case 't': return '\t'; + case 'r': return '\r'; + case '\\': return '\\'; + case '"': return '"'; + default: + this.throwError(this._getPosition(), `Invalid escape sequence \\${char}`); + } + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 291-291: packages/shader-lab/src/lexer/Lexer.ts#L291
Added line #L291 was not covered by tests
[warning] 293-293: packages/shader-lab/src/lexer/Lexer.ts#L293
Added line #L293 was not covered by tests
Line range hint
344-377: Improve numeric literal scanning robustnessThe number scanning implementation could be more robust:
- No validation of maximum numeric values
- Complex nested conditions make the code harder to maintain
- Duplicate numeric scanning logic in multiple places
Consider refactoring to improve maintainability:
private _scanNum() { + const start = this._getPosition(); const buffer: string[] = []; - while (LexerUtils.isNum(this.getCurChar())) { - buffer.push(this.getCurChar()); - this._advance(); - } + this._scanDigits(buffer); + if (this.getCurChar() === ".") { buffer.push(this.getCurChar()); - this._advance(); + this._advance(1); - while (LexerUtils.isNum(this.getCurChar())) { - buffer.push(this.getCurChar()); - this._advance(); - } + this._scanDigits(buffer); this._scanFloatSuffix(buffer); const token = BaseToken.pool.get(); - token.set(ETokenType.FLOAT_CONSTANT, buffer.join(""), this._getPosition(buffer.length)); + token.set(ETokenType.FLOAT_CONSTANT, buffer.join(""), start); return token; } // ... rest of the method } + private _scanDigits(buffer: string[]) { + while (LexerUtils.isNum(this.getCurChar())) { + buffer.push(this.getCurChar()); + this._advance(1); + } + }packages/shader-lab/src/contentParser/ShaderContentParser.ts (3)
224-226: Consider using class name instead ofthisin static contextWhile extracting the scope dropping logic improves code maintainability, using
thisin static methods can be confusing. Consider using the class name directly.private static _dropScope() { - this._symbolTable.dropScope(); + ShaderContentParser._symbolTable.dropScope(); }🧰 Tools
🪛 Biome
[error] 225-225: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
416-416: Replacethiswith class name for static method callThe scope cleanup is correctly placed at the end of subshader parsing. However, for consistency with static method usage:
- this._dropScope(); + ShaderContentParser._dropScope();🧰 Tools
🪛 Biome
[error] 416-416: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
515-515: Replacethiswith class name for static method callThe scope cleanup is correctly placed at the end of pass parsing. However, for consistency with static method usage:
- this._dropScope(); + ShaderContentParser._dropScope();🧰 Tools
🪛 Biome
[error] 515-515: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
416-420: Consider consolidating parenthesis handling logic.The parenthesis parsing logic could be made more robust by:
- Adding explicit error handling for mismatched parentheses
- Considering a helper method for paired character matching
private static _parseParenthesisExpression(scanner: PpScanner): PpConstant { if (scanner.getCurChar() === "(") { scanner._advance(); scanner.skipSpace(false); const ret = this._parseConstantExpression(scanner); - scanner.scanToChar(")"); - scanner._advance(); + if (!scanner.scanToChar(")")) { + this.reportError( + scanner.getShaderPosition(), + "Mismatched parentheses in expression", + scanner.source, + scanner.file + ); + return; + } + scanner._advance(); return ret; } return this._parseConstant(scanner); }🧰 Tools
🪛 Biome
[error] 418-418: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Line range hint
667-686: Improve parameter parsing robustness in_expandToken.The current implementation of macro parameter parsing could be improved:
- Consider extracting the parameter parsing logic into a separate method
- Add validation for maximum parameter count
- Improve error handling for malformed parameter lists
private static _expandToken(token: BaseToken, scanner: PpScanner) { const macro = this._definedMacros.get(token.lexeme); if (macro) { let replace = macro.body.lexeme; if (macro.isFunction) { scanner.scanToChar("("); scanner._advance(); - // extract parameters - const args: string[] = []; - let curLvl = 1; - let curIdx = scanner.current; - while (true) { - if (scanner.getCurChar() === "(") curLvl += 1; - else if (scanner.getCurChar() === ")") { - curLvl -= 1; - if (curLvl === 0) break; - } else if (scanner.getCurChar() === "," && curLvl === 1) { - args.push(scanner.source.slice(curIdx, scanner.current)); - curIdx = scanner.current + 1; - } - scanner._advance(); - } - args.push(scanner.source.slice(curIdx, scanner.current)); + const args = this._parseMacroParameters(scanner, macro); + if (!args) return; scanner._advance(); const range = ShaderLab.createRange(token.location!.start, scanner.getCurPosition());Add this new method:
private static _parseMacroParameters(scanner: PpScanner, macro: MacroDefine): string[] | undefined { const args: string[] = []; let curLvl = 1; let curIdx = scanner.current; const maxParams = macro.args?.length ?? 0; while (true) { if (scanner.isEnd()) { this.reportError( scanner.getShaderPosition(), "Unexpected end of input while parsing macro parameters", scanner.source, scanner.file ); return; } if (scanner.getCurChar() === "(") curLvl += 1; else if (scanner.getCurChar() === ")") { curLvl -= 1; if (curLvl === 0) break; } else if (scanner.getCurChar() === "," && curLvl === 1) { args.push(scanner.source.slice(curIdx, scanner.current).trim()); if (args.length >= maxParams) { this.reportError( scanner.getShaderPosition(), `Too many parameters for macro ${macro.name}`, scanner.source, scanner.file ); return; } curIdx = scanner.current + 1; } scanner._advance(); } args.push(scanner.source.slice(curIdx, scanner.current).trim()); if (args.length !== maxParams) { this.reportError( scanner.getShaderPosition(), `Expected ${maxParams} parameters but got ${args.length}`, scanner.source, scanner.file ); return; } return args; }🧰 Tools
🪛 Biome
[error] 689-689: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/shader-lab/src/common/BaseScanner.ts(1 hunks)packages/shader-lab/src/contentParser/ShaderContentParser.ts(3 hunks)packages/shader-lab/src/lexer/Lexer.ts(10 hunks)packages/shader-lab/src/preprocessor/PpParser.ts(6 hunks)packages/shader-lab/src/preprocessor/PpScanner.ts(5 hunks)
🧰 Additional context used
🪛 Biome
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 225-225: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 416-416: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 515-515: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 418-418: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/lexer/Lexer.ts
[warning] 53-53: packages/shader-lab/src/lexer/Lexer.ts#L53
Added line #L53 was not covered by tests
[warning] 55-55: packages/shader-lab/src/lexer/Lexer.ts#L55
Added line #L55 was not covered by tests
[warning] 72-72: packages/shader-lab/src/lexer/Lexer.ts#L72
Added line #L72 was not covered by tests
[warning] 74-74: packages/shader-lab/src/lexer/Lexer.ts#L74
Added line #L74 was not covered by tests
[warning] 81-81: packages/shader-lab/src/lexer/Lexer.ts#L81
Added line #L81 was not covered by tests
[warning] 105-105: packages/shader-lab/src/lexer/Lexer.ts#L105
Added line #L105 was not covered by tests
[warning] 119-119: packages/shader-lab/src/lexer/Lexer.ts#L119
Added line #L119 was not covered by tests
[warning] 127-127: packages/shader-lab/src/lexer/Lexer.ts#L127
Added line #L127 was not covered by tests
[warning] 129-129: packages/shader-lab/src/lexer/Lexer.ts#L129
Added line #L129 was not covered by tests
[warning] 137-137: packages/shader-lab/src/lexer/Lexer.ts#L137
Added line #L137 was not covered by tests
[warning] 139-139: packages/shader-lab/src/lexer/Lexer.ts#L139
Added line #L139 was not covered by tests
[warning] 143-143: packages/shader-lab/src/lexer/Lexer.ts#L143
Added line #L143 was not covered by tests
[warning] 157-157: packages/shader-lab/src/lexer/Lexer.ts#L157
Added line #L157 was not covered by tests
[warning] 165-165: packages/shader-lab/src/lexer/Lexer.ts#L165
Added line #L165 was not covered by tests
[warning] 167-167: packages/shader-lab/src/lexer/Lexer.ts#L167
Added line #L167 was not covered by tests
[warning] 171-171: packages/shader-lab/src/lexer/Lexer.ts#L171
Added line #L171 was not covered by tests
[warning] 202-202: packages/shader-lab/src/lexer/Lexer.ts#L202
Added line #L202 was not covered by tests
[warning] 204-204: packages/shader-lab/src/lexer/Lexer.ts#L204
Added line #L204 was not covered by tests
[warning] 257-257: packages/shader-lab/src/lexer/Lexer.ts#L257
Added line #L257 was not covered by tests
[warning] 267-267: packages/shader-lab/src/lexer/Lexer.ts#L267
Added line #L267 was not covered by tests
[warning] 272-272: packages/shader-lab/src/lexer/Lexer.ts#L272
Added line #L272 was not covered by tests
[warning] 277-277: packages/shader-lab/src/lexer/Lexer.ts#L277
Added line #L277 was not covered by tests
[warning] 291-291: packages/shader-lab/src/lexer/Lexer.ts#L291
Added line #L291 was not covered by tests
[warning] 293-293: packages/shader-lab/src/lexer/Lexer.ts#L293
Added line #L293 was not covered by tests
[warning] 305-305: packages/shader-lab/src/lexer/Lexer.ts#L305
Added line #L305 was not covered by tests
[warning] 396-396: packages/shader-lab/src/lexer/Lexer.ts#L396
Added line #L396 was not covered by tests
[warning] 403-403: packages/shader-lab/src/lexer/Lexer.ts#L403
Added line #L403 was not covered by tests
packages/shader-lab/src/preprocessor/PpParser.ts
[warning] 349-349: packages/shader-lab/src/preprocessor/PpParser.ts#L349
Added line #L349 was not covered by tests
[warning] 395-395: packages/shader-lab/src/preprocessor/PpParser.ts#L395
Added line #L395 was not covered by tests
🔇 Additional comments (11)
packages/shader-lab/src/common/BaseScanner.ts (2)
76-83: Consider unifying the implementation between verbose and non-verbose modes.
The current implementation has different behaviors between verbose and non-verbose modes:
- Verbose mode: Incremental advancement with position tracking
- Non-verbose mode: Direct index increment without position tracking
This could lead to inconsistencies in debugging and behavior. Consider:
- Either using the same implementation for both modes
- Or documenting the behavioral differences clearly
Let's verify if there are any dependencies on the verbose mode behavior:
#!/bin/bash
# Search for usage of position tracking variables
rg -A 2 '(_line|_column)' --type ts76-76: Breaking Change: Method signature requires explicit count parameter.
The change from advance(count = 1) to advance(count: number) is a breaking change that requires all callers to provide an explicit count parameter.
Let's verify the impact on existing callers:
✅ Verification successful
Let me try a different search pattern to find calls to the advance() method:
Based on the search results, I can now generate the final response:
The change to advance(count: number) is safe and consistent with usage.
The search results show that all calls to advance() across the codebase are made with explicit numeric arguments:
- Direct numeric values:
advance(2)for skipping multi-character tokens - No instances of calling
advance()without parameters - Most calls are to the internal
_advance()method, which is separate from the publicadvance(count)method
The change from optional to required parameter actually better reflects the real usage in the codebase, making the API more explicit and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to advance() without parameters
rg -A 2 'advance\(\s*\)' --type ts
Length of output: 5831
Script:
#!/bin/bash
# Search for calls to advance method
rg 'advance\(' --type ts -A 2
Length of output: 24242
packages/shader-lab/src/preprocessor/PpScanner.ts (5)
74-74: LGTM! Consistent usage of _advance().
The changes maintain the same scanning logic while consistently using the new _advance() method.
Also applies to: 84-84
168-168: LGTM! Simple and correct usage.
The change to _advance() is appropriate for this scanning operation.
235-235: LGTM! Proper numeric scanning.
The change to _advance() maintains the correct integer scanning behavior.
264-264: LGTM! Maintains comment handling integrity.
The change to _advance() preserves the correct line scanning behavior while properly integrating with comment handling.
63-63: Verify base class method visibility.
The change from advance() to _advance() suggests this is now using a protected/private method. Ensure that _advance() is properly declared in the BaseScanner class with the correct visibility.
Also applies to: 66-66
✅ Verification successful
Method visibility change is correct and consistent
The change from advance() to _advance() is verified to be correct. The _advance() method is properly defined in the BaseScanner class and is being used consistently across multiple derived classes (PpScanner, Lexer, Scanner). The underscore prefix convention is maintained throughout the codebase, indicating a protected/private method that should be used by derived classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the BaseScanner class definition and its advance method
ast-grep --pattern 'class BaseScanner {
$$$
_advance() {
$$$
}
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Let's try a broader search for BaseScanner class and any advance-related methods
rg -A 5 "class BaseScanner"
# Also search for _advance method declarations
rg "_advance\(\)"
Length of output: 7707
Script:
#!/bin/bash
# Let's check the implementation of _advance in BaseScanner
rg -A 5 "_advance\(\): void" packages/shader-lab/src/common/BaseScanner.ts
# Also check if there are any public advance methods
rg "advance\(\)" packages/shader-lab/src/common/BaseScanner.ts
Length of output: 583
packages/shader-lab/src/lexer/Lexer.ts (1)
Line range hint 51-272: Add test coverage for token scanning paths
Many token scanning paths lack test coverage, including:
- Compound operators (<<=, >>=, etc.)
- Increment/decrement operators
- Bitwise operators
- String constants
- Various punctuation tokens
This is concerning for a lexer implementation where correctness is critical.
Let's verify the current test coverage:
Would you like me to help generate comprehensive test cases for these uncovered paths?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-53: packages/shader-lab/src/lexer/Lexer.ts#L53
Added line #L53 was not covered by tests
[warning] 55-55: packages/shader-lab/src/lexer/Lexer.ts#L55
Added line #L55 was not covered by tests
[warning] 72-72: packages/shader-lab/src/lexer/Lexer.ts#L72
Added line #L72 was not covered by tests
[warning] 74-74: packages/shader-lab/src/lexer/Lexer.ts#L74
Added line #L74 was not covered by tests
[warning] 81-81: packages/shader-lab/src/lexer/Lexer.ts#L81
Added line #L81 was not covered by tests
[warning] 105-105: packages/shader-lab/src/lexer/Lexer.ts#L105
Added line #L105 was not covered by tests
[warning] 119-119: packages/shader-lab/src/lexer/Lexer.ts#L119
Added line #L119 was not covered by tests
[warning] 127-127: packages/shader-lab/src/lexer/Lexer.ts#L127
Added line #L127 was not covered by tests
[warning] 129-129: packages/shader-lab/src/lexer/Lexer.ts#L129
Added line #L129 was not covered by tests
[warning] 137-137: packages/shader-lab/src/lexer/Lexer.ts#L137
Added line #L137 was not covered by tests
[warning] 139-139: packages/shader-lab/src/lexer/Lexer.ts#L139
Added line #L139 was not covered by tests
[warning] 143-143: packages/shader-lab/src/lexer/Lexer.ts#L143
Added line #L143 was not covered by tests
[warning] 157-157: packages/shader-lab/src/lexer/Lexer.ts#L157
Added line #L157 was not covered by tests
[warning] 165-165: packages/shader-lab/src/lexer/Lexer.ts#L165
Added line #L165 was not covered by tests
[warning] 167-167: packages/shader-lab/src/lexer/Lexer.ts#L167
Added line #L167 was not covered by tests
[warning] 171-171: packages/shader-lab/src/lexer/Lexer.ts#L171
Added line #L171 was not covered by tests
packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
Line range hint 224-515: Overall refactoring improves code maintainability
The introduction of _dropScope() and its consistent usage across the parser methods is a good refactoring that:
- Centralizes scope management logic
- Makes the code more maintainable
- Preserves the existing parsing functionality
The only improvement needed is to use the class name instead of this in static contexts for better clarity.
🧰 Tools
🪛 Biome
[error] 221-221: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 225-225: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
434-434: LGTM: Consistent use of _advance().
The change to use _advance() in _parseConstant is consistent with the overall refactoring pattern.
349-349:
Add test coverage for the _parseAdditiveExpression method.
The modified line using _advance() is not covered by tests. Since this is part of the expression parsing logic, it's crucial to have test coverage to ensure correct operator handling.
Consider adding test cases for:
- Basic addition/subtraction
- Operator precedence
- Error handling for invalid operators
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 349-349: packages/shader-lab/src/preprocessor/PpParser.ts#L349
Added line #L349 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
29-29: Dispose of pooled objects upon return, not retrievalCalling
ret.dispose()immediately after obtaining it from the pool suggests that the object may not be reset upon retrieval. It's a best practice to clean or reset pooled objects before returning them to the pool to ensure they are in a consistent state when reused.packages/shader-lab/src/lexer/Utils.ts (1)
6-12: MethodisLetterincludes digits; consider renaming or adjusting implementationThe
isLettermethod currently checks for letters, digits, and the underscore character. Including digits may not align with the expected behavior implied by the method nameisLetter.Consider renaming the method to
isLetterOrDigitor adjusting the implementation to exclude digits if only letters are intended.packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
225-227: Use class name instead ofthisin static methodsUsing
thisin static methods can be confusing. For better clarity, use the class name directly.Apply this diff to improve static method usage:
private static _dropScope() { - this._symbolTable.dropScope(); + ShaderContentParser._symbolTable.dropScope(); } // In _parseSubShader - this._dropScope(); + ShaderContentParser._dropScope(); // In _parsePass - this._dropScope(); + ShaderContentParser._dropScope();Also applies to: 409-409, 508-508
🧰 Tools
🪛 Biome (1.9.4)
[error] 226-226: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts (1)
Line range hint
302-317: Enhance error messages with type information.The error messages for invalid operators could be more helpful by including:
- The actual types of the operands
- The expected types
- The operator being used
Example enhancement:
-this.reportError(opPos, "invalid operator in relation expression.", scanner.source, scanner.file); +this.reportError(opPos, `invalid operator '${operator}' for types ${typeof operand1} and ${typeof operand2}`, scanner.source, scanner.file);Also applies to: 328-339, 374-389
🧰 Tools
🪛 Biome (1.9.4)
[error] 305-305: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
packages/core/src/shader/ShaderPass.ts(2 hunks)packages/loader/src/gltf/extensions/EXT_texture_webp.ts(1 hunks)packages/shader-lab/src/TempArray.ts(1 hunks)packages/shader-lab/src/codeGen/CodeGenVisitor.ts(1 hunks)packages/shader-lab/src/common/BaseScanner.ts(3 hunks)packages/shader-lab/src/contentParser/ShaderContentParser.ts(3 hunks)packages/shader-lab/src/lexer/Lexer.ts(10 hunks)packages/shader-lab/src/lexer/Utils.ts(1 hunks)packages/shader-lab/src/preprocessor/PpParser.ts(15 hunks)packages/shader-lab/src/preprocessor/PpScanner.ts(11 hunks)tests/src/shader-lab/ShaderValidate.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loader/src/gltf/extensions/EXT_texture_webp.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shader-lab/src/lexer/Lexer.ts
- packages/shader-lab/src/preprocessor/PpScanner.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 226-226: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 409-409: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 508-508: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 420-420: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 425-425: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 459-459: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (7)
packages/shader-lab/src/common/BaseScanner.ts (1)
80-88: Add validation for the count parameter in advance() method
The advance() method lacks validation for the count parameter. Consider adding checks for negative values and values that would exceed the source length to prevent potential errors or unexpected behavior.
packages/shader-lab/src/TempArray.ts (1)
3-9: Ensure proper disposal and initialization of TempArray instances
The TempArray class correctly implements IPoolElement and provides a dispose() method to reset the array. Ensure that dispose() is called before returning instances to the pool and that instances are properly initialized when retrieved.
packages/shader-lab/src/lexer/Utils.ts (1)
2-3: Ensure consistent character code usage in methods
The methods now accept charCode: number parameters instead of char: string, which improves performance by avoiding string comparisons. Verify that all calls to these methods have been updated accordingly throughout the codebase.
Run the following script to check for any remaining usages of the old method signatures:
✅ Verification successful
All method calls are using charCode parameters correctly
Based on the search results, all calls to LexerUtils.isNum(), LexerUtils.isLetter(), and LexerUtils.isAlpha() are using charCode parameters obtained through getCurCharCode() method calls. There are no instances of string parameters being passed to these methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to LexerUtils methods using string parameters.
rg --pcre2 'LexerUtils\.(isNum|isLetter|isAlpha)\(\s*([\'"]).\2\s*\)' -t typescript -A 2
Length of output: 116
Script:
#!/bin/bash
# Search for all usages of LexerUtils methods
rg "LexerUtils\.(isNum|isLetter|isAlpha)" -t typescript -A 2
Length of output: 97
Script:
#!/bin/bash
# Find all files that import or use LexerUtils
rg "LexerUtils" -t typescript --files-with-matches
Length of output: 85
Script:
#!/bin/bash
# Get the implementation of LexerUtils class to see all available methods
ast-grep --pattern 'class LexerUtils {
$$$
}'
Length of output: 2809
Script:
#!/bin/bash
# Search for all usages of LexerUtils methods without typescript file type restriction
rg "LexerUtils\.(isNum|isLetter|isAlpha)" -g "*.ts" -g "*.js" -A 2
Length of output: 3751
Script:
#!/bin/bash
# Find all files that import or use LexerUtils
fd -e ts -e js -x rg "LexerUtils" {}
Length of output: 1171
tests/src/shader-lab/ShaderValidate.ts (1)
36-36: LGTM: Type assertion improves type safety
The addition of as WebGL2RenderingContext type assertion aligns with the WebGL2 context check and enhances type safety.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
225-227: LGTM: Good refactoring to centralize scope dropping logic
Extracting the scope dropping logic into a dedicated method improves maintainability and follows the DRY principle.
🧰 Tools
🪛 Biome (1.9.4)
[error] 226-226: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
129-129: LGTM: Consistent position tracking implementation.
The standardization of position tracking using getShaderPosition(0) improves code consistency.
Also applies to: 162-162, 205-205
429-429: LGTM: Improved character evaluation performance.
Using getCurCharCode() with LexerUtils methods is more efficient as it:
- Avoids unnecessary string allocations
- Provides direct character code comparisons
Also applies to: 455-455
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/shader-lab/src/ShaderLab.ts(1 hunks)
🔇 Additional comments (4)
packages/shader-lab/src/ShaderLab.ts (4)
Line range hint 98-102: Ensure consistent error handling between parser and class errors
The parser errors are directly pushed to the errors array without type casting, which is consistent with the new Error[] type. However, we should verify that parser errors contain all necessary information.
✅ Verification successful
Parser errors are properly handled with consistent error types
Based on the comprehensive analysis of the codebase, I can verify that parser errors are consistently handled:
- All errors in the parser and throughout the codebase use the
GSErrorclass which extendsError - The
GSErrorclass provides rich error information including:- Error name (from
GSErrorNameenum) - Error message
- Location information (via
ShaderRangeorShaderPosition) - Source code context
- File information (optional)
- Error name (from
- Parser errors are properly type-cast when pushed to the errors array:
this.sematicAnalyzer.errors.push(<GSError>error);
- The error handling is consistent across different components (parser, preprocessor, code generator, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check parser error creation
ast-grep --pattern 'class ShaderTargetParser {
$$$
errors: $_[] = [];
$$$
}'
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search for error handling patterns
rg "errors.*\[\]" -A 5 --type ts
# Also search for parser error creation
rg "parser.*error" -A 5 --type ts
# And check for error type definitions
rg "type.*Error" --type ts
Length of output: 2875
Script:
#!/bin/bash
# Let's check the GSError type definition and usage
rg "GSError" -A 5 --type ts
# Also check how errors are created and handled in the parser
rg "push.*error" -A 5 --type ts
Length of output: 27555
Line range hint 144-150: Consider preserving error type information in ShaderContentParser
The errors from ShaderContentParser are pushed directly. Consider documenting the expected error structure to maintain consistency across different error sources.
✅ Verification successful
Let me analyze the error handling in ShaderContentParser. I can see that the class uses GSError type for errors, which is stored in the static _errors array. Let me check the error type definition and how it's used.
Let me try a different approach to find error-related code and GSError usage.
Based on the search results, I can now provide a final response:
Error handling in ShaderContentParser is well-structured and consistent
The error handling in ShaderContentParser is already well-documented and structured:
- All errors are instances of
GSErrorclass which extends Error GSErrorincludes detailed information:- Error name (from
GSErrorNameenum) - Message
- Source code location (ShaderRange or ShaderPosition)
- Source text
- Optional file path
- Error name (from
- Errors are created consistently using
ShaderLabUtils.createGSError()helper - Error types are properly categorized:
- CompilationError
- PreprocessorError
- ScannerError
The original review comment's concern about documenting error structure is already addressed by the codebase's consistent use of GSError class and the createGSError utility function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ShaderContentParser error types
ast-grep --pattern 'class ShaderContentParser {
$$$
static _errors: $_[] = [];
$$$
}'
Length of output: 47361
Script:
#!/bin/bash
# Check GSError type definition and usage
ast-grep --pattern 'type GSError = $_'
# Also check for GSError interface/class definition
ast-grep --pattern 'interface GSError {
$$$
}'
# And check class definition
ast-grep --pattern 'class GSError {
$$$
}'
Length of output: 138
Script:
#!/bin/bash
# Search for GSError imports and related code
rg "GSError" -A 3
# Search for error creation
rg "createGSError" -A 3
Length of output: 22220
Line range hint 82-89: Verify error type casting in preprocessor error handling
The error casting <GSError>err might be unnecessary now that errors accepts generic Error objects.
Let's check if this casting is still needed:
✅ Verification successful
Let me check the error handling in the codebase to verify if the error type casting is still needed.
Let me try a different approach to check the error handling in ShaderLab.ts.
Let me try one more search to find error handling patterns in the codebase.
Based on the search results, I can now provide a final response about the error type casting concern.
The error type casting to <GSError> is still necessary
The codebase shows that GSError is consistently used throughout the error handling system:
- The
errorsarray in various classes specifically expectsGSErrorobjects ShaderLabUtils.createGSError()is used to create properly typed error objects- Error handling code across multiple files (
ShaderContentParser,ShaderTargetParser,CodeGenVisitor, etc.) explicitly casts errors to<GSError>
The type casting ensures type safety and maintains consistency with the error handling system that expects GSError instances with specific properties like location, name, and custom error formatting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check PpParser error types
ast-grep --pattern 'class PpParser {
$$$
_errors: $_[] = [];
$$$
}'
Length of output: 58086
Script:
#!/bin/bash
# Check error handling in ShaderLab.ts
ast-grep --pattern 'try {
$$$
} catch (err) {
$$$
}'
Length of output: 66154
Script:
#!/bin/bash
# Search for error handling in ShaderLab.ts
rg -A 2 "catch.*err" packages/shader-lab/src/ShaderLab.ts
Length of output: 59
Script:
#!/bin/bash
# Search for GSError usage and error handling
rg -A 2 "GSError" packages/shader-lab/src/
Length of output: 13322
Line range hint 127-131: Verify CodeGen error compatibility
CodeGen errors are pushed directly to the errors array. We should verify that these errors provide sufficient information without GSError-specific properties.
✅ Verification successful
CodeGen errors are properly handled with GSError implementation
After analyzing the codebase, I can confirm that CodeGen errors are properly handled and provide sufficient information:
-
All errors in the codebase, including CodeGen errors, use the
GSErrorclass which extends the standardErrorclass and provides additional shader-specific properties:location: Contains precise position information (line, column)source: Contains the source code contextname: Specific error type (CompilationError for CodeGen)- Enhanced
toString()method that shows the error location with source context
-
The CodeGen visitor specifically creates errors with
GSErrorName.CompilationErrortype and includes all necessary context:new GSError(GSErrorName.CompilationError, message, loc, ShaderLab._processingPassText)
The error handling implementation provides rich debugging information and is consistent across all components of the shader compilation pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check CodeGen error creation in both GLES visitors
ast-grep --pattern 'class $_Visitor {
$$$
errors: $_[] = [];
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Let's search for error handling patterns in the codebase
rg "errors\." -A 3 -B 3 packages/shader-lab/src/
Length of output: 11292
Script:
#!/bin/bash
# Look for error-related type definitions and interfaces
rg "interface.*Error|type.*Error" packages/shader-lab/src/
Length of output: 60
Script:
#!/bin/bash
# Search for error creation and pushing patterns
ast-grep --pattern 'errors.push($_)'
Length of output: 38
Script:
#!/bin/bash
# Check the actual file content for context
cat packages/shader-lab/src/ShaderLab.ts
Length of output: 4298
Script:
#!/bin/bash
# Let's check the GSError implementation
cat packages/shader-lab/src/GSError.ts
Length of output: 1868
Script:
#!/bin/bash
# Search for error type references in the codebase
rg "GSError|GSErrorName" packages/shader-lab/src/
Length of output: 5418
Script:
#!/bin/bash
# Look for error creation in CodeGen visitors
ast-grep --pattern 'new $_Error($$$)'
Length of output: 39
…feat/shader_lab_opt
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
ScannerdropScopeinParserfor greater readabilitySummary by CodeRabbit
New Features
getCurCharCodefor character code retrieval.TempArray<T>for managing temporary arrays.Improvements
LexerUtilswith direct numeric comparisons.Refactor
ShaderContentParser.CodeGenVisitorto an abstract class for better memory management.ShaderLabclass to generalize error types in error handling.