ShaderLab support MRT #2452
ShaderLab support MRT #2452
Conversation
Sway007
added
enhancement
WalkthroughThis pull request introduces support for Multi-Render Target (MRT) rendering in the shader lab system. It includes the addition of a new shader implementation, configuration updates for MRT testing, and modifications to the code generation and parsing processes to accommodate MRT-specific structures and layout qualifiers. The changes span multiple files, enhancing the overall functionality and flexibility of the shader lab environment. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 #2452 +/- ##
===========================================
+ Coverage 68.30% 68.92% +0.61%
===========================================
Files 908 921 +13
Lines 94510 95512 +1002
Branches 7936 8112 +176
===========================================
+ Hits 64557 65832 +1275
+ Misses 29703 29427 -276
- Partials 250 253 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (15)
packages/shader-lab/src/codeGen/GLESVisitor.ts (1)
30-30: LGTM! Well-designed abstract method for MRT support.The abstract method follows the established pattern and maintains separation of concerns by delegating MRT declaration specifics to concrete implementations.
Consider documenting the expected format and requirements of the MRT declarations in JSDoc comments to guide implementers of concrete visitor classes.
packages/shader-lab/src/codeGen/GLES100.ts (2)
30-32: Implement or documentgetReferencedMRTPropTextmethodThe
getReferencedMRTPropTextmethod currently returns an empty string. If this is intentional and will be implemented in the future, consider adding a TODO comment to indicate that. Otherwise, provide an implementation to ensure complete functionality.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-32: packages/shader-lab/src/codeGen/GLES100.ts#L31-L32
Added lines #L31 - L32 were not covered by tests
56-59: Clarify the purpose of returningundefinedingetMRTDeclareThe
getMRTDeclaremethod returnsundefined. If this is expected because MRT declarations are not applicable or required forGLES100Visitor, consider adding a comment to explain this behavior for future maintainability.e2e/case/shaderLab-mrt.ts (1)
84-85: Add error handling for engine creationWhen creating the
WebGLEngine, consider adding error handling for the promise in case of rejection. This enhances robustness and helps in debugging potential issues during engine initialization.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 84-86: e2e/case/shaderLab-mrt.ts#L84-L86
Added lines #L84 - L86 were not covered by testspackages/shader-lab/src/codeGen/CodeGenVisitor.ts (2)
21-21: Specify the error type inerrorspropertyThe
errorsproperty is typed asError[], but it may be more appropriate to specify a more specific error type, such asGSError[], to reflect the actual error instances stored.
211-212: Remove unnecessary override ofvisitLayoutQualifierThe
visitLayoutQualifiermethod currently just callsdefaultCodeGen, which is the default behavior if the method is not overridden. If no additional logic is needed, consider removing this method override.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 211-212: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L211-L212
Added lines #L211 - L212 were not covered by testspackages/shader-lab/src/parser/AST.ts (3)
6-6: Redundant import ofBaseTokenThe import statement includes
BaseTokentwice, both asBaseTokenand aliased asToken. This redundancy can be simplified.Apply this diff to streamline the import:
-import { BaseToken, BaseToken as Token } from "../common/BaseToken"; +import { BaseToken as Token } from "../common/BaseToken";
Line range hint
172-176: Inconsistent use ofpoolproperty initializationClasses like
SingleDeclaration,IntegerConstantExpression,TypeSpecifierNonArray,ExtBuiltinTypeSpecifierNonArray, andPostfixExpressionmanually declare astatic override pool. Since you've introduced the@ASTNodeDecoratorto handlepoolinitialization and non-terminal assignment, consider applying the decorator to these classes for consistency.Refactor the
SingleDeclarationclass as follows:- export class SingleDeclaration extends TreeNode { - static override pool = ShaderLabUtils.createObjectPool(SingleDeclaration); + @ASTNodeDecorator(ENonTerminal.single_declaration) + export class SingleDeclaration extends TreeNode { // Class implementation... }Apply similar refactoring to the other classes mentioned.
Also applies to: 344-348, 375-379, 393-397, 829-833
1067-1077: Add unit tests for the newLayoutQualifierclassThe
LayoutQualifierclass has been introduced to handle layout qualifiers but lacks test coverage. Adding unit tests will help ensure its correctness and prevent future regressions.Consider writing tests that cover various scenarios, including valid and invalid layout qualifiers.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1074-1074: packages/shader-lab/src/parser/AST.ts#L1074
Added line #L1074 was not covered by testspackages/shader-lab/src/parser/builtin/variables.ts (2)
1-1: Remove unused importTypeAnyThe
TypeAnyimport appears to be unused in this file. Consider removing it to keep the imports clean.-import { EKeyword, GalaceanDataType, TypeAny } from "../../common"; +import { EKeyword, GalaceanDataType } from "../../common";
37-37: Add documentation for MRT supportThe addition of
gl_FragDataenables Multi-Render Target (MRT) support, which is a significant feature. Consider adding JSDoc comments to document:
- The purpose and usage of gl_FragData
- The relationship with MRT
- Any WebGL version requirements (e.g., WebGL 2.0)
packages/shader-lab/src/parser/GrammarSymbol.ts (1)
75-75: Document the layout_qualifier's role in MRTThe addition of
layout_qualifiertoENonTerminalenables location specification for multiple render targets. Consider adding comments to explain:
- Its relationship with MRT functionality
- Expected syntax and usage in shaders
- Examples of valid layout qualifiers
packages/shader-lab/src/codeGen/VisitorContext.ts (1)
30-30: Consider adding JSDoc documentation for MRT functionality.Adding documentation would help explain:
- Purpose and usage of MRT properties
- Expected structure of MRT data
- Error conditions and handling
Also applies to: 37-37, 66-68, 100-113
packages/shader-lab/src/common/types.ts (1)
Line range hint
1-1: Consider architectural improvements for MRT implementation.While the core MRT implementation is solid, consider these improvements:
- Add comprehensive test suite for MRT functionality
- Create a dedicated error hierarchy for shader-specific errors
- Add developer documentation explaining MRT usage and limitations
packages/shader-lab/src/lalr/CFG.ts (1)
196-209: Consider extending layout qualifier to support multiple layout parametersThe current implementation only supports a single
locationparameter in the layout qualifier. However, GLSL layout qualifiers commonly support multiple parameters (e.g.,layout(location = 0, binding = 1)).Consider extending the grammar to support multiple layout parameters:
...GrammarUtils.createProductionWithOptions( ENonTerminal.layout_qualifier, [ [ EKeyword.LAYOUT, ETokenType.LEFT_PAREN, - EKeyword.LOCATION, - ETokenType.EQUAL, - ETokenType.INT_CONSTANT, + ENonTerminal.layout_qualifier_id_list, ETokenType.RIGHT_PAREN ] ], ASTNode.LayoutQualifier.pool ), + ...GrammarUtils.createProductionWithOptions( + ENonTerminal.layout_qualifier_id_list, + [ + [ENonTerminal.layout_qualifier_id], + [ENonTerminal.layout_qualifier_id_list, ETokenType.COMMA, ENonTerminal.layout_qualifier_id] + ], + ASTNode.LayoutQualifierIdList.pool + ), + ...GrammarUtils.createProductionWithOptions( + ENonTerminal.layout_qualifier_id, + [ + [EKeyword.LOCATION, ETokenType.EQUAL, ETokenType.INT_CONSTANT], + [EKeyword.BINDING, ETokenType.EQUAL, ETokenType.INT_CONSTANT] + ], + ASTNode.LayoutQualifierId.pool + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
e2e/fixtures/originImage/Material_shaderLab-mrt.jpgis excluded by!**/*.jpgtests/src/shader-lab/shaders/mrt-webgl1-normal.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/mrt-webgl1-struct.shaderis excluded by!**/*.shader
📒 Files selected for processing (21)
e2e/case/shaderLab-mrt.ts(1 hunks)e2e/config.ts(1 hunks)packages/shader-lab/src/GSError.ts(1 hunks)packages/shader-lab/src/ShaderLab.ts(1 hunks)packages/shader-lab/src/codeGen/CodeGenVisitor.ts(4 hunks)packages/shader-lab/src/codeGen/GLES100.ts(3 hunks)packages/shader-lab/src/codeGen/GLES300.ts(4 hunks)packages/shader-lab/src/codeGen/GLESVisitor.ts(3 hunks)packages/shader-lab/src/codeGen/VisitorContext.ts(5 hunks)packages/shader-lab/src/common/Keywords.ts(2 hunks)packages/shader-lab/src/common/types.ts(2 hunks)packages/shader-lab/src/lalr/CFG.ts(1 hunks)packages/shader-lab/src/lalr/Utils.ts(0 hunks)packages/shader-lab/src/lexer/Lexer.ts(0 hunks)packages/shader-lab/src/parser/AST.ts(48 hunks)packages/shader-lab/src/parser/GrammarSymbol.ts(1 hunks)packages/shader-lab/src/parser/TargetParser.y(2 hunks)packages/shader-lab/src/parser/builtin/variables.ts(2 hunks)packages/shader-lab/src/parser/types.ts(1 hunks)tests/src/shader-lab/ShaderLab.test.ts(2 hunks)tests/src/shader-lab/ShaderValidate.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/shader-lab/src/lalr/Utils.ts
- packages/shader-lab/src/lexer/Lexer.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
e2e/config.ts
[warning] 113-117: e2e/config.ts#L113-L117
Added lines #L113 - L117 were not covered by tests
e2e/case/shaderLab-mrt.ts
[warning] 6-8: e2e/case/shaderLab-mrt.ts#L6-L8
Added lines #L6 - L8 were not covered by tests
[warning] 10-10: e2e/case/shaderLab-mrt.ts#L10
Added line #L10 was not covered by tests
[warning] 12-12: e2e/case/shaderLab-mrt.ts#L12
Added line #L12 was not covered by tests
[warning] 84-86: e2e/case/shaderLab-mrt.ts#L84-L86
Added lines #L84 - L86 were not covered by tests
[warning] 88-90: e2e/case/shaderLab-mrt.ts#L88-L90
Added lines #L88 - L90 were not covered by tests
[warning] 93-95: e2e/case/shaderLab-mrt.ts#L93-L95
Added lines #L93 - L95 were not covered by tests
[warning] 98-106: e2e/case/shaderLab-mrt.ts#L98-L106
Added lines #L98 - L106 were not covered by tests
[warning] 108-108: e2e/case/shaderLab-mrt.ts#L108
Added line #L108 was not covered by tests
[warning] 110-111: e2e/case/shaderLab-mrt.ts#L110-L111
Added lines #L110 - L111 were not covered by tests
packages/shader-lab/src/codeGen/GLESVisitor.ts
[warning] 62-62: packages/shader-lab/src/codeGen/GLESVisitor.ts#L62
Added line #L62 was not covered by tests
[warning] 115-116: packages/shader-lab/src/codeGen/GLESVisitor.ts#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 120-121: packages/shader-lab/src/codeGen/GLESVisitor.ts#L120-L121
Added lines #L120 - L121 were not covered by tests
packages/shader-lab/src/codeGen/GLES100.ts
[warning] 27-28: packages/shader-lab/src/codeGen/GLES100.ts#L27-L28
Added lines #L27 - L28 were not covered by tests
[warning] 31-32: packages/shader-lab/src/codeGen/GLES100.ts#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 65-71: packages/shader-lab/src/codeGen/GLES100.ts#L65-L71
Added lines #L65 - L71 were not covered by tests
packages/shader-lab/src/codeGen/VisitorContext.ts
[warning] 105-111: packages/shader-lab/src/codeGen/VisitorContext.ts#L105-L111
Added lines #L105 - L111 were not covered by tests
packages/shader-lab/src/codeGen/GLES300.ts
[warning] 116-117: packages/shader-lab/src/codeGen/GLES300.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
packages/shader-lab/src/codeGen/CodeGenVisitor.ts
[warning] 63-64: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L63-L64
Added lines #L63 - L64 were not covered by tests
[warning] 211-212: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 215-217: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L215-L217
Added lines #L215 - L217 were not covered by tests
packages/shader-lab/src/parser/AST.ts
[warning] 1074-1074: packages/shader-lab/src/parser/AST.ts#L1074
Added line #L1074 was not covered by tests
🔇 Additional comments (25)
packages/shader-lab/src/codeGen/GLESVisitor.ts (1)
126-128: LGTM! Verify MRT declaration ordering.
The integration of MRT declarations into the global code generation is well-implemented. The nullish coalescing operator provides a clean fallback, and the sort order is maintained for consistent output.
Let's verify the MRT implementation in concrete visitor classes:
✅ Verification successful
The previous pattern might have been too specific. Let's try a broader search to understand the MRT implementation and its ordering in the codebase.
The previous searches didn't give us enough context about MRT declarations. Let's try to find more specific implementation details about shader declarations and MRT handling.
Based on the search results, I can now generate the final response since I have enough context about the MRT implementation. The code shows that:
- The MRT declarations are properly ordered through the
sortoperation inGLESVisitor.ts(line 127-129) - The GLES300 implementation handles MRT declarations with proper location indices
- The nullish coalescing operator provides a clean fallback for GLES100 which doesn't support MRT
LGTM! MRT declaration ordering is correctly implemented.
The MRT declarations are properly sorted by their index before being joined into the final shader code. The implementation ensures consistent output ordering through:
- Sort by declaration index in global code generation
- Proper location indices in GLES300
- Clean fallback for GLES100
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MRT implementation in GLES visitors
# Look for getMRTDeclare implementations
ast-grep --pattern 'getMRTDeclare() {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Let's try a broader search pattern to find MRT-related methods
ast-grep --pattern 'getMRTDeclare'
# Also search for any MRT-related code
rg -i "mrt" --type ts
Length of output: 66180
Script:
#!/bin/bash
# Let's search for MRT-related code in shader files and implementation
rg -A 5 "MRT" --type ts
# Also look for any shader declarations or related code
rg -A 5 "declare" --type ts
Length of output: 81968
packages/shader-lab/src/codeGen/GLES100.ts (1)
26-28: Add unit tests for getFragDataCodeGen method
The newly added getFragDataCodeGen method is not covered by unit tests. To ensure its correctness and prevent future regressions, please add appropriate unit tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-28: packages/shader-lab/src/codeGen/GLES100.ts#L27-L28
Added lines #L27 - L28 were not covered by tests
e2e/case/shaderLab-mrt.ts (2)
53-56: Check shader compatibility with GLES100
The layout qualifiers used in the mrt struct may not be supported in GLSL ES 100. Since the engine might target WebGL1 (which uses GLSL ES 100), please ensure that the shader code is compatible with all intended platforms or provide alternative code paths.
88-88: Verify shader compilation success
After creating the shader with Shader.create(shaderSource), it would be beneficial to check for compilation errors and handle them appropriately. This ensures that any issues with the shader source are caught early.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-90: e2e/case/shaderLab-mrt.ts#L88-L90
Added lines #L88 - L90 were not covered by tests
packages/shader-lab/src/codeGen/GLES300.ts (4)
25-27: Add unit tests for getFragDataCodeGen method
The getFragDataCodeGen method is crucial for MRT support in GLES300. Please add unit tests to verify its correctness and to prevent regressions.
29-32: Add unit tests for getReferencedMRTPropText method
Unit tests for getReferencedMRTPropText will ensure that the layout declarations for MRT outputs are generated correctly. This is important for shader correctness.
58-76: Validate MRT declarations in getMRTDeclare
Ensure that the getMRTDeclare method handles all possible cases in _referencedMRTList, including entries that are strings or objects with mrtIndex. Consider adding unit tests to cover different scenarios.
116-117: Add unit tests for error handling when using gl_FragColor with MRT
An error is reported if gl_FragColor is used with MRT in the visitVariableIdentifier method. Please add unit tests to confirm that this error is correctly triggered and handled.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 116-117: packages/shader-lab/src/codeGen/GLES300.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3)
23-25: Ensure all subclasses implement new abstract methods
With the addition of abstract methods getFragDataCodeGen and getReferencedMRTPropText, verify that all subclasses of CodeGenVisitor implement these methods to prevent runtime errors.
63-64: Add unit tests for error handling in visitPostfixExpression
The error handling added in visitPostfixExpression should be covered by unit tests to ensure errors are reported and handled as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 63-64: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L63-L64
Added lines #L63 - L64 were not covered by tests
215-217: Verify error creation in _reportError
The _reportError method now uses ShaderLabUtils.createGSError to generate errors. Ensure that this utility function provides all necessary error information and that the errors are correctly processed downstream.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 215-217: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L215-L217
Added lines #L215 - L217 were not covered by tests
packages/shader-lab/src/parser/types.ts (2)
9-13: Refactored constructors enhance code clarity
Using public parameter properties in the SymbolType constructor improves code readability by reducing boilerplate code.
17-21: Improved StructProp constructor with optional mrtIndex
The refactored constructor makes mrtIndex optional, adding flexibility to the StructProp class without impacting existing functionality.
packages/shader-lab/src/GSError.ts (1)
50-51: Fixed error span calculation for same-line errors
Changing else if to if allows both conditions to be evaluated when start.line equals end.line. This ensures that remarkStart and remarkEnd are correctly set for errors occurring on a single line.
packages/shader-lab/src/codeGen/VisitorContext.ts (2)
30-30: LGTM! MRT properties follow established patterns.
The implementation of MRT-related properties follows the same pattern as attributes and varying, maintaining consistency in the codebase.
Also applies to: 37-37
66-68: LGTM! Type checking method follows existing pattern.
The isMRTStruct implementation mirrors the existing isAttributeStruct and isVaryingStruct methods, maintaining consistency.
packages/shader-lab/src/common/types.ts (1)
66-68: LGTM! Type system extensions support MRT functionality.
The additions of layout/location keywords and VEC4_ARRAY type properly support MRT functionality while maintaining type safety.
Also applies to: 214-214
e2e/config.ts (1)
113-117: LGTM! Consider adding test coverage.
The new configuration entry follows the established pattern and maintains consistency with other Material category tests. However, static analysis indicates these lines aren't covered by tests.
Let's verify the test file exists:
✅ Verification successful
LGTM! Test implementation is complete and well-structured
The test file exists at e2e/case/shaderLab-mrt.ts with proper implementation, including shader source code, engine setup, and test scene configuration. The test case follows the established pattern with proper title and category annotations. The corresponding fixture image is also present at the correct location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the test case file
fd "shaderLab-mrt" --type f
Length of output: 105
Script:
#!/bin/bash
# Let's check if the test file contains actual test implementation
cat e2e/case/shaderLab-mrt.ts
Length of output: 3315
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 113-117: e2e/config.ts#L113-L117
Added lines #L113 - L117 were not covered by tests
packages/shader-lab/src/parser/TargetParser.y (2)
31-32: LGTM! Token additions for layout qualifiers.
The new tokens layout and location are required for supporting MRT functionality in GLSL.
111-112: Verify MRT layout qualifier implementation.
The grammar rules for layout qualifiers and struct declarations look correct. However, we should verify the implementation handles all MRT cases correctly.
Let's check for any existing MRT-related implementations:
Also applies to: 115-117
✅ Verification successful
Let me gather more information about the layout qualifier implementation:
MRT layout qualifier implementation is correctly handled
The implementation properly supports MRT (Multiple Render Target) through both WebGL1 and WebGL2/WebGL3 paths:
- WebGL1: Uses
gl_FragData[n]array syntax - WebGL2/3: Uses
layout(location = n)qualifier with proper code generation
Evidence found in:
- Layout qualifier grammar rule correctly defined in
TargetParser.y - GLES300 codegen properly handles layout qualifiers via
layout(location = n) out vec4syntax - Comprehensive test coverage in
mrt-webgl1-normal.shader,mrt-webgl1-struct.shaderand e2e tests - ShaderFactory handles MRT conversion between different GL versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for MRT-related code patterns
# Look for layout qualifiers usage
ast-grep --pattern 'layout ( location = $_)'
# Look for gl_FragData usage
rg "gl_FragData" -A 3
Length of output: 3947
Script:
#!/bin/bash
# Search for layout qualifier related code in parser and codegen
rg "layout.*location" -A 3
# Search for MRT-related code generation
ast-grep --pattern 'class $_ extends $_ {
$$$
visitLayoutQualifier($_) {
$$$
}
$$$
}'
# Look for MRT test cases
fd "mrt" --exec cat {}
Length of output: 13149
tests/src/shader-lab/ShaderLab.test.ts (3)
1-7: LGTM! Clean import organization.
The addition of ShaderPlatformTarget is properly grouped with other core imports.
266-270: LGTM! Comprehensive MRT normal test coverage.
The test validates MRT functionality across both GLES100 (WebGL1) and GLES300 (WebGL2) platforms.
272-276: LGTM! Comprehensive MRT struct test coverage.
The test ensures struct-based MRT works correctly across different platform targets.
Let's verify the test shader files exist:
✅ Verification successful
Let me check the content of these shader files to ensure they properly test MRT functionality with structs.
Let me also check the normal MRT shader for comparison to ensure we have comprehensive coverage.
Verified: Test coverage for MRT is indeed comprehensive
The test files demonstrate thorough coverage of Multiple Render Target (MRT) functionality:
mrt-webgl1-struct.shader: Tests struct-based MRT usinglayout(location = X)syntaxmrt-webgl1-normal.shader: Tests traditional MRT usinggl_FragData[X]syntax- Both shaders are validated against GLES100 and GLES300 targets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of MRT test shader files
fd "mrt-webgl1-(normal|struct).shader" --type f
Length of output: 157
Script:
#!/bin/bash
# Check the content of the MRT struct shader file
cat tests/src/shader-lab/shaders/mrt-webgl1-struct.shader
Length of output: 1030
Script:
#!/bin/bash
# Check the content of the normal MRT shader file
cat tests/src/shader-lab/shaders/mrt-webgl1-normal.shader
Length of output: 886
packages/shader-lab/src/lalr/CFG.ts (2)
190-191: LGTM: Struct declaration rule extended to support layout qualifiers
The addition of the layout qualifier rule to struct declarations is correctly implemented and aligns with the PR objective of adding MRT support.
196-209: Verify layout qualifier usage in shaders
Let's verify how the layout qualifiers are being used in the shaders to ensure the grammar changes align with the actual usage patterns.
✅ Verification successful
Let me gather more information about the layout qualifier grammar and usage by checking the test files and related code.
Based on the gathered information, I can now provide the final response:
Layout qualifier grammar correctly matches shader usage
The current grammar implementation for layout qualifiers correctly matches the usage pattern found in the test shader. The test file shows layout qualifiers being used with location and integer values exactly as defined in the grammar:
layout(location = 0) vec4 fragColor0;
layout(location = 1) vec4 fragColor1;This matches the grammar production that expects:
LAYOUTkeyword- Left parenthesis
LOCATIONkeyword- Equals sign
- Integer constant
- Right parenthesis
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for layout qualifier usage patterns in shader files
# Expected results: Find layout qualifier usage to verify if multiple parameters are needed
# Search for layout qualifier usage in shader files
echo "Searching for layout qualifier usage patterns..."
rg -g "*.{glsl,shader}" "layout\s*\([^)]*\)" -A 2
# Search for struct declarations with layout qualifiers
echo "Searching for struct declarations with layout qualifiers..."
rg -g "*.{glsl,shader}" "layout\s*\([^)]*\)\s*\w+\s+\w+\s*;" -A 2
Length of output: 1011
Script:
#!/bin/bash
# Check the test shader file content to understand the context better
echo "Checking the complete shader file content:"
cat tests/src/shader-lab/shaders/mrt-webgl1-struct.shader
# Search for any other layout qualifier related test files or grammar definitions
echo -e "\nSearching for other layout qualifier related files:"
rg -g "!*.{md,txt}" "layout.*qualifier" -l
# Search for layout qualifier related test cases
echo -e "\nSearching for layout qualifier test cases:"
rg -g "*.{ts,js}" "layout.*qualifier" -A 2
Length of output: 2491
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/vitest.config.ts (1)
19-21: Consider documenting the GPU-related Chrome flagsThe Chrome flags are crucial for WebGL testing but their purpose isn't immediately clear. Consider adding comments explaining why each flag is necessary.
Here's a suggested documentation:
providerOptions: { launch: { - args: ["--use-gl=egl", "--ignore-gpu-blocklist", "--use-gl=angle"] + args: [ + "--use-gl=egl", // Force EGL graphics system + "--ignore-gpu-blocklist", // Bypass GPU compatibility checks + "--use-gl=angle" // Use ANGLE for WebGL rendering + ] } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/shader-lab/src/codeGen/GLES100.ts(3 hunks)tests/vitest.config.ts(1 hunks)
🔇 Additional comments (4)
tests/vitest.config.ts (1)
22-23: Verify WebGL compatibility in headless mode
The addition of headless mode is good for CI/CD, but we should ensure it works reliably with WebGL-dependent shader tests.
Let's verify if there are any WebGL-specific test configurations or known issues:
✅ Verification successful
Let me gather more information about the WebGL test setup and configurations.
Based on the search results, I can see that:
- The project uses WebGL extensively with proper GPU flags in the Vitest config
- The headless mode is being added to a browser-based test setup that includes WebGL/shader tests
- The Chrome flags already include important GPU-related settings:
--use-gl=egl: Forces EGL graphics system--ignore-gpu-blocklist: Bypasses GPU compatibility checks--use-gl=angle: Uses ANGLE for WebGL
Since:
- The configuration already includes the necessary GPU flags for WebGL support
- Chrome's headless mode has mature WebGL support
- The flags explicitly enable GPU features and bypass blockers
Headless mode is compatible with the WebGL test setup
The combination of the existing GPU flags with headless mode is a standard and supported configuration for WebGL testing. The explicit GPU flags ensure WebGL functionality even in headless mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related WebGL test configurations and comments
rg -g '!node_modules' -g '*.{ts,js,md}' -i 'headless.*webgl|webgl.*headless'
# Check if there are any existing discussions about WebGL in headless mode
gh issue list --search "webgl headless"
gh pr list --search "webgl headless"
Length of output: 239
Script:
#!/bin/bash
# Check the test configuration and browser setup
cat tests/vitest.config.ts
# Look for WebGL-related test files and configurations
rg -g '!node_modules' -g '*.{ts,js}' -i 'webgl|shader.*test'
# Check if there are any existing test results or logs
fd -g '*test-results*' -g '*playwright*' -g '*vitest*'
Length of output: 110488
packages/shader-lab/src/codeGen/GLES100.ts (3)
26-28: LGTM! Correct implementation for WebGL 1.0 MRT support.
The implementation correctly uses gl_FragData array which is available through the EXT_draw_buffers extension (already enabled in extensions list).
56-58:
Implementation appears incomplete
The method returns undefined without any MRT declarations. This could affect shader compilation if MRT structures are used.
Let's check the base implementation and other platform-specific implementations:
✅ Verification successful
Empty implementation is correct for GLES100
The empty implementation in GLES100 is intentional and correct. Here's why:
- The base class
GLESVisitordefines the abstract method that can returnundefined - GLES100 (OpenGL ES 1.0) doesn't support Multiple Render Targets (MRT), so returning
undefinedis the appropriate implementation - In contrast, GLES300 (OpenGL ES 3.0) does implement MRT support with a full implementation
- The base visitor handles undefined returns gracefully by using the nullish coalescing operator (
?? [])
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getMRTDeclare implementations
ast-grep --pattern 'getMRTDeclare() {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Search for getMRTDeclare method with override keyword and class context
ast-grep --pattern 'class $_ {
$$$
override getMRTDeclare(): $_ {
$$$
}
$$$
}'
# Also search for the base class implementation
ast-grep --pattern 'abstract class $_ {
$$$
getMRTDeclare(): $_ {
$$$
}
$$$
}'
Length of output: 185
Script:
#!/bin/bash
# Let's try a simpler search for any mention of getMRTDeclare
rg "getMRTDeclare" -A 3
# Also search for the file to understand the class hierarchy
rg -l "class GLES100"
Length of output: 1444
30-32:
Implementation appears incomplete
The method returns an empty string which suggests incomplete implementation. This could affect MRT property references.
Let's verify if this method is used in the codebase:
✅ Verification successful
Let me gather more context about the GLES100 and GLES300 implementations to understand if the empty return is intentional.
Let me gather more context about the MRT implementation across different GLES versions.
Let me check how MRT is handled in different GLES versions and their implementations.
Based on the context gathered, I can now provide a final response:
Empty implementation is intentional for GLES100
The empty return in GLES100's getReferencedMRTPropText is correct implementation. GLES100 (WebGL 1) handles MRT through gl_FragData array instead of explicit layout locations, while GLES300 (WebGL 2) uses the modern layout-based approach. The empty string return is part of the version-specific implementation pattern.
Key evidence:
- GLES100 handles MRT through
gl_FragData[index]as seen in the implementation - GLES300 properly implements layout-based MRT with
layout(location = ${index}) out vec4 ${ident} - The empty implementation aligns with WebGL 1's MRT approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to getReferencedMRTPropText
rg "getReferencedMRTPropText" -A 2
Length of output: 1381
Script:
#!/bin/bash
# Search for class definitions and inheritance
ast-grep --pattern 'class $_ extends $_'
# Look for any MRT-related comments or documentation
rg -i "mrt" --type ts
Length of output: 131070
Script:
#!/bin/bash
# Look for MRT-related implementations and usages
ast-grep --pattern 'getReferencedMRTPropText($_, $_) { $$$ }'
# Search for MRT struct and related code
rg "struct mrt" -A 5
Length of output: 935
Script:
#!/bin/bash
# Look for MRT-related code in GLES implementations
rg "MRT" -A 3 packages/shader-lab/src/codeGen/GLES*
# Check for any MRT-related tests
rg "MRT" tests/src/shader-lab/ShaderLab.test.ts -A 3
Length of output: 3622
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3)
25-28: LGTM! Well-structured abstract methods for MRT support.The new abstract methods provide a clean extension point for different shader generation contexts to implement their own MRT-specific code generation logic.
Consider adding JSDoc comments to document:
- Expected format of the generated code
- Valid ranges for the index parameter
- Example usage in different contexts (GLSL 100 vs 300)
159-165: LGTM! Consider documenting the declaration filtering behavior.The code correctly filters out varying and MRT struct declarations, but this behavior should be documented to prevent confusion.
Add a comment explaining why these declarations are filtered out and how they are handled elsewhere.
223-225: Consider future extensions for layout qualifier handling.While the current implementation using defaultCodeGen is sufficient for basic cases, you might want to add specific handling for MRT-related layout qualifiers in the future.
Consider:
- Adding validation for MRT-specific layout qualifiers (e.g., location)
- Implementing checks for maximum color attachment constraints
- Adding support for explicit binding points
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/shader-lab/src/codeGen/CodeGenVisitor.ts(4 hunks)packages/shader-lab/src/lexer/Lexer.ts(0 hunks)tests/src/shader-lab/ShaderValidate.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/shader-lab/src/lexer/Lexer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/shader-lab/ShaderValidate.ts
🔇 Additional comments (2)
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (2)
58-70: LGTM! Consistent error handling implementation.
The error handling is well-structured with appropriate conditional compilation for verbose mode.
71-95: Verify gl_FragData usage constraints.
The implementation looks correct, but we should verify that gl_FragData is only used in appropriate shader stages and contexts.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information to verify gl_FragData usage:
gl_FragData usage is correctly constrained and handled
The implementation properly handles gl_FragData with appropriate constraints:
- Correctly declared as fragment shader built-in variable (vec4[]) in
variables.ts - Only used in fragment shader contexts as shown in examples and tests
- Proper MRT handling with index validation and code generation
- Includes WebGL to WebGL2 compatibility handling in ShaderFactory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify gl_FragData usage is only in fragment shaders
# and follows WebGL/GLSL constraints
# Search for gl_FragData usage in shader files
rg --type ts "gl_FragData" -B 5 -A 5
# Look for shader stage declarations near gl_FragData usage
ast-grep --pattern 'class $_ extends $_ {
$$$
gl_FragData
$$$
}'
Length of output: 8397
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/shader-lab/src/codeGen/GLES100.ts (1)
60-74:⚠️ Potential issueImprove error handling and type safety
Several issues need attention:
- Returning an empty string on error could lead to shader compilation issues
- The non-null assertion on
prop.mrtIndexis unsafeConsider this safer implementation:
override visitPostfixExpression(node: ASTNode.PostfixExpression): string { const { children } = node; const postExpr = children[0]; const { context } = VisitorContext; if (postExpr instanceof ASTNode.PostfixExpression && context.isMRTStruct(<string>postExpr.type)) { const propReferenced = children[2] as BaseToken; const prop = context.mrtStruct!.propList.find((item) => item.ident.lexeme === propReferenced.lexeme); if (!prop) { this._reportError(propReferenced.location, `not found mrt property: ${propReferenced.lexeme}`); - return ""; + return "gl_FragData[0]"; // Fallback to first render target } + if (typeof prop.mrtIndex !== 'number') { + this._reportError(propReferenced.location, `invalid mrt index for property: ${propReferenced.lexeme}`); + return "gl_FragData[0]"; // Fallback to first render target + } - return `gl_FragData[${prop.mrtIndex!}]`; + return `gl_FragData[${prop.mrtIndex}]`; } return super.visitPostfixExpression(node); }
🧹 Nitpick comments (4)
packages/shader-lab/src/parser/SemanticAnalyzer.ts (2)
Line range hint
18-23: Enhance documentation to cover MRT functionalityWhile the @internal tag is appropriate, consider expanding the documentation to explain this class's role in Multi-Render Target (MRT) support, particularly how it handles semantic analysis of MRT-related shader constructs.
35-35: Consider keeping error type specific to GSErrorThe errors array type has been broadened from GSError[] to Error[]. Since only GSError instances are being pushed, consider keeping it specific:
- readonly errors: Error[] = []; + readonly errors: GSError[] = [];packages/shader-lab/src/codeGen/GLES300.ts (1)
133-151: Consider extracting duplicated token creation logicThe token creation logic is duplicated between
visitJumpStatementandvisitVariableIdentifier. Consider extracting it into a helper method.+private createFragColorToken(): Token { + const token = Token.pool.get(); + token.set(ETokenType.ID, V3_GL_FragColor, ShaderLab.createPosition(0, 0, 0)); + return token; +}packages/shader-lab/src/parser/AST.ts (1)
1068-1085: LGTM: Comprehensive struct property handling with MRT support.The implementation properly handles both regular struct declarations and MRT struct declarations with layout qualifiers.
Consider adding validation for the layout index value to ensure it's within valid bounds for the target GLES version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/shader-lab/src/codeGen/GLES100.ts(3 hunks)packages/shader-lab/src/codeGen/GLES300.ts(5 hunks)packages/shader-lab/src/codeGen/GLESVisitor.ts(3 hunks)packages/shader-lab/src/codeGen/VisitorContext.ts(5 hunks)packages/shader-lab/src/parser/AST.ts(51 hunks)packages/shader-lab/src/parser/SemanticAnalyzer.ts(4 hunks)
🔇 Additional comments (15)
packages/shader-lab/src/parser/SemanticAnalyzer.ts (1)
73-79: Verify error handling coverage for MRT scenarios
The error reporting has been improved with better typing, but we should ensure it covers all MRT-related validation scenarios.
Let's verify the error handling coverage:
Consider adding specific error messages for MRT validation:
- Invalid layout qualifiers
- Mismatched fragment data indices
- Incompatible MRT configurations
packages/shader-lab/src/codeGen/GLES100.ts (3)
56-58: LGTM: Correct implementation for GLES 1.0
Returning undefined is appropriate as GLES 1.0 doesn't support MRT declarations.
76-82: Verify gl_FragColor usage with MRT
The implementation assigns to gl_FragColor which could conflict with MRT usage. Consider adding a check similar to GLES300's implementation to prevent using both gl_FragColor and gl_FragData.
30-32: Empty string return in getReferencedMRTPropText needs justification
The empty string return might cause issues if the caller expects a valid MRT property text. Consider documenting the rationale or throwing an error if this method shouldn't be called for GLES100.
✅ Verification successful
Let me gather more context about the MRT (Multiple Render Target) implementation to understand if the empty string return is justified.
Let me gather more information about the MRT implementation to understand if the empty string return is intentional.
Let me check the test files and shader implementations to understand how MRT is handled in different GLES versions.
Based on the gathered information, I can now provide the final response:
Empty string return in getReferencedMRTPropText for GLES100 is correct
The empty string return is intentional and correct because:
- In GLES100, MRT (Multiple Render Target) is handled using the built-in
gl_FragDataarray - The implementation correctly uses
gl_FragData[index]ingetFragDataCodeGenmethod - The test files confirm this behavior, showing both direct
gl_FragDatausage and struct-based MRT declarations - GLES300 needs explicit MRT declarations (layout qualifiers), while GLES100 doesn't
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getReferencedMRTPropText is called in the codebase
rg "getReferencedMRTPropText" -A 3
Length of output: 1751
Script:
#!/bin/bash
# Check the base class and inheritance hierarchy
ast-grep --pattern 'class $_ extends $_ {
$$$
}' packages/shader-lab/src/codeGen/GLES100.ts
# Look for MRT-related code and comments
rg -i "mrt|multiple render target" -A 3
Length of output: 75952
Script:
#!/bin/bash
# Check the base class implementation and MRT-related code
ast-grep --pattern 'class GLESVisitor {
$$$
}'
# Look for MRT usage in test files
rg -g "*.test.ts" "mrt" -A 3
Length of output: 1242
Script:
#!/bin/bash
# Check the test shader files content
cat tests/src/shader-lab/shaders/mrt-webgl1-normal.shader tests/src/shader-lab/shaders/mrt-webgl1-struct.shader
# Look for gl_FragData usage which is related to MRT in GLES100
rg "gl_FragData" -A 3
Length of output: 5810
packages/shader-lab/src/codeGen/VisitorContext.ts (3)
27-27: LGTM: Consistent property declarations
The new properties follow the existing patterns and are correctly typed.
Also applies to: 34-34
63-65: LGTM: Safe implementation
The method follows the same pattern as isVaryingStruct and uses safe optional chaining.
97-110: Add test coverage for MRT property handling
While the implementation is correct, it needs test coverage for various scenarios:
- Missing MRT properties
- Invalid MRT references
- Error message formatting
packages/shader-lab/src/codeGen/GLES300.ts (3)
25-31: LGTM: Correct GLES 3.0 implementation
The implementation correctly uses layout qualifiers and follows GLES 3.0 specifications for MRT.
58-76: Consider caching the array to improve performance
The implementation is correct but could be optimized by caching the array as suggested in the past review comments.
override getMRTDeclare(): ICodeSegment[] {
- const ret: ICodeSegment[] = [];
+ const ret: ICodeSegment[] = Array(Object.keys(referencedMRTList).length);
+ let index = 0;
const referencedMRTList = VisitorContext.context._referencedMRTList;
for (let ident in referencedMRTList) {
const info = referencedMRTList[ident];
if (typeof info === "string") {
- ret.push({
+ ret[index++] = {
text: info,
index: Number.MAX_SAFE_INTEGER
});
} else {
- ret.push({
+ ret[index++] = {
text: this.getReferencedMRTPropText(info.mrtIndex, ident),
index: info.ident.location.start.index
});
}
}
return ret;
}Line range hint 113-131: LGTM: Correct handling of gl_FragColor with MRT
The implementation correctly prevents using gl_FragColor with MRT as required.
packages/shader-lab/src/codeGen/GLESVisitor.ts (4)
30-30: LGTM: Well-designed abstract method for MRT declarations.
The abstract method provides a clean extension point for different GLES versions to implement their MRT-specific declarations.
109-113: LGTM: Proper marking of fragment shader return statements.
The flag helps distinguish fragment shader returns for MRT-specific processing.
115-126: LGTM: Comprehensive MRT return type validation.
The validation logic properly handles both traditional vec4 returns and struct-based MRT returns. Error messages are clear and helpful.
Would you like me to help generate test cases that cover:
- Invalid MRT struct scenarios
- Invalid return type scenarios
- Valid MRT struct handling
131-133: LGTM: Clean integration of MRT declarations.
The code properly handles optional MRT declarations and maintains correct ordering in the global scope.
packages/shader-lab/src/parser/AST.ts (1)
97-99: LGTM: Added fragment return statement tracking.
The flag is essential for proper MRT return type validation in fragment shaders.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shader-lab/src/parser/AST.ts (2)
16-28: Good implementation of the decorator pattern for AST nodes.The decorator effectively reduces code duplication and centralizes nonterminal assignment. However, there's a TypeScript type safety concern.
Consider fixing the type safety issue:
- // @ts-ignore - ASTNode.pool = ShaderLabUtils.createObjectPool(ASTNodeClass); + (ASTNode as any).pool = ShaderLabUtils.createObjectPool(ASTNodeClass);
100-117: Consider documenting the purpose of isFragReturnStatement.The implementation correctly tracks return statements, but the purpose of the isFragReturnStatement flag isn't immediately clear.
Add a comment explaining when and why isFragReturnStatement is set to true:
export class JumpStatement extends TreeNode { + /** Indicates if this is a return statement in a fragment shader */ isFragReturnStatement: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-lab/src/parser/AST.ts(51 hunks)
🔇 Additional comments (4)
packages/shader-lab/src/parser/AST.ts (4)
31-32: LGTM: Well-structured base class implementation.
The TreeNode class provides a good foundation with object pooling support and initialization hooks.
Also applies to: 52-52
659-673: LGTM: Comprehensive function return validation.
The implementation properly validates return statements for both void and non-void functions, with clear error messages and proper state cleanup.
1089-1097:
Add validation for layout qualifier index.
The current implementation doesn't validate the index value, which could lead to runtime issues.
Add validation in the semanticAnalyze method:
@ASTNodeDecorator(ENonTerminal.layout_qualifier)
export class LayoutQualifier extends TreeNode {
get index(): number {
return Number((<BaseToken>this.children[4]).lexeme);
}
+
+ override semanticAnalyze(sa: SematicAnalyzer): void {
+ const indexToken = this.children[4] as BaseToken;
+ const index = Number(indexToken.lexeme);
+ if (isNaN(index) || index < 0) {
+ sa.reportError(indexToken.location, `Invalid layout qualifier index: ${indexToken.lexeme}`);
+ }
+ }
override codeGen(visitor: CodeGenVisitor): string {
return visitor.visitLayoutQualifier(this);
}
}Likely invalid or redundant comment.
1066-1083: Well-structured implementation of MRT support in struct declarations.
The code handles both regular struct members and MRT outputs cleanly, with proper type preservation and layout handling.
Let's verify the MRT layout index usage:
✅ Verification successful
Based on the search results, I can verify the MRT (Multiple Render Target) implementation. The code shows:
- The LayoutQualifier class properly extracts index values from layout qualifiers
- The MRT support is used consistently across the codebase:
- GLES300.ts implements MRT output declarations with proper layout locations
- CodeGenVisitor tracks MRT references and generates appropriate declarations
- The struct declaration properly handles both regular members and MRT outputs with layout indices
MRT layout index handling is implemented correctly and used consistently throughout the codebase.
The implementation properly preserves layout indices from struct declarations through to the final GLSL output, ensuring correct MRT binding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent MRT layout index handling
ast-grep --pattern 'layout_qualifier {
$$$
index() {
$$$
}
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Let's try a different approach to find layout qualifier and index handling
rg -A 5 "class LayoutQualifier"
# Also search for layout index usage
rg -A 3 "layout.*index"
# And check struct prop creation with layout
rg -A 3 "new StructProp.*layout"
Length of output: 1117
Script:
#!/bin/bash
# Let's check the complete implementation of StructProp and its usage
ast-grep --pattern 'class StructProp {
$$$
}'
# Also check for MRT-related code
rg -A 5 "MRT" --type ts
Length of output: 82069
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/shader-lab/src/ShaderLabUtils.ts (1)
34-35: 🛠️ Refactor suggestionEnhance error logging in non-verbose mode
The current implementation loses important error details in non-verbose mode, making debugging more difficult in production environments.
Consider enhancing the error logging:
- Logger.error(message); + Logger.error(`[${errorName}] ${message} at ${location} in ${file || 'unknown file'}`);Also, consider maintaining consistency between verbose and non-verbose modes by always returning an Error object:
- Logger.error(message); + const error = new Error(message); + Logger.error(`[${errorName}] ${message} at ${location} in ${file || 'unknown file'}`); + return error;🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
🧹 Nitpick comments (4)
packages/shader-lab/src/parser/AST.ts (2)
100-117: Add documentation for the isFragReturnStatement field.The new field
isFragReturnStatementappears to be related to fragment shader return statements, but its purpose and usage are not documented.Consider adding JSDoc comments:
@ASTNodeDecorator(ENonTerminal.jump_statement) export class JumpStatement extends TreeNode { + /** Indicates if this is a return statement in a fragment shader */ isFragReturnStatement: boolean; override init(): void { this.isFragReturnStatement = false; }
659-673: Improve error messages in function return validation.The error messages for function return validation could be more descriptive and helpful.
Consider these improvements:
if (header.returnType.type === EKeyword.VOID) { if (returnStatement) { - sa.reportError(header.returnType.location, "Return in void function."); + sa.reportError(header.returnType.location, + `Function declared as void cannot have a return value at ${returnStatement.location}`); } } else { if (!returnStatement) { - sa.reportError(header.returnType.location, `No return statement found.`); + sa.reportError(header.returnType.location, + `Function must return a value of type ${header.returnType.type}`); } else { this._returnStatement = returnStatement; } }packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
71-78: Consider enhancing error messages for MRT struct validation.The MRT struct handling follows the established pattern and integrates well with the existing error reporting system.
Consider adding more descriptive error messages in
referenceMRTPropto help developers quickly identify MRT-specific issues. For example:
- Invalid MRT property access
- Unsupported MRT operations
- Type mismatches in MRT assignments
packages/shader-lab/src/ShaderLabUtils.ts (1)
1-1: Consider documenting the behavioral change in error handlingThe change from returning
ErrortoError | undefinedand the addition of Logger represents a significant behavioral change in error handling. This could impact downstream error handling and logging strategies.Consider:
- Adding JSDoc comments to document this behavioral change
- Updating the README or API documentation
- Adding migration notes if this is a breaking change
Also applies to: 30-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/shader-lab/src/ShaderLabUtils.ts(2 hunks)packages/shader-lab/src/codeGen/CodeGenVisitor.ts(4 hunks)packages/shader-lab/src/common/BaseScanner.ts(1 hunks)packages/shader-lab/src/parser/AST.ts(51 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shader-lab/src/ShaderLabUtils.ts
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (7)
packages/shader-lab/src/parser/AST.ts (3)
Line range hint 1-1185: Verify MRT implementation compatibility.
The MRT implementation needs verification against different GLSL versions and hardware capabilities.
✅ Verification successful
Based on the shell script results, I can now provide a final verification response:
MRT implementation is correctly structured with proper version handling
The implementation properly handles MRT across different GLSL versions:
- WebGL 1.0 (#version 100) uses gl_FragData[] syntax
- WebGL 2.0 (#version 300 es) uses layout(location=N) syntax
- Layout qualifiers are correctly implemented in AST.ts for parsing MRT outputs
- Version-specific code generators (GLES100.ts, GLES300.ts) handle the appropriate syntax
The AST implementation in the reviewed file correctly supports both approaches through:
- LayoutQualifier node for WebGL 2.0 syntax
- Proper type system and semantic analysis for output variables
- Version-aware code generation visitors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for MRT usage and compatibility
echo "Checking MRT usage patterns..."
# Find all layout qualifier usage
echo "Layout qualifier usage:"
rg -A 2 "layout.*location.*="
# Find potential MRT output declarations
echo "MRT output declarations:"
rg -A 2 "gl_FragData\["
# Check for version declarations that support MRT
echo "GLSL version declarations:"
rg "#version.*"
Length of output: 4243
1089-1094:
Add validation for layout qualifier index.
The layout qualifier index is parsed but not validated. For MRT support, the index should be validated against hardware limits.
Add validation in the semantic analysis:
@ASTNodeDecorator(ENonTerminal.layout_qualifier)
export class LayoutQualifier extends TreeNode {
get index(): number {
- return Number((<BaseToken>this.children[4]).lexeme);
+ const indexToken = <BaseToken>this.children[4];
+ const index = Number(indexToken.lexeme);
+ if (isNaN(index) || index < 0 || index >= 8) { // Most GPUs support up to 8 MRT
+ throw new Error(`Invalid MRT index: ${indexToken.lexeme}`);
+ }
+ return index;
}
}Likely invalid or redundant comment.
16-28: 🛠️ Refactor suggestion
Consider improving type safety in the decorator implementation.
The decorator implementation is good for reducing boilerplate, but there are some type safety concerns:
- The type parameter
Tcould be constrained to ensure it extendsTreeNode - The
@ts-ignoresuppression should be addressed
Consider this safer implementation:
-function ASTNodeDecorator(nonterminal: ENonTerminal) {
- return function <T extends { new (...args: any[]): TreeNode }>(ASTNode: T) {
+function ASTNodeDecorator(nonterminal: ENonTerminal) {
+ return function <T extends { new (...args: any[]): TreeNode; pool?: ClearableObjectPool<TreeNode> }>(ASTNode: T) {
const ASTNodeClass = class extends ASTNode {
override set(loc: ShaderRange, children: NodeChild[]): void {
super.set(loc, children, nonterminal);
super.init();
}
};
- // @ts-ignore
- ASTNode.pool = ShaderLabUtils.createObjectPool(ASTNodeClass);
+ ASTNode.pool = ShaderLabUtils.createObjectPool(ASTNodeClass as any);
return ASTNodeClass;
};
}Likely invalid or redundant comment.
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3)
85-95: LGTM! Well-structured gl_FragData handling.
The implementation correctly handles gl_FragData array access and maintains the MRT reference list in the context.
162-164:
Fix: Local MRT struct declarations are incorrectly ignored.
The current implementation ignores all MRT struct declarations, including local variables, which could lead to incorrect code generation. This issue was previously identified in a review comment.
Consider this fix:
- if (context.isVaryingStruct(typeLexeme) || context.isMRTStruct(typeLexeme)) return "";
+ if (context.isVaryingStruct(typeLexeme)) return "";
+ if (context.isMRTStruct(typeLexeme)) {
+ // Only ignore global MRT struct declarations
+ const isGlobalScope = !context._curFn;
+ return isGlobalScope ? "" : this.defaultCodeGen(node.children);
+ }This change:
- Preserves the existing behavior for varying structs
- Only ignores MRT struct declarations in global scope
- Properly generates code for local MRT struct variables
Likely invalid or redundant comment.
25-27: LGTM! Verify implementations in subclasses.
The abstract methods provide a clear contract for MRT code generation. The design choice to make these abstract ensures that all concrete visitor implementations must handle MRT appropriately.
Let's verify the implementations in subclasses:
✅ Verification successful
Abstract MRT methods are properly implemented in concrete classes
The abstract methods are correctly implemented in two concrete classes with version-specific MRT handling:
-
GLES100:
getFragDataCodeGen: Returnsgl_FragData[index](WebGL 1.0 style)getReferencedMRTPropText: Returns empty string (no declarations needed in GLES 1.0)
-
GLES300:
getFragDataCodeGen: Returns${V3_GL_FragData}_${index}(WebGL 2.0 style)getReferencedMRTPropText: Returns layout declarationlayout(location = ${index}) out vec4 ${ident};
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all concrete visitors implement the abstract methods
# Search for concrete visitor classes implementing these methods
ast-grep --pattern 'class $_ extends CodeGenVisitor {
$$$
getFragDataCodeGen($_) {
$$$
}
getReferencedMRTPropText($_) {
$$$
}
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find all files that might contain CodeGenVisitor implementations
echo "=== Files containing 'extends CodeGenVisitor' ==="
rg "extends\s+CodeGenVisitor" -l
echo -e "\n=== Implementations of getFragDataCodeGen ==="
rg "getFragDataCodeGen\s*\(" -A 5
echo -e "\n=== Implementations of getReferencedMRTPropText ==="
rg "getReferencedMRTPropText\s*\(" -A 5
Length of output: 5087
packages/shader-lab/src/ShaderLabUtils.ts (1)
30-36: Verify error handling in dependent code
The change in return type from Error to Error | undefined requires verification of how dependent code handles the undefined case.
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/shader-lab/src/codeGen/GLES100.ts (1)
56-70:⚠️ Potential issueImprove error handling in MRT property access
The current error handling could lead to shader compilation errors. Consider adding proper fallbacks and null checks.
override visitPostfixExpression(node: ASTNode.PostfixExpression): string { const { children } = node; const postExpr = children[0]; const { context } = VisitorContext; if (postExpr instanceof ASTNode.PostfixExpression && context.isMRTStruct(<string>postExpr.type)) { const propReferenced = children[2] as BaseToken; const prop = context.mrtStruct!.propList.find((item) => item.ident.lexeme === propReferenced.lexeme); if (!prop) { this._reportError(propReferenced.location, `not found mrt property: ${propReferenced.lexeme}`); - return ""; + return "gl_FragData[0]"; // Fallback to first render target } + if (typeof prop.mrtIndex !== 'number') { + this._reportError(propReferenced.location, `invalid mrt index for property: ${propReferenced.lexeme}`); + return "gl_FragData[0]"; // Fallback to first render target + } return `gl_FragData[${prop.mrtIndex!}]`; } return super.visitPostfixExpression(node); }
🧹 Nitpick comments (2)
packages/shader-lab/src/codeGen/GLES100.ts (2)
26-28: Consider adding index validationThe method should validate that the index is within valid bounds for gl_FragData array (typically 0-3 in GLSL ES 1.00 with GL_EXT_draw_buffers).
override getFragDataCodeGen(index: string | number): string { + const maxDrawBuffers = 4; // GL_MAX_DRAW_BUFFERS_EXT is typically 4 + const idx = typeof index === 'string' ? parseInt(index) : index; + if (idx < 0 || idx >= maxDrawBuffers) { + this._reportError(null, `Invalid gl_FragData index: ${index}. Must be between 0 and ${maxDrawBuffers - 1}`); + return `gl_FragData[0]`; // Fallback to first buffer + } return `gl_FragData[${index}]`; }
52-54: Consider adding documentation for empty implementationIf the empty implementation is intentional (since gl_FragData is built-in in GLSL ES 1.00), please add a comment explaining why no declarations are needed.
override getMRTDeclare(out: ICodeSegment[]): void { + // No declarations needed for MRT in GLSL ES 1.00 as gl_FragData is built-in return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/shader-lab/src/codeGen/GLES100.ts(2 hunks)packages/shader-lab/src/codeGen/GLES300.ts(4 hunks)packages/shader-lab/src/codeGen/GLESVisitor.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shader-lab/src/codeGen/GLESVisitor.ts
- packages/shader-lab/src/codeGen/GLES300.ts
🔇 Additional comments (3)
packages/shader-lab/src/codeGen/GLES100.ts (3)
34-42: LGTM! Clean implementation of attribute declarations
The method correctly generates GLSL ES 1.00 attribute declarations with proper source location tracking.
43-50: LGTM! Clean implementation of varying declarations
The method correctly generates GLSL ES 1.00 varying declarations with proper source location tracking.
30-32: Implementation appears incomplete
The method returns an empty string but seems intended to generate text for referenced MRT properties based on its name and parameters. Please clarify if this is intentional or needs implementation.
✅ Verification successful
Based on the search results, I can now provide a final response:
Empty implementation is intentional for GLES100
The empty implementation in GLES100.ts is intentional as GLES100 (OpenGL ES 1.0) doesn't support Multiple Render Targets (MRT). This is confirmed by:
- The base class
CodeGenVisitordefines this as an abstract method that each implementation must provide - GLES300.ts implements it with proper MRT layout declarations:
layout(location = ${index}) out vec4 ${ident}; - GLES100.ts intentionally returns empty string as MRT is not supported in this version
- The corresponding
getMRTDeclaremethod in GLES100 is also empty for the same reason
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this method is implemented differently in other visitors
ast-grep --pattern 'getReferencedMRTPropText($_$, $_$) { $$$$ }'
Length of output: 66
Script:
#!/bin/bash
# Let's try a different approach to find implementations and usages
# Find files containing 'getReferencedMRTPropText'
echo "=== Files containing getReferencedMRTPropText ==="
rg "getReferencedMRTPropText" -l
# Look for the base class or interface definition
echo -e "\n=== Context around getReferencedMRTPropText definition ==="
rg "getReferencedMRTPropText" -B 5 -A 5
# Look for any MRT-related code to understand the context
echo -e "\n=== MRT-related code ==="
rg "MRT" -B 2 -A 2
Length of output: 85401
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/shader-lab/src/codeGen/GLES300.ts (2)
52-67: Consider using a more predictable indexing strategy.Using
Number.MAX_SAFE_INTEGERas the index for string-based MRT info might cause unexpected ordering of declarations. Consider using a base index (e.g., 1000000) plus a counter to maintain consistent ordering while still ensuring these declarations appear after object-based ones.- index: Number.MAX_SAFE_INTEGER + index: 1000000 + Object.keys(referencedMRTList).indexOf(ident)
130-151: Consider extracting the gl_FragColor reference tracking logic.The gl_FragColor reference tracking code is duplicated between
visitJumpStatementandvisitVariableIdentifier. Consider extracting this into a helper method to improve maintainability.+ private ensureGLFragColorReference(node: ASTNode.Node): void { + const { _referencedVaryingList } = VisitorContext.context; + if (!_referencedVaryingList[V3_GL_FragColor]) { + const token = Token.pool.get(); + token.set(ETokenType.ID, V3_GL_FragColor, ShaderLab.createPosition(0, 0, 0)); + _referencedVaryingList[V3_GL_FragColor] = { + ident: token, + typeInfo: new SymbolType(EKeyword.VEC4, "vec4"), + qualifier: "out", + astNode: node + }; + } + } override visitJumpStatement(node: ASTNode.JumpStatement): string { if (node.isFragReturnStatement) { const { _referencedVaryingList, mrtStruct } = VisitorContext.context; if (mrtStruct) { return ""; } - if (!_referencedVaryingList[V3_GL_FragColor]) { - const token = Token.pool.get(); - token.set(ETokenType.ID, V3_GL_FragColor, ShaderLab.createPosition(0, 0, 0)); - _referencedVaryingList[V3_GL_FragColor] = { - ident: token, - typeInfo: new SymbolType(EKeyword.VEC4, "vec4"), - qualifier: "out", - astNode: node - }; - } + this.ensureGLFragColorReference(node);packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
Line range hint
50-103: Consider decomposing the postfix expression visitor.The method is handling multiple concerns (struct member access, array access, gl_FragData). Consider extracting the gl_FragData handling into a separate method for better maintainability.
+ private handleGLFragData(identNode: ASTNode.PostfixExpression, indexNode: ASTNode.Expression): string { + const context = VisitorContext.context; + const identLexeme = identNode.codeGen(this); + const indexLexeme = indexNode.codeGen(this); + + if (identLexeme === "gl_FragData") { + // #if _VERBOSE + if (context._referencedVaryingList[V3_GL_FragColor]) { + this._reportError(identNode.location, "cannot use both gl_FragData and gl_FragColor"); + } + // #endif + const mrtLexeme = this.getFragDataCodeGen(indexLexeme); + context._referencedMRTList[mrtLexeme] = this.getReferencedMRTPropText(indexLexeme, mrtLexeme); + return mrtLexeme; + } + return `${identLexeme}[${indexLexeme}]`; + } visitPostfixExpression(node: ASTNode.PostfixExpression) { // ... existing code ... } else if (derivationLength === 4) { - const identNode = node.children[0] as ASTNode.PostfixExpression; - const indexNode = node.children[2] as ASTNode.Expression; - const identLexeme = identNode.codeGen(this); - const indexLexeme = indexNode.codeGen(this); - if (identLexeme === "gl_FragData") { - // #if _VERBOSE - if (context._referencedVaryingList[V3_GL_FragColor]) { - this._reportError(identNode.location, "cannot use both gl_FragData and gl_FragColor"); - } - // #endif - const mrtLexeme = this.getFragDataCodeGen(indexLexeme); - context._referencedMRTList[mrtLexeme] = this.getReferencedMRTPropText(indexLexeme, mrtLexeme); - return mrtLexeme; - } - return `${identLexeme}[${indexLexeme}]`; + return this.handleGLFragData( + node.children[0] as ASTNode.PostfixExpression, + node.children[2] as ASTNode.Expression + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/src/shader-lab/shaders/mrt-error1.shaderis excluded by!**/*.shader
📒 Files selected for processing (5)
e2e/case/shaderLab-mrt.ts(1 hunks)packages/shader-lab/src/codeGen/CodeGenVisitor.ts(5 hunks)packages/shader-lab/src/codeGen/GLES300.ts(4 hunks)packages/shader-lab/src/codeGen/GLESVisitor.ts(4 hunks)tests/src/shader-lab/ShaderLab.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/case/shaderLab-mrt.ts
- tests/src/shader-lab/ShaderLab.test.ts
- packages/shader-lab/src/codeGen/GLESVisitor.ts
🔇 Additional comments (4)
packages/shader-lab/src/codeGen/GLES300.ts (3)
23-29: LGTM! Good use of prefixed identifiers.
The fragment data code generation methods are well-implemented. The use of prefixed identifiers (GS_glFragData_) helps avoid naming conflicts with user-defined variables.
31-50: LGTM! Good performance optimization.
The refactored methods now avoid unnecessary array allocations by using an output parameter. This is more memory-efficient, especially when processing large shaders.
Line range hint 105-128: LGTM! Good implementation of gl_FragColor restrictions.
The implementation correctly enforces the restrictions on mixing gl_FragColor with MRT and gl_FragData, providing clear error messages. The reference tracking is properly handled.
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
Line range hint 17-30: LGTM! Good use of constants and abstract methods.
The implementation uses constants to avoid string literals and enforces MRT support implementation in derived classes through abstract methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shader-lab/src/codeGen/GLES300.ts (2)
52-67: Consider adding type safety for MRT declarations.The
referencedMRTListhandling could benefit from stronger typing to make the code more maintainable and prevent potential runtime errors.Consider defining an interface for the MRT info:
interface MRTInfo { mrtIndex: number | string; ident: { location: { start: { index: number } } }; } type MRTListEntry = string | MRTInfo;
135-147: Consider adding error handling for token pool exhaustion.The
_registerFragColorVariablemethod uses token pooling but doesn't handle the case where the pool might be exhausted.Consider adding a check:
const token = Token.pool.get(); if (!token) { throw new Error('Token pool exhausted while registering gl_FragColor'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-lab/src/codeGen/GLES300.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/codeGen/GLES300.ts
[warning] 127-130: packages/shader-lab/src/codeGen/GLES300.ts#L127-L130
Added lines #L127 - L130 were not covered by tests
🔇 Additional comments (2)
packages/shader-lab/src/codeGen/GLES300.ts (2)
105-119: LGTM: Proper validation of gl_FragColor usage.
The implementation correctly enforces:
- Mutual exclusivity between gl_FragColor and gl_FragData
- MRT compatibility constraints
This aligns with the requirements mentioned in previous reviews.
121-133: Add test coverage for fragment return statement handling.
The new visitJumpStatement implementation lacks test coverage, particularly for:
- Fragment return statements with MRT struct
- Fragment return statements without MRT struct
Let's verify existing test coverage:
Also, consider adding a comment explaining why an empty string is returned when mrtStruct is present.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 127-130: packages/shader-lab/src/codeGen/GLES300.ts#L127-L130
Added lines #L127 - L130 were not covered by tests
| } | ||
|
|
||
| export type NodeChild = TreeNode | BaseToken; | ||
|
|
||
| export type IParamInfo = { ident: BaseToken; typeInfo: SymbolType; astNode: TreeNode }; | ||
| export type IParamInfo = { ident: BaseToken; typeInfo: SymbolType; astNode?: TreeNode }; |
| super.init(); | ||
| } | ||
| }; | ||
| // @ts-ignore |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/shader-lab/src/parser/AST.ts (3)
1070-1087: Refactor struct declaration property list handling.The property list generation logic mixes layout qualifier handling with regular struct declarations. Consider extracting this into separate methods:
+ private createPropWithLayout( + typeSpecifier: TypeSpecifier, + declarator: StructDeclarator, + layoutQualifier: LayoutQualifier + ): StructProp { + const typeInfo = new SymbolType(typeSpecifier.type, typeSpecifier.lexeme); + return new StructProp(typeInfo, declarator.ident, layoutQualifier.index); + } + + private createPropWithoutLayout( + typeSpecifier: TypeSpecifier, + declarator: StructDeclarator + ): StructProp { + const typeInfo = new SymbolType( + typeSpecifier.type, + typeSpecifier.lexeme, + declarator.arraySpecifier + ); + return new StructProp(typeInfo, declarator.ident); + } get propList(): StructProp[] { const ret: StructProp[] = []; const firstChild = this.children[0]; if (firstChild instanceof LayoutQualifier) { - const typeSpecifier = this.children[1] as TypeSpecifier; - const declarator = this.children[2] as StructDeclarator; - const typeInfo = new SymbolType(typeSpecifier.type, typeSpecifier.lexeme); - const prop = new StructProp(typeInfo, declarator.ident, firstChild.index); + const prop = this.createPropWithLayout( + this.children[1] as TypeSpecifier, + this.children[2] as StructDeclarator, + firstChild + ); ret.push(prop); } else { for (const declarator of this.declaratorList.declaratorList) { - const typeInfo = new SymbolType( - this.typeSpecifier.type, - this.typeSpecifier.lexeme, - declarator.arraySpecifier - ); - const prop = new StructProp(typeInfo, declarator.ident); + const prop = this.createPropWithoutLayout(this.typeSpecifier, declarator); ret.push(prop); } } return ret; }
Line range hint
640-677: Enhance error messages for function return validation.The error messages for return statement validation could be more descriptive to help developers understand and fix issues:
if (returnStatement) { - sa.reportError(header.returnType.location, "Return in void function."); + sa.reportError( + header.returnType.location, + `Function '${header.ident.lexeme}' is void but contains a return statement.` + ); } } else { if (!returnStatement) { - sa.reportError(header.returnType.location, `No return statement found.`); + sa.reportError( + header.returnType.location, + `Function '${header.ident.lexeme}' must return a value of type '${header.returnType.type}'.` + ); }
370-373: Enhance error messages with more context.Error messages throughout the semantic analysis could be more descriptive:
- sa.reportError(id.location, `Undeclared symbol: ${id.lexeme}`); + sa.reportError(id.location, `Undeclared symbol '${id.lexeme}' used in integer constant expression`); - sa.reportError(id.location, "Invalid integer."); + sa.reportError(id.location, `Symbol '${id.lexeme}' cannot be used as an integer constant`); - sa.reportError(this.location, `No overload function type found: ${functionIdentifier.ident}`); + sa.reportError( + this.location, + `No matching overload found for function '${functionIdentifier.ident}' with the given parameter types` + ); - sa.reportError(this.location, `undeclared identifier: ${token.lexeme}`); + sa.reportError(this.location, `Use of undeclared identifier '${token.lexeme}'`);Also applies to: 729-729, 1171-1171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/shader-lab/src/parser/AST.ts(51 hunks)packages/shader-lab/src/parser/SemanticAnalyzer.ts(4 hunks)
🔇 Additional comments (4)
packages/shader-lab/src/parser/SemanticAnalyzer.ts (2)
73-79: LGTM! Clear and well-structured error reporting.
The rename to reportError and the signature change improve clarity and type safety. The implementation properly handles both verbose and non-verbose modes.
27-30: 🛠️ Refactor suggestion
Initialize curFunctionInfo properties explicitly.
The empty object initialization could lead to type safety issues. Consider initializing the properties explicitly:
- curFunctionInfo: {
- header?: ASTNode.FunctionDeclarator;
- returnStatement?: ASTNode.JumpStatement;
- } = {};
+ curFunctionInfo: {
+ header?: ASTNode.FunctionDeclarator;
+ returnStatement?: ASTNode.JumpStatement;
+ } = {
+ header: undefined,
+ returnStatement: undefined
+ };Likely invalid or redundant comment.
packages/shader-lab/src/parser/AST.ts (2)
16-28:
Remove ts-ignore by properly typing the pool property.
The @ts-ignore comment suppresses type checking, which could hide potential issues. Consider adding proper type definitions:
- // @ts-ignore
- ASTNode.pool = ShaderLabUtils.createObjectPool(ASTNodeClass);
+ (ASTNode as { pool?: ClearableObjectPool<TreeNode> }).pool = ShaderLabUtils.createObjectPool(ASTNodeClass);Likely invalid or redundant comment.
1093-1098:
Add validation for layout qualifier index.
The index value is parsed from the token lexeme without validation. Consider adding checks for:
- Valid number format
- Non-negative value
- Maximum allowed MRT outputs
@ASTNodeDecorator(ENonTerminal.layout_qualifier)
export class LayoutQualifier extends TreeNode {
get index(): number {
- return Number((<BaseToken>this.children[4]).lexeme);
+ const token = <BaseToken>this.children[4];
+ const value = Number(token.lexeme);
+ if (isNaN(value) || value < 0) {
+ throw new Error(`Invalid layout qualifier index: ${token.lexeme}`);
+ }
+ return value;
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/shader-lab/src/parser/AST.ts (4)
7-7: Consider unifying imports instead of aliasing BaseToken twice.
Currently, the line 'import { BaseToken, BaseToken as Token } from "../common/BaseToken";' introduces a redundant alias for the same symbol. If there’s no specific reason for the alias, consider using a single import reference for clarity.
343-343: Offer handling for unimplemented operators.
The error message “not implemented operator” suggests some operators remain unsupported. If additional operators need to be supported, consider implementing them or clarifying why they are intentionally unsupported.Would you like assistance generating a patch to handle the missing operators?
860-860: Initialize this.type in all branches.
In PostfixExpression’s init() method, “this.type” is only set if “this.children.length === 1.” If there’s another branch (e.g., children.length > 1) that’s valid, consider initializing “this.type” to a default or extended value to avoid potential undefined type scenarios.
661-668: Ensure error messages remain consistent.
Within this block, the code reports different errors such as “No return statement found.” or “Return in void function.” Although it is correct logic, consider harmonizing the style and phrasing to maintain a uniform user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-lab/src/parser/AST.ts(50 hunks)
🔇 Additional comments (3)
packages/shader-lab/src/parser/AST.ts (3)
24-26: Avoid suppressing TypeScript errors with @ts-ignore.
A past comment addressed the same issue. Whenever possible, address the underlying type problem rather than using @ts-ignore, to avoid obscuring potential bugs.
1091-1092: Add validation for layout qualifier index.
A previous review comment also suggested validation for the layout index to ensure it’s a valid non-negative number and within the maximum MRT outputs allowed. Currently, the code simply converts the token to a number without any additional checks, which could introduce runtime errors or misconfiguration if the index is invalid.
1091-1092: Check for potential out-of-range child index usage.
The code references “this.children[4]” without verifying that “children.length > 4.” If the child array is shorter than expected, this could cause a runtime error. Verify that this node’s grammar rules guarantee at least five children.
✅ Verification successful
The previous pattern didn't work as it might not match the exact syntax. Let's try to understand the grammar rules and class structure first.
Based on the output, I can now generate the final response as we have found the grammar definition in TargetParser.y and the corresponding implementation in CFG.ts. The grammar rule shows that a layout_qualifier must have the pattern layout '(' location '=' INT_CONSTANT ')', and the CFG implementation confirms this structure with [EKeyword.LAYOUT, ETokenType.LEFT_PAREN, EKeyword.LOCATION, ...]. This guarantees that index 4 will always contain the INT_CONSTANT token.
The array access is safe as per grammar rules
The layout_qualifier grammar rule enforces exactly 6 tokens in a specific order: 'layout', '(', 'location', '=', INT_CONSTANT, and ')'. Therefore, accessing children[4] is guaranteed to be safe as it will always contain the INT_CONSTANT token.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to layout_qualifier usage in the codebase to verify grammar rules:
ast-grep --pattern $'layout_qualifier: LAYOUT_LEFT_PAREN layout_qualifier_id EQUAL int_constant_expression LAYOUT_RIGHT_PAREN ;'
Length of output: 129
Script:
#!/bin/bash
# Let's first find the grammar file that defines the layout_qualifier rule
fd "grammar" --type f
# Then let's look for the class definition containing this index getter
ast-grep --pattern 'class $_ {
$$$
get index(): number {
$$$
}
$$$
}'
# Also search for any test files related to layout qualifiers
fd "layout.*test" --type f
Length of output: 224
Script:
#!/bin/bash
# Let's examine the grammar file content to find layout qualifier rules
cat packages/shader-lab/src/parser/Grammar.ts
# Also let's look for any test cases that might show the structure
rg -A 5 "layout.*qualifier"
Length of output: 3375
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/shader-lab/src/parser/AST.ts (2)
31-32: Add JSDoc documentation for the TreeNode class.The TreeNode class is a fundamental part of the AST implementation. Consider adding comprehensive JSDoc documentation to explain:
- The purpose of the class
- The object pooling mechanism
- The relationship with IPoolElement
106-109: Initialize class properties in the constructor.The
isFragReturnStatementproperty should be initialized in the constructor rather than in theinit()method to ensure it's always properly initialized when a new instance is created.- isFragReturnStatement: boolean; + isFragReturnStatement: boolean = false; - override init(): void { - this.isFragReturnStatement = false; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-lab/src/parser/AST.ts(50 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/parser/AST.ts
[warning] 342-342: packages/shader-lab/src/parser/AST.ts#L342
Added line #L342 was not covered by tests
[warning] 365-365: packages/shader-lab/src/parser/AST.ts#L365
Added line #L365 was not covered by tests
[warning] 368-368: packages/shader-lab/src/parser/AST.ts#L368
Added line #L368 was not covered by tests
[warning] 433-433: packages/shader-lab/src/parser/AST.ts#L433
Added line #L433 was not covered by tests
[warning] 662-663: packages/shader-lab/src/parser/AST.ts#L662-L663
Added lines #L662 - L663 were not covered by tests
[warning] 666-667: packages/shader-lab/src/parser/AST.ts#L666-L667
Added lines #L666 - L667 were not covered by tests
🔇 Additional comments (2)
packages/shader-lab/src/parser/AST.ts (2)
24-26: Remove TypeScript error suppression.
The // @ts-ignore comment suppresses TypeScript's type checking, which could hide potential issues. Consider adding proper type definitions instead.
1092-1097:
Add validation for layout qualifier index.
The layout qualifier index should be validated to ensure it's within valid bounds for MRT (Multi-Render Target).
get index(): number {
- return Number((<BaseToken>this.children[4]).lexeme);
+ const indexToken = <BaseToken>this.children[4];
+ const index = Number(indexToken.lexeme);
+ if (isNaN(index) || index < 0 || index >= 8) { // Most GPUs support up to 8 MRT outputs
+ throw new Error(`Invalid layout qualifier index: ${indexToken.lexeme}`);
+ }
+ return index;
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/shader-lab/src/ParserUtils.ts (1)
38-38: Test coverage gap for the 'isTerminal' check
Line 38 is not covered by tests according to static analysis. To increase coverage, consider adding a scenario that invokes this condition.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-38: packages/shader-lab/src/ParserUtils.ts#L38
Added line #L38 was not covered by testspackages/shader-lab/src/parser/types.ts (1)
9-13: Constructor refactor to public parameter properties in SymbolType.This is a concise way to define and initialize class properties. Confirm that external references to members of SymbolType haven’t been broken by the refactor or changed param order.
packages/shader-lab/src/lalr/Utils.ts (1)
73-73: Stringifying the production using NoneTerminal.This is consistent with the new type. No issues, but ensure all derived code that logs or prints grammar productions is accounted for in test coverage, particularly line 20’s usage.
packages/shader-lab/src/parser/GrammarSymbol.ts (1)
5-5: Consider keeping the 'E' prefix for enum naming.The rename from
ENonTerminaltoNoneTerminaldeviates from TypeScript naming conventions where enums typically start with 'E'. Consider keeping the original name or providing justification for the change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/shader-lab/src/ParserUtils.ts(3 hunks)packages/shader-lab/src/codeGen/CodeGenVisitor.ts(7 hunks)packages/shader-lab/src/lalr/CFG.ts(7 hunks)packages/shader-lab/src/lalr/LALR1.ts(8 hunks)packages/shader-lab/src/lalr/Production.ts(1 hunks)packages/shader-lab/src/lalr/StateItem.ts(2 hunks)packages/shader-lab/src/lalr/Utils.ts(3 hunks)packages/shader-lab/src/lalr/types.ts(1 hunks)packages/shader-lab/src/parser/AST.ts(50 hunks)packages/shader-lab/src/parser/Grammar.ts(1 hunks)packages/shader-lab/src/parser/GrammarSymbol.ts(3 hunks)packages/shader-lab/src/parser/ShaderTargetParser.ts(2 hunks)packages/shader-lab/src/parser/types.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/ParserUtils.ts
[warning] 38-38: packages/shader-lab/src/ParserUtils.ts#L38
Added line #L38 was not covered by tests
packages/shader-lab/src/codeGen/CodeGenVisitor.ts
[warning] 78-79: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L78-L79
Added lines #L78 - L79 were not covered by tests
packages/shader-lab/src/lalr/Utils.ts
[warning] 20-20: packages/shader-lab/src/lalr/Utils.ts#L20
Added line #L20 was not covered by tests
packages/shader-lab/src/parser/AST.ts
[warning] 342-342: packages/shader-lab/src/parser/AST.ts#L342
Added line #L342 was not covered by tests
[warning] 365-365: packages/shader-lab/src/parser/AST.ts#L365
Added line #L365 was not covered by tests
[warning] 368-368: packages/shader-lab/src/parser/AST.ts#L368
Added line #L368 was not covered by tests
[warning] 433-433: packages/shader-lab/src/parser/AST.ts#L433
Added line #L433 was not covered by tests
[warning] 662-663: packages/shader-lab/src/parser/AST.ts#L662-L663
Added lines #L662 - L663 were not covered by tests
[warning] 666-667: packages/shader-lab/src/parser/AST.ts#L666-L667
Added lines #L666 - L667 were not covered by tests
🪛 Biome (1.9.4)
packages/shader-lab/src/lalr/Utils.ts
[error] 17-17: 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 (39)
packages/shader-lab/src/lalr/types.ts (2)
1-1: Solid rename for consistency.
Renaming from ENonTerminal to NoneTerminal aligns with the introduced naming convention across the codebase. This helps maintain clarity when distinguishing between terminals and "none-terminals" in the grammar.
6-6: Confirm references to GotoTable usage.
All accesses to GotoTable in the rest of the codebase should correctly use NoneTerminal now. A quick verification ensures no references to the old type remain.
✅ Verification successful
No references to ENonTerminal found, and GotoTable is correctly using NoneTerminal.
Based on the search results, there are no references to ENonTerminal in the codebase, and all usages of GotoTable are correctly using the NoneTerminal type. The type definition in packages/shader-lab/src/lalr/types.ts shows:
export type GotoTable = Map<NoneTerminal, number /** state ID */>;And all its usages in the codebase consistently use NoneTerminal. The change is safe and correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checks for any leftover references to ENonTerminal in the codebase.
rg 'ENonTerminal' -A 5
Length of output: 22
Script:
#!/bin/bash
# Let's search for GotoTable usage to verify its correct usage with NoneTerminal
rg "GotoTable" -A 2
# Also search for NoneTerminal to understand its usage
rg "NoneTerminal" -A 2
Length of output: 66115
packages/shader-lab/src/lalr/Production.ts (3)
1-1: Consistent renaming of imports
Renaming from a previous enumeration to "NoneTerminal" helps maintain clarity and consistency. No issues found here.
7-7: Accurate type update for the 'goal' property
The property rename from a previous type to "NoneTerminal" is consistent with the new naming convention. Good work ensuring a clear and descriptive type name.
11-11: Constructor parameters updated for clarity
Using "NoneTerminal" in the constructor parameter aligns well with the property naming. This enhances readability and consistency in the type system.
packages/shader-lab/src/parser/Grammar.ts (1)
2-2: Refactor from ENonTerminal to NoneTerminal
All these lines reflect the transition from "ENonTerminal" to "NoneTerminal." This consistent rename maintains clarity across the grammar definitions. The approach to inject a new start production is also sound.
Also applies to: 8-8, 10-10, 12-12, 17-17, 19-19, 23-23, 27-27
packages/shader-lab/src/ParserUtils.ts (1)
1-1: Refactor from ENonTerminal to NoneTerminal
These lines ensure that the parser utilities consistently reference the new "NoneTerminal" type. The logic remains unchanged and correct.
Also applies to: 10-10, 33-33
packages/shader-lab/src/lalr/StateItem.ts (2)
2-2: Accurate import adjustment
Replacing "ENonTerminal" with "NoneTerminal" in the import preserves consistency across the codebase.
73-73: String representation updated for clarity
Referencing "NoneTerminal" in the item’s string output aligns with the new naming convention. Good job ensuring it matches the rest of the refactor.
packages/shader-lab/src/parser/ShaderTargetParser.ts (2)
2-2: Consistent renaming from ENonTerminal to NoneTerminal.
This import change is part of the broader refactor to replace ENonTerminal with NoneTerminal. Ensure that all references to the old type have been updated in this file and that there are no backward compatibility issues in dependent code.
134-134: Confirm correctness of type assertion.
Casting the stack element to NoneTerminal indicates that you expect only NoneTerminal and tokens in this sequence. Double-check that no additional types are pushed onto this stack.
✅ Verification successful
Let me gather information about the stack usage and types by analyzing the codebase.
Let me gather more information about the types and usage patterns.
Let me check one more thing to understand the NoneTerminal type and its relationship with the stack.
Let me check one final thing to understand how the GrammarSymbol is defined.
Based on the gathered information, I can now provide the final response:
Type assertion to NoneTerminal is correct and safe.
The type assertion is used in a debug print method _printStack where the stack alternates between NoneTerminal (parser states) and BaseToken items, as evidenced by:
TraceStackItemis explicitly defined asNoneTerminal | BaseToken- The stack access pattern uses
i++to skip every other item, correctly matching the alternating pattern - The context shows this is part of a debug trace printing functionality (wrapped in
#if _VERBOSE)
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the file and check the context around the type assertion
rg -A 10 -B 10 "_traceBackStack\[i\+\+\]" packages/shader-lab/src/parser/ShaderTargetParser.ts
# Look for places where items are pushed onto _traceBackStack
ast-grep --pattern 'this._traceBackStack.push($_)'
# Check the type definition and initialization of _traceBackStack
rg "_traceBackStack" packages/shader-lab/src/parser/ShaderTargetParser.ts
Length of output: 1268
Script:
#!/bin/bash
# Check the definition of TraceStackItem and NoneTerminal types
rg "type TraceStackItem|type NoneTerminal|interface TraceStackItem|interface NoneTerminal" packages/shader-lab/src/parser/
# Look for the push operations on _traceBackStack
rg -A 5 -B 5 "this\._traceBackStack\.push" packages/shader-lab/src/parser/ShaderTargetParser.ts
# Check for the definition of BaseToken
rg "class BaseToken|interface BaseToken" packages/shader-lab/src/
Length of output: 505
Script:
#!/bin/bash
# Check the definition and usage of NoneTerminal
rg "NoneTerminal" packages/shader-lab/src/parser/
# Look at the content of types.ts to understand the type hierarchy
cat packages/shader-lab/src/parser/types.ts
Length of output: 9378
Script:
#!/bin/bash
# Check the GrammarSymbol.ts file to understand the NoneTerminal enum
cat packages/shader-lab/src/parser/GrammarSymbol.ts
Length of output: 2579
packages/shader-lab/src/lalr/LALR1.ts (9)
2-2: Consistent refactor from ENonTerminal to NoneTerminal.
This ensures uniform usage of the new type alias throughout the parser. Confirm that any remaining references to ENonTerminal have been removed from the codebase for consistency.
22-22: Update of the _firstSetNTStack type.
The stack now holds NoneTerminal elements. Check that logic pushing/popping to/from this stack aligns with the new type to avoid runtime mismatches.
118-118: START rule comparison changed to NoneTerminal.START.
This matches the updated type. Confirm that the grammar’s starting symbol references the renamed START constant.
184-184: Method signature updated to _computeFirstSetForNT(NT: NoneTerminal).
Ensure that any calls to _computeFirstSetForNT use NoneTerminal. An incorrect usage could lead to runtime errors.
212-217: Use of NoneTerminal in first set computation.
The code properly transitions from ENonTerminal to NoneTerminal in the loop. Confirm related grammar definitions and usage sites to ensure they match the new type references.
147-147: Goto table uses NoneTerminal as the key.
Verify that no prior references to ENonTerminal remain in the table definitions or in the grammar that might lead to incorrect transitions.
✅ Verification successful
Let me search for any references to ENonTerminal in the codebase and check the table definitions.
Based on the search results, I can now generate the final response since I have enough information to verify the concern about NoneTerminal usage in the goto table.
No remaining references to ENonTerminal found in table definitions or grammar.
The codebase consistently uses NoneTerminal enum throughout, particularly in:
- State and goto table definitions in
LALR1.ts - Grammar productions in
CFG.ts - Type definitions in
types.ts
There are no legacy references to ENonTerminal that could cause incorrect transitions. The goto table and all related grammar components are properly using the NoneTerminal type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for ENonTerminal references
rg "ENonTerminal" -A 3
# Search for table definitions and usage of NoneTerminal
rg "NoneTerminal" -A 3
Length of output: 76552
75-75: Use of firstSetMap with NoneTerminal key.
Changing the key to NoneTerminal is correct given the codebase-wide rename. Confirm the map's usage with all relevant grammar rules to ensure consistency.
✅ Verification successful
Let me gather information about the usage of firstSetMap and NoneTerminal in the codebase to verify the consistency.
Based on the search results, I can see that firstSetMap is consistently used with NoneTerminal throughout the codebase, particularly in the LALR1 parser implementation. The map is declared with the correct type Map<NoneTerminal, Set<Terminal>> and all its usages properly cast symbols to NoneTerminal before using them as keys.
The type cast to NoneTerminal is consistent with the codebase's grammar implementation.
- The
firstSetMapis used for computing FIRST sets in LALR(1) parsing, where non-terminals are mapped to their corresponding sets of terminals - The type cast is used consistently across the codebase when accessing the map
- The implementation aligns with the grammar rules defined in CFG.ts where all production rules use
NoneTerminalenum values
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for firstSetMap usage and its type declarations
rg "firstSetMap" -A 2
# Search for NoneTerminal usage in relation to firstSetMap
ast-grep --pattern 'firstSetMap.get(<NoneTerminal>$_)'
# Look for NoneTerminal type definition and usage
rg "NoneTerminal" -A 2
Length of output: 67076
14-15: Refactor Map key type from ENonTerminal to NoneTerminal.
Changing both firstSetMap and followSetMap enhances consistency with the new type rename. Verify that no external references still rely on the old type signature, particularly in grammar construction or usage.
61-61: Casting curSymbol to NoneTerminal.
Ensure that any symbol recognized here is genuinely a non-terminal. If there's a chance of receiving a terminal, consider safer type checks before casting.
✅ Verification successful
Let me gather more information about how curSymbol is used and its type safety.
Let me check for more context about the type safety and validation.
Based on the gathered information, I can now provide a final response as the code shows clear type safety measures:
Type safety for curSymbol is properly implemented
The code is actually safe because:
- There's an explicit check using
GrammarUtils.isTerminal(item.curSymbol)before the cast - The
isTerminalimplementation in Utils.ts uses a numeric comparison (return sm < NoneTerminal.START) to definitively determine the symbol type - The cast to NoneTerminal only happens after confirming it's not a terminal symbol
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's first look at the LALR1.ts file to understand the context
rg -A 5 -B 5 "curSymbol" packages/shader-lab/src/lalr/LALR1.ts
# Let's also check for the Symbol type definitions and usage
ast-grep --pattern 'interface Symbol {
$$$
}'
# Check for NoneTerminal type definition
ast-grep --pattern 'interface NoneTerminal {
$$$
}'
# Look for any type checks or validations before using curSymbol
ast-grep --pattern 'if ($_ instanceof NoneTerminal) {
$$$
}'
Length of output: 953
Script:
#!/bin/bash
# Check the GrammarUtils.isTerminal implementation
rg -A 5 "isTerminal" packages/shader-lab/src/lalr/
# Look for the StateItem class definition and curSymbol property
ast-grep --pattern 'class StateItem {
$$$
}'
# Check how curSymbol is assigned/initialized
rg -A 5 "curSymbol =" packages/shader-lab/src/lalr/
Length of output: 7828
packages/shader-lab/src/parser/types.ts (3)
1-1: Importing NoneTerminal.
This aligns with the broader type rename. Confirm that no other references to ENonTerminal remain in this file or its dependents.
17-21: StructProp constructor refactor and optional mrtIndex.
The introduction of an optional mrtIndex property is beneficial for MRT usage. Verify that the rest of the pipeline uses this property safely without assuming it’s always present.
6-6: TraceStackItem updated to NoneTerminal | BaseToken.
Ensure any code referencing this union type is aware that NoneTerminal is now possible in place of the old ENonTerminal. Verify usage within the parser logic to confirm correctness.
✅ Verification successful
Let me search for references to TraceStackItem and its usage in the codebase to verify the impact of this type change.
Let me check how _traceBackStack is used in ShaderTargetParser.ts to understand if there are any type-specific operations that might be affected by the type change.
Let me check one more thing - let's look for the actual usage of NoneTerminal and any type-casting operations in the parser.
Based on the search results, I can now generate the final response:
No impact from TraceStackItem type change.
The type change from ENonTerminal to NoneTerminal in TraceStackItem is safe as there are no type-specific operations or type checks in the codebase that would be affected by this change. The only usage of TraceStackItem is in ShaderTargetParser.ts where it's used in _traceBackStack array, and the type casting operations are compatible with both the old and new types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for references to TraceStackItem
rg "TraceStackItem" -A 3
# Search for any usage patterns of TraceStackItem with type checks
ast-grep --pattern 'TraceStackItem'
Length of output: 1195
Script:
#!/bin/bash
# Search for usage of _traceBackStack in ShaderTargetParser.ts
rg "_traceBackStack" packages/shader-lab/src/parser/ShaderTargetParser.ts -B 2 -A 5
# Look for any type checks or operations related to ENonTerminal
ast-grep --pattern 'instanceof ENonTerminal'
Length of output: 1239
Script:
#!/bin/bash
# Look for NoneTerminal usage and type casts
rg "NoneTerminal" packages/shader-lab/src/parser -B 2 -A 5
# Check for the definition and history of ENonTerminal
rg "ENonTerminal" packages/shader-lab/src/parser -B 2 -A 5
Length of output: 44015
packages/shader-lab/src/lalr/Utils.ts (4)
4-4: Renamed import from ENonTerminal to NoneTerminal.
Maintains the consistent renaming approach across the codebase. Make sure all references to GrammarSymbol remain valid.
24-24: createProductionWithOptions now takes a NoneTerminal.
This method signature is aligned with the rename. Confirm other references in the grammar creation logic and remove any leftover references to the old type.
[approve]
13-13: Terminal check uses NoneTerminal.START boundary.
This numeric comparison can be subtle. If additional NoneTerminal constants are added later, ensure START remains the correct threshold for differentiating terminals vs. non-terminals.
✅ Verification successful
Let me gather information about the NoneTerminal enum and its usage to verify this concern.
Let me search for the Terminal enum to understand the complete symbol system.
Let me search for the type definition of GrammarSymbol to understand how terminals and non-terminals are represented.
Let me search for the Terminal enum definition one more time with a different approach.
Based on the gathered information, I can now provide a final response:
Terminal check using NoneTerminal.START=2000 is a robust boundary.
The implementation is safe and well-designed. The NoneTerminal.START value is set to 2000 and serves as a clear boundary between terminal and non-terminal symbols. This is a robust design choice because:
- All non-terminals are defined after START in the enum
- The value 2000 provides ample space for terminal symbols (0-1999)
- The boundary is used consistently across the codebase for symbol differentiation
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the NoneTerminal enum definition and its usage
ast-grep --pattern 'enum NoneTerminal {
$$$
}'
# Also search for any direct references to NoneTerminal.START
rg "NoneTerminal\.START" -A 2 -B 2
Length of output: 9188
Script:
#!/bin/bash
# Find Terminal enum definition and any other related symbol definitions
ast-grep --pattern 'enum Terminal {
$$$
}'
# Also search for any other enum types that might be related to grammar symbols
rg "enum.*Terminal" -A 5
Length of output: 450
Script:
#!/bin/bash
# Find GrammarSymbol type definition
ast-grep --pattern 'type GrammarSymbol = $_'
# Also search for any files that export or define GrammarSymbol
rg "GrammarSymbol" -A 2 -B 2 --type typescript
Length of output: 233
Script:
#!/bin/bash
# Find Terminal enum and its values
rg "enum Terminal" -A 50
# Also check how Terminal values are defined
rg "Terminal\." -B 2 -A 2
Length of output: 65591
20-20: Converting GrammarSymbol to string using NoneTerminal array lookup.
The logic is correct for non-terminals, but pay attention to potential out-of-bounds array lookups if new NoneTerminal values are introduced. Unit tests should cover newly added symbols.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: packages/shader-lab/src/lalr/Utils.ts#L20
Added line #L20 was not covered by tests
packages/shader-lab/src/parser/GrammarSymbol.ts (2)
75-75: LGTM: Layout qualifier support for MRT.
The addition of layout_qualifier to the grammar is essential for supporting Multi-Render Target (MRT) functionality, allowing proper parsing of layout qualifiers in shaders.
111-111: LGTM: Type definition updated correctly.
The GrammarSymbol type definition has been correctly updated to use the renamed enum.
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (4)
17-18: LGTM: Clear naming for MRT-related constants.
The constants V3_GL_FragColor and V3_GL_FragData are well-named and clearly indicate their purpose for version 3 shader support.
29-30: LGTM: Well-defined abstract methods for MRT support.
The abstract methods provide a clear contract for subclasses to implement MRT-specific code generation:
getFragDataCodeGen: Generates code for accessing fragment datagetReferencedMRTPropText: Generates text for MRT property references
74-81: Handle MRT struct variable declarations without initialization.
The current implementation doesn't handle MRT struct variable declarations without initialization, as mentioned in the previous review.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 78-79: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L78-L79
Added lines #L78 - L79 were not covered by tests
88-102: LGTM: Proper handling of gl_FragData and gl_FragColor conflicts.
The code correctly:
- Detects conflicts between gl_FragData and gl_FragColor usage
- Generates appropriate MRT code
- Reports errors when conflicts are found
However, the error handling code is not covered by tests.
✅ Verification successful
Error handling for gl_FragData and gl_FragColor conflicts is properly tested
The code's error handling for conflicts between gl_FragData and gl_FragColor is well covered by tests:
- The test file
tests/src/shader-lab/ShaderLab.test.tsincludes a specific test case "mrt-error1" - The test shader
mrt-error1.shadercontains code that deliberately triggers the conflict by using both gl_FragColor and gl_FragData[0] - The test verifies that:
- An error is raised
- The error message matches the expected text "cannot use both gl_FragData and gl_FragColor"
- The error is of the correct type (GSError)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for gl_FragData and gl_FragColor conflict handling
# Search for relevant test cases
rg -l "gl_FragData.*gl_FragColor|gl_FragColor.*gl_FragData" --type ts --glob "*test.ts"
Length of output: 128
Script:
#!/bin/bash
# Let's examine the actual test content to verify the coverage
rg -A 10 -B 10 "gl_FragData.*gl_FragColor|gl_FragColor.*gl_FragData" tests/src/shader-lab/ShaderLab.test.ts
Length of output: 648
Script:
#!/bin/bash
# Let's check the actual test shader file to understand the test coverage better
cat ./shaders/mrt-error1.shader
Length of output: 93
Script:
#!/bin/bash
# Let's find the correct path to the test shader file
fd mrt-error1.shader
Length of output: 69
Script:
#!/bin/bash
# Now let's examine the content of the test shader file with the correct path
cat tests/src/shader-lab/shaders/mrt-error1.shader
Length of output: 703
packages/shader-lab/src/lalr/CFG.ts (2)
197-209: LGTM: Well-defined layout qualifier grammar.
The layout qualifier production rules correctly implement the GLSL specification for layout qualifiers:
layout(location = n)This enables proper parsing of MRT output declarations.
182-193: LGTM: Proper integration of layout qualifiers with struct declarations.
The struct declaration grammar has been correctly extended to support layout qualifiers, enabling MRT struct declarations like:
layout(location = 0) vec4 color;packages/shader-lab/src/parser/AST.ts (4)
Line range hint 31-56: LGTM! Good improvements to the TreeNode class.
The changes improve memory management through object pooling and provide better initialization hooks for subclasses.
1092-1097: 🛠️ Refactor suggestion
Add validation for layout qualifier index.
The implementation should validate that:
- The index token is a valid number
- The index is non-negative
- The index is within the maximum allowed MRT outputs for the target GLES version
@ASTNodeDecorator(NoneTerminal.layout_qualifier)
export class LayoutQualifier extends TreeNode {
get index(): number {
- return Number((<BaseToken>this.children[4]).lexeme);
+ const indexToken = this.children[4] as BaseToken;
+ const index = Number(indexToken.lexeme);
+ if (isNaN(index) || index < 0) {
+ throw new Error(`Invalid layout qualifier index: ${indexToken.lexeme}`);
+ }
+ return index;
}
}Likely invalid or redundant comment.
16-28: 🛠️ Refactor suggestion
Consider improving type safety of the decorator implementation.
The decorator pattern is a good improvement for maintainability, but there are some type safety concerns:
- The
@ts-ignorecomment suppresses TypeScript's type checking - The
any[]type in the constructor args could be more specific
Consider this safer implementation:
function ASTNodeDecorator(nonTerminal: NoneTerminal) {
- return function <T extends { new (...args: any[]): TreeNode }>(ASTNode: T) {
+ return function <T extends { new (...args: unknown[]): TreeNode }>(ASTNode: T) {
const ASTNodeClass = class extends ASTNode {
constructor(...args: unknown[]) {
super();
this.nt = nonTerminal;
}
};
- // @ts-ignore
- ASTNode.pool = ShaderLabUtils.createObjectPool(ASTNodeClass);
+ (ASTNode as unknown as { pool: ClearableObjectPool<TreeNode> }).pool =
+ ShaderLabUtils.createObjectPool(ASTNodeClass);
return ASTNodeClass;
};
}Likely invalid or redundant comment.
365-368: 🛠️ Refactor suggestion
Improve error messages and add test coverage.
The error handling paths lack test coverage and could benefit from more descriptive messages.
Consider improving the error messages:
- sa.reportError(id.location, `Undeclared symbol: ${id.lexeme}`);
+ sa.reportError(id.location, `Undeclared symbol '${id.lexeme}' in integer constant expression`);
- sa.reportError(id.location, "Invalid integer.");
+ sa.reportError(id.location, `Type mismatch: expected 'int', got '${id.typeInfo}'`);Let's verify the test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 365-365: packages/shader-lab/src/parser/AST.ts#L365
Added line #L365 was not covered by tests
[warning] 368-368: packages/shader-lab/src/parser/AST.ts#L368
Added line #L368 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/shader-lab/src/parser/AST.ts (2)
16-21: Improve type safety of the ASTNodeDecoratorThe decorator implementation could benefit from stronger type safety.
-function ASTNodeDecorator(nonTerminal: NoneTerminal) { - return function <T extends { new (): TreeNode }>(ASTNode: T) { +function ASTNodeDecorator(nonTerminal: NoneTerminal) { + return function <T extends { new (...args: any[]): TreeNode }>( + ASTNode: T & { pool?: ClearableObjectPool<TreeNode> } + ) { ASTNode.prototype.nt = nonTerminal; - (<any>ASTNode).pool = ShaderLabUtils.createObjectPool(ASTNode); + ASTNode.pool = ShaderLabUtils.createObjectPool(ASTNode); }; }
331-331: Add comprehensive test coverage for error handling pathsSeveral error handling paths lack test coverage:
- Operator validation in IntegerConstantExpressionOperator
- Array of array validation in InitDeclaratorList
- Return statement validation in FunctionDefinition
Would you like me to help create test cases for these scenarios? I can:
- Generate test cases for invalid operators
- Create tests for array of array declarations
- Add tests for return statement validation in both void and non-void functions
Also applies to: 422-422, 651-652, 655-656
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 331-331: packages/shader-lab/src/parser/AST.ts#L331
Added line #L331 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/shader-lab/src/parser/AST.ts(50 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/parser/AST.ts
[warning] 331-331: packages/shader-lab/src/parser/AST.ts#L331
Added line #L331 was not covered by tests
[warning] 354-354: packages/shader-lab/src/parser/AST.ts#L354
Added line #L354 was not covered by tests
[warning] 357-357: packages/shader-lab/src/parser/AST.ts#L357
Added line #L357 was not covered by tests
[warning] 422-422: packages/shader-lab/src/parser/AST.ts#L422
Added line #L422 was not covered by tests
[warning] 651-652: packages/shader-lab/src/parser/AST.ts#L651-L652
Added lines #L651 - L652 were not covered by tests
[warning] 655-656: packages/shader-lab/src/parser/AST.ts#L655-L656
Added lines #L655 - L656 were not covered by tests
🔇 Additional comments (1)
packages/shader-lab/src/parser/AST.ts (1)
1081-1086: 🛠️ Refactor suggestion
Add validation for layout qualifier index
The implementation should validate that:
- The index token is a valid number
- The index is non-negative
- The index is within the maximum allowed MRT outputs for the target GLES version
@ASTNodeDecorator(NoneTerminal.layout_qualifier)
export class LayoutQualifier extends TreeNode {
get index(): number {
- return Number((<BaseToken>this.children[4]).lexeme);
+ const indexToken = <BaseToken>this.children[4];
+ const index = Number(indexToken.lexeme);
+ if (isNaN(index) || index < 0) {
+ throw new Error(`Invalid layout qualifier index: ${indexToken.lexeme}`);
+ }
+ return index;
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
173-175:⚠️ Potential issueHandle MRT struct variable declarations without initialization
The current implementation doesn't handle MRT struct variable declarations without initialization, as mentioned in the previous review.
Apply this diff to fix the issue:
if (child instanceof ASTNode.InitDeclaratorList) { const typeLexeme = child.typeInfo.typeLexeme; if (context.isVaryingStruct(typeLexeme) || context.isMRTStruct(typeLexeme)) return ""; +} else if (child instanceof ASTNode.Declaration) { + const typeLexeme = child.typeInfo.typeLexeme; + if (context.isMRTStruct(typeLexeme)) return ""; }
🧹 Nitpick comments (18)
packages/shader-lab/src/parser/symbolTable/SymbolTable.ts (3)
11-22: Clarify symbol replacement behavior
When_compareWithreturnstrue, the existing symbol is replaced. If overloading or signature-based distinctions are needed, consider logging more details or ensuring that replacement is intentional.
24-43: Eliminate the<any>type assertion
Casting to<any>weakens type guarantees. Explore adjusting the return type or using a more specific type assertion to maintain strong typing.
50-73: Check for potential collisions
ForVARandSTRUCT,_compareWithends up returningtruewithout further checks, potentially treating distinct definitions as duplicates. Consider adding more logic for overshadowing or redefinition handling.packages/shader-lab/src/parser/symbolTable/SymbolInfo.ts (1)
25-26: Clarifysignatureusage
An optionalsignaturemakes sense for functions, but consider documenting or restricting usage for non-function symbols to prevent confusion.packages/shader-lab/src/parser/SemanticAnalyzer.ts (1)
88-92: Unify error handling
When_VERBOSEis disabled, errors are only logged. If you want consistent diagnostic data, consider always storing them inthis.errorsas well, with logging purely for user-facing feedback.packages/shader-lab/src/codeGen/VisitorContext.ts (1)
27-27: Consider enhancing type safety for MRT structures.The MRT-related properties could benefit from more specific types:
- mrtStruct?: ASTNode.StructSpecifier; + readonly mrtStruct?: Readonly<ASTNode.StructSpecifier>; - _referencedMRTList: Record<string, StructProp | string> = Object.create(null); + _referencedMRTList: Record<string, Readonly<StructProp> | string> = Object.create(null);Also applies to: 34-34
packages/shader-lab/src/codeGen/GLES300.ts (1)
106-119: Enhance gl_FragColor validation robustness.While the implementation correctly prevents simultaneous use of gl_FragColor and gl_FragData, consider:
- Moving the validation logic to a dedicated method for better maintainability
- Adding early returns to reduce nesting
- if (context.stage === EShaderStage.FRAGMENT && node.lexeme === "gl_FragColor") { - // #if _VERBOSE - if (context._referencedMRTList["gl_FragData"]) { - this._reportError(node.location, "cannot use both gl_FragData and gl_FragColor"); - } - if (context.mrtStruct) { - this._reportError(node.location, "gl_FragColor cannot be used with MRT (Multiple Render Targets)."); - } - // #endif - this._registerFragColorVariable(node); - return V3_GL_FragColor; - } + if (context.stage !== EShaderStage.FRAGMENT || node.lexeme !== "gl_FragColor") { + return super.visitVariableIdentifier(node); + } + + this._validateFragColorUsage(node); + this._registerFragColorVariable(node); + return V3_GL_FragColor;packages/shader-lab/src/codeGen/GLESVisitor.ts (1)
29-31: Add JSDoc to clarify output parameter ownership.The methods now use output parameters for better performance, but their behavior should be documented:
+ /** + * Populates the output array with attribute declarations. + * @param out The array to populate with declarations. The array is modified in place. + */ abstract getAttributeDeclare(out: ICodeSegment[]): void; + /** + * Populates the output array with varying declarations. + * @param out The array to populate with declarations. The array is modified in place. + */ abstract getVaryingDeclare(out: ICodeSegment[]): void; + /** + * Populates the output array with MRT declarations. + * @param out The array to populate with declarations. The array is modified in place. + */ abstract getMRTDeclare(out: ICodeSegment[]): void;packages/shader-lab/src/codeGen/CodeGenVisitor.ts (2)
29-30: Add JSDoc documentation for abstract methodsThese abstract methods are crucial for MRT functionality. Please add JSDoc documentation explaining:
- Purpose and usage of each method
- Parameter descriptions and constraints
- Return value description
- Example usage if applicable
94-102: Extract gl_FragData string literal to a constantConsider extracting the "gl_FragData" string literal to a constant, similar to how
V3_GL_FragColorandV3_GL_FragDataare defined.+export const GL_FRAG_DATA = "gl_FragData"; if (identLexeme === GL_FRAG_DATA) {packages/shader-lab/src/contentParser/ShaderContentParser.ts (6)
59-59: Static symbol table stack
Defining a static_symbolTableStackcentralizes symbol management across parsing routines. Ensure concurrency or re-entrance is not a concern.
63-64: Avoidthisfrom static context
The static analysis tool flags usage ofthisin static methods as confusing. Consider referencing the class name directly to improve clarity.- this._symbolTableStack.clear(); - this._newScope(); + ShaderContentParser._symbolTableStack.clear(); + ShaderContentParser._newScope();🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: 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] 64-64: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
159-159: Avoidthisfrom static context
For clarity and to address the static analysis hint, consider referencingShaderContentParser._symbolTableStack.dropScope()instead ofthis._symbolTableStack.dropScope().- this._symbolTableStack.dropScope(); + ShaderContentParser._symbolTableStack.dropScope();🧰 Tools
🪛 Biome (1.9.4)
[error] 159-159: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
216-216: Avoidthisfrom static context
Same suggestion to reference the class directly:- this._symbolTableStack.insert({ ident: ident.lexeme, ... + ShaderContentParser._symbolTableStack.insert({ ident: ident.lexeme, ...🧰 Tools
🪛 Biome (1.9.4)
[error] 216-216: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
230-231: Creating and pushing a new scope
The approach is solid. Similarly, replacethis._symbolTableStack.newScope(symbolTable);withShaderContentParser._symbolTableStack.newScope(symbolTable);if strictly following the static analysis suggestion.- this._symbolTableStack.newScope(symbolTable); + ShaderContentParser._symbolTableStack.newScope(symbolTable);🧰 Tools
🪛 Biome (1.9.4)
[error] 231-231: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
235-235: Dropping scope
Use the class name to avoid confusion in a static method:- this._symbolTableStack.dropScope(); + ShaderContentParser._symbolTableStack.dropScope();🧰 Tools
🪛 Biome (1.9.4)
[error] 235-235: 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/contentParser/ContentSymbolTable.ts (1)
5-8: Typings forISymbol
DefiningISymbolwithtypeand optionalvalueis flexible. Consider refining thevaluetype if more constraints are known to avoid potential misuse.packages/shader-lab/src/parser/AST.ts (1)
16-21: Enhance type safety of the ASTNodeDecoratorThe decorator implementation could be improved with better type constraints.
-function ASTNodeDecorator(nonTerminal: NoneTerminal) { - return function <T extends { new (): TreeNode }>(ASTNode: T) { +function ASTNodeDecorator(nonTerminal: NoneTerminal) { + return function <T extends { new (): TreeNode } & { prototype: { nt: NoneTerminal } }>(ASTNode: T) { ASTNode.prototype.nt = nonTerminal; (<any>ASTNode).pool = ShaderLabUtils.createObjectPool(ASTNode); }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/shader-lab/src/codeGen/CodeGenVisitor.ts(8 hunks)packages/shader-lab/src/codeGen/GLES300.ts(3 hunks)packages/shader-lab/src/codeGen/GLESVisitor.ts(4 hunks)packages/shader-lab/src/codeGen/VisitorContext.ts(7 hunks)packages/shader-lab/src/common/BaseSymbolTable.ts(1 hunks)packages/shader-lab/src/contentParser/ContentSymbolTable.ts(1 hunks)packages/shader-lab/src/contentParser/ShaderContentParser.ts(7 hunks)packages/shader-lab/src/contentParser/SymbolTable.ts(0 hunks)packages/shader-lab/src/parser/AST.ts(47 hunks)packages/shader-lab/src/parser/SemanticAnalyzer.ts(3 hunks)packages/shader-lab/src/parser/symbolTable/SymbolInfo.ts(3 hunks)packages/shader-lab/src/parser/symbolTable/SymbolTable.ts(1 hunks)tests/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/shader-lab/src/contentParser/SymbolTable.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/codeGen/GLESVisitor.ts
[warning] 63-63: packages/shader-lab/src/codeGen/GLESVisitor.ts#L63
Added line #L63 was not covered by tests
[warning] 122-123: packages/shader-lab/src/codeGen/GLESVisitor.ts#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 127-128: packages/shader-lab/src/codeGen/GLESVisitor.ts#L127-L128
Added lines #L127 - L128 were not covered by tests
packages/shader-lab/src/codeGen/CodeGenVisitor.ts
[warning] 79-80: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
packages/shader-lab/src/contentParser/ContentSymbolTable.ts
[warning] 14-20: packages/shader-lab/src/contentParser/ContentSymbolTable.ts#L14-L20
Added lines #L14 - L20 were not covered by tests
packages/shader-lab/src/codeGen/GLES300.ts
[warning] 74-74: packages/shader-lab/src/codeGen/GLES300.ts#L74
Added line #L74 was not covered by tests
[warning] 128-131: packages/shader-lab/src/codeGen/GLES300.ts#L128-L131
Added lines #L128 - L131 were not covered by tests
packages/shader-lab/src/parser/AST.ts
[warning] 331-331: packages/shader-lab/src/parser/AST.ts#L331
Added line #L331 was not covered by tests
[warning] 354-354: packages/shader-lab/src/parser/AST.ts#L354
Added line #L354 was not covered by tests
[warning] 357-357: packages/shader-lab/src/parser/AST.ts#L357
Added line #L357 was not covered by tests
[warning] 422-422: packages/shader-lab/src/parser/AST.ts#L422
Added line #L422 was not covered by tests
[warning] 428-428: packages/shader-lab/src/parser/AST.ts#L428
Added line #L428 was not covered by tests
packages/shader-lab/src/codeGen/VisitorContext.ts
[warning] 103-109: packages/shader-lab/src/codeGen/VisitorContext.ts#L103-L109
Added lines #L103 - L109 were not covered by tests
🪛 Biome (1.9.4)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 62-62: 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] 63-63: 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] 159-159: 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] 216-216: 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] 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] 231-231: 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] 235-235: 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/codeGen/VisitorContext.ts
[error] 68-68: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 98-98: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (29)
tests/vitest.config.ts (1)
22-23: Adding headless mode for browser tests is a great practice!
This change ensures that tests can run reliably in a CI or any environment without needing a graphical interface. This is beneficial for automated testing scenarios.
packages/shader-lab/src/parser/symbolTable/SymbolTable.ts (1)
1-8: Imports look good
All newly introduced imports appear consistent with the usage in the file.
packages/shader-lab/src/parser/symbolTable/SymbolInfo.ts (2)
2-2: Good addition of GalaceanDataType import
This ensures the symbol’s signatures can be typed consistently throughout the code.
12-12: Exporting SymbolAstNode
Making SymbolAstNode exportable broadens its usability and aligns with the extended features in other files.
packages/shader-lab/src/parser/SemanticAnalyzer.ts (2)
36-36: Changing to Error[] may reduce functionality
Using a generic Error[] instead of GSError[] could lose specialized features. This was previously noted; ensure it aligns with your overall error handling strategy.
74-86: lookupSymbolBy returns only the first match
This is appropriate if only one symbol per identifier is expected. However, if multiple function overloads are supported, consider returning all matches or implementing a more detailed resolution strategy.
packages/shader-lab/src/codeGen/VisitorContext.ts (1)
98-111: Add test coverage for MRT property error handling.
The error handling path for missing MRT properties lacks test coverage. This is critical for ensuring robust MRT functionality.
Would you like me to help generate test cases that verify:
- Error handling when referenced MRT property is not found
- Proper error message formatting
- Correct property registration in _referencedMRTList
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 GitHub Check: codecov/patch
[warning] 103-109: packages/shader-lab/src/codeGen/VisitorContext.ts#L103-L109
Added lines #L103 - L109 were not covered by tests
packages/shader-lab/src/codeGen/GLES300.ts (1)
23-29: LGTM! Clean implementation of MRT code generation methods.
The implementation follows consistent patterns and properly handles MRT output declarations.
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3)
17-18: LGTM: Constants and class modifications are well-structured
The addition of fragment-related constants and the conversion to an abstract class with proper error handling improves the code structure.
Also applies to: 23-27
75-82: Add test coverage for MRT error handling
The error handling code for MRT properties is not covered by tests according to the coverage report.
Please add test cases covering:
- Valid MRT property references
- Invalid MRT property references
- Error conditions and error message validation
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-80: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L79-L80
Added lines #L79 - L80 were not covered by tests
210-228: LGTM: Return statement handling is well-implemented
The changes to handle return statements with varying structs are well-structured and include proper type checking.
packages/shader-lab/src/common/BaseSymbolTable.ts (6)
13-13: Embrace the new abstract insertion contract
Changing the insert method to abstract forces concrete implementations to provide a tailor-made insert logic. This is a solid move to ensure each derived table handles insertion details appropriately.
17-17: Reconsider exposing stack as public
By making stack publicly accessible, external callers can modify internal state at will. Please confirm there is a deliberate need for this exposure. Otherwise, consider restricting visibility to safeguard encapsulation.
20-20: Evaluate potential empty-stack edge case
this.stack[this.stack.length - 1] may throw if stack is empty. Ensure that an empty stack scenario (e.g., calling _scope outside a scope) is properly handled.
24-24: Ensure consistent scope push logic
Pushing a new scope is straightforward. Just confirm there’s no need to validate scope collisions or duplicates.
28-28: Clear operation
Resetting this.stack.length = 0 is fine and lightweight. Confirm there are no references to older scopes that need explicit disposal if memory or resource cleanup is required.
32-32: Maintain scope integrity
Dropping the top scope is correct. Double-check if you need a finalization routine (e.g., calling a destructor or cleanup function) upon scope removal.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (7)
3-3: Imports look good
No issues detected with import statements.
7-7: Adoption of ContentSymbolTable
Refactoring to use ContentSymbolTable aligns well with the new symbol management approach.
108-110: Render state declarator check
These lines succinctly identify render state tokens. Looks good.
112-114: Engine type check
Similarly for engine types, the approach is clear and direct.
116-124: Hierarchical symbol lookup
Traversing the stack from top to bottom is a solid approach to scope-based symbol resolution. Returning undefined is acceptable. Just be sure callers handle the undefined case properly.
195-195: Double-check missing symbol scenarios
_lookupSymbolByType returns undefined if not found. Ensure the fallback or error path is tested thoroughly to avoid silent failures.
226-226: Insert usage
Inserting a new symbol table entry is straightforward. Ensure references to this._symbolTableStack in static context are replaced consistently.
🧰 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/contentParser/ContentSymbolTable.ts (3)
1-4: Module import
These imports form the backbone of the new symbol table class. No issues identified.
24-33: Lookup by type
Scanning the entries linearly is acceptable for smaller sets. If performance becomes a concern, consider indexing by (ident, type). For now, this is clear and straightforward.
10-22: Insertion logic with replace semantics
This approach replaces an existing symbol if types match, logging a warning. Ensure test coverage for this replacement path.
You may consider separate logs for insertion vs. replacement to differentiate normal insert from override.
- Logger.warn("replace symbol:", sm.ident);
+ Logger.warn(`Replacing existing symbol of type ${sm.type}:`, sm.ident);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-20: packages/shader-lab/src/contentParser/ContentSymbolTable.ts#L14-L20
Added lines #L14 - L20 were not covered by tests
packages/shader-lab/src/parser/AST.ts (2)
1081-1086: 🛠️ Refactor suggestion
Add validation for layout qualifier index
The implementation should validate that:
- The index token is a valid number
- The index is non-negative
- The index is within the maximum allowed MRT outputs for the target GLES version
get index(): number {
- return Number((<BaseToken>this.children[4]).lexeme);
+ const indexToken = <BaseToken>this.children[4];
+ const index = Number(indexToken.lexeme);
+ if (isNaN(index) || index < 0) {
+ throw new Error(`Invalid layout qualifier index: ${indexToken.lexeme}`);
+ }
+ return index;
}Likely invalid or redundant comment.
354-357: Improve error messages for integer constant expression validation
The error messages could be more descriptive to help developers understand the issue better.
- sa.reportError(id.location, `Undeclared symbol: ${id.lexeme}`);
+ sa.reportError(id.location, `Undeclared symbol '${id.lexeme}' in integer constant expression`);
- sa.reportError(id.location, "Invalid integer.");
+ sa.reportError(id.location, `Type mismatch: expected 'int', got '${id.typeInfo}'`);Let's verify the test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 354-354: packages/shader-lab/src/parser/AST.ts#L354
Added line #L354 was not covered by tests
[warning] 357-357: packages/shader-lab/src/parser/AST.ts#L357
Added line #L357 was not covered by tests
| const { returnStatement } = fnNode; | ||
| if (returnStatement) { | ||
| returnStatement.isFragReturnStatement = true; | ||
| } | ||
|
|
||
| const { context } = VisitorContext; | ||
| context.stage = EShaderStage.FRAGMENT; | ||
|
|
||
| const { type: returnDataType, location: returnLocation } = fnNode.protoType.returnType; | ||
| if (typeof returnDataType === "string") { | ||
| const mrtStruct = symbolTable.lookupBy(returnDataType, ESymbolType.STRUCT); | ||
| if (!mrtStruct) { | ||
| this._reportError(returnLocation, `invalid mrt struct: ${returnDataType}`); | ||
| } else { | ||
| context.mrtStruct = mrtStruct.astNode; | ||
| } | ||
| } else if (returnDataType !== EKeyword.VOID && returnDataType !== EKeyword.VEC4) { | ||
| this._reportError(returnLocation, "fragment main entry can only return struct or vec4."); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for fragment main error handling needs improvement
Based on the analysis of the test files and implementation, there are gaps in test coverage:
- The
mrt-struct.shaderonly tests the valid MRT struct case but doesn't test error handling - The
mrt-error1.shadertests an invalid fragment shader but not specifically the MRT struct validation errors - No tests found for invalid return type validation (non-vec4, non-struct cases)
Here are the missing test cases that should be added:
- Test error handling when referencing a non-existent MRT struct
- Test error when returning invalid types (e.g., vec2, vec3, float)
- Test error message formatting for both MRT struct and return type validation
- Test proper error location reporting in the error messages
🔗 Analysis chain
Add test coverage for fragment main error handling.
The error handling paths for MRT struct validation and return type checking lack test coverage:
Would you like me to help generate test cases that verify:
- Error handling for invalid MRT struct references
- Error handling for invalid return types
- Proper error message formatting
- Correct MRT struct registration
Run this script to verify current test coverage:
Let me search for test files and test cases more effectively.
Let me check for specific test files that might contain shader validation tests.
Let me try one more search to find any existing test files or test cases.
Let me check for test files specifically related to fragment shader validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for fragment main error handling
# Find all test files related to fragment shaders
fd -e ts -e js test shader fragment
# Search for existing test cases
rg -A 5 "fragment.*main.*error"
rg -A 5 "invalid.*mrt.*struct"
Length of output: 872
Script:
#!/bin/bash
# Find test files in the shader-lab package
fd -e ts -e js test packages/shader-lab
# Look for fragment shader test cases
rg -l "fragment" $(fd -e ts -e js test packages/shader-lab)
# Search for GLESVisitor test cases
rg -A 10 "GLESVisitor" $(fd -e ts -e js test packages/shader-lab)
Length of output: 4695
Script:
#!/bin/bash
# Check shader validation test content
rg -A 10 "ShaderValidate" tests/src/shader-lab/ShaderValidate.ts
# Look for any test cases related to main function validation
rg -A 10 "main.*function" tests/src/shader-lab/ShaderValidate.ts
# Check for any error validation tests
rg -A 10 "expect.*error" tests/src/shader-lab/ShaderValidate.ts
rg -A 10 "_reportError" tests/src/shader-lab/ShaderValidate.ts
Length of output: 260
Script:
#!/bin/bash
# Find all test files in the repository
fd -e test.ts -e spec.ts . packages/shader-lab/
# Search for any shader validation or error handling tests
rg -l "test|describe|it" $(fd -e test.ts -e spec.ts . packages/shader-lab/)
# Look for any existing fragment shader validation
rg -A 10 "fragment" $(fd -e test.ts -e spec.ts . packages/shader-lab/)
Length of output: 66662
Script:
#!/bin/bash
# Look for test files with MRT or fragment shader validation
rg -A 10 "mrt.*struct|fragment.*main" tests/src/shader-lab/shaders/
# Check content of potential test files
cat tests/src/shader-lab/shaders/mrt-struct.shader
cat tests/src/shader-lab/shaders/mrt-error1.shader
Length of output: 1818
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-123: packages/shader-lab/src/codeGen/GLESVisitor.ts#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 127-128: packages/shader-lab/src/codeGen/GLESVisitor.ts#L127-L128
Added lines #L127 - L128 were not covered by tests
| } else if (this.children.length === 4 || this.children.length === 6) { | ||
| const typeInfo = this.typeInfo; | ||
| const arraySpecifier = this.children[3] as ArraySpecifier; | ||
| // #if _VERBOSE | ||
| if (typeInfo.arraySpecifier && arraySpecifier) { | ||
| sa.error(arraySpecifier.location, "Array of array is not supported."); | ||
| sa.reportError(arraySpecifier.location, "Array of array is not supported."); |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for array of array validation error
The verification reveals that:
- There are no test files (*.test.ts or *.spec.ts) in the entire shader-lab package
- The array of array validation error message exists only in AST.ts
- The error handling for array of array declarations is completely untested
Please add test cases to cover:
- Array declarations with valid single dimension
- Array declarations with invalid multiple dimensions to verify the error message
- Edge cases around array specifier parsing
🔗 Analysis chain
Add test coverage for array of array validation
The codebase analysis reveals that:
- The error message "Array of array is not supported" is only present in AST.ts
- No test files were found in the expected test directories
- The error handling for array of array declarations appears to be completely untested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find test files for array of array validation
fd -e test.ts -e spec.ts . packages/shader-lab/src/parser/__tests__/
# Look for existing test cases
rg -l "Array of array is not supported" packages/shader-lab/src/parser/__tests__/
Length of output: 365
Script:
#!/bin/bash
# Find all test files in the shader-lab package
fd -e test.ts -e spec.ts . packages/shader-lab/
# Look for existing test cases in all files
rg -l "Array of array is not supported" packages/shader-lab/
# Find the test directory structure
fd . packages/shader-lab/ -t d
Length of output: 604
Script:
#!/bin/bash
# Look for any existing test files that might test array declarations
rg -l "array" packages/shader-lab/ -g "*.test.ts" -g "*.spec.ts"
# Check if there are any test files in the parser directory
fd . packages/shader-lab/src/parser/ -e test.ts -e spec.ts
# Look for array-related test cases in any file
rg "array" -i packages/shader-lab/src/parser/ -g "*.test.ts" -g "*.spec.ts" -A 5
Length of output: 206
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 422-422: packages/shader-lab/src/parser/AST.ts#L422
Added line #L422 was not covered by tests
| const { curFunctionInfo } = sa; | ||
| const { header, returnStatement } = curFunctionInfo; | ||
| if (header.returnType.type === EKeyword.VOID) { | ||
| if (returnStatement) { | ||
| sa.reportError(header.returnType.location, "Return in void function."); | ||
| } | ||
| } else { | ||
| if (!returnStatement) { | ||
| sa.reportError(header.returnType.location, `No return statement found.`); | ||
| } else { | ||
| this._returnStatement = returnStatement; | ||
| } | ||
| } | ||
| curFunctionInfo.header = undefined; | ||
| curFunctionInfo.returnStatement = undefined; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results from multiple attempts, I can now provide a final response:
Test coverage needed for function return validation
The return statement validation logic in packages/shader-lab/src/parser/AST.ts lacks test coverage. No test files were found in the parser directory or related directories that verify:
- Return statement validation in void functions
- Return statement validation in non-void functions
- Error reporting for missing return statements
- Error reporting for invalid return statements in void functions
🔗 Analysis chain
Add test coverage for return statement validation
The return statement validation logic lacks test coverage for both void and non-void functions.
Run the following script to verify test coverage:
Let me try a different approach to locate the test files.
Let me search for the actual test content to verify the test coverage.
Let me try one more search to find test files specifically for the parser.
Let me try one final search to look for any test files in the parser directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find test files for function return validation
fd -e test.ts -e spec.ts . packages/shader-lab/src/parser/__tests__/
# Look for test cases that verify return statement validation
rg -l "returnType|return.*statement" packages/shader-lab/src/parser/__tests__/
Length of output: 362
Script:
#!/bin/bash
# Find all test files in the repository
fd -e test.ts -e spec.ts . packages/shader-lab/
# Search for return type and statement validation tests in all files
rg -l "returnType|return.*statement" packages/shader-lab/
Length of output: 351
Script:
#!/bin/bash
# Look for test files in all possible test directories
fd -e test.ts -e spec.ts .
# Search for test cases specifically related to function return validation
rg -A 5 -B 5 "test.*return.*statement|test.*void.*function|describe.*return"
Length of output: 68534
Script:
#!/bin/bash
# Find all test files in the repository
fd -e test.ts -e spec.ts . tests/src/shader-lab/
# Search for test cases related to function return validation
rg -B 5 -A 5 "test.*function.*return|test.*void.*function|describe.*return" tests/src/shader-lab/
Length of output: 231
Script:
#!/bin/bash
# Look for test files in the parser directory
fd -e test.ts -e spec.ts . packages/shader-lab/src/parser/
# Search for any test cases in the parser directory that might be related to functions or return statements
rg -B 5 -A 5 "test|describe" packages/shader-lab/src/parser/__tests__/ || echo "No tests found in __tests__ directory"
Length of output: 300
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/shader-lab/src/codeGen/VisitorContext.ts (1)
68-68: Replacevoidwithundefinedin union types.The use of
voidin union types can be confusing. Consider usingundefinedinstead for better clarity.-referenceAttribute(ident: BaseToken): Error | void { +referenceAttribute(ident: BaseToken): Error | undefined { -referenceVarying(ident: BaseToken): Error | void { +referenceVarying(ident: BaseToken): Error | undefined { -referenceMRTProp(ident: BaseToken): Error | void { +referenceMRTProp(ident: BaseToken): Error | undefined {Also applies to: 83-83, 98-98
🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/shader-lab/src/parser/symbolTable/TargetSymbolTable.ts (1)
24-43: Make the fallback return explicit for clarity.When no matching symbol is found, the function implicitly returns
undefined. An explicitreturn undefined;(or adding a default branch) can make the behavior clearer for maintainers.if (entry) { for (let length = entry.length, i = 0; i < length; i++) { const item = entry[i]; if (this._compareWith(item, symbolType, signature, astNode)) return <any>item; } } + return undefined; }packages/shader-lab/src/contentParser/ContentSymbolTable.ts (2)
5-8: Reassess the use ofvalue?: anyinISymbol.Using
anycan obscure type safety, even in flexible systems. If you can narrow the possible types, consider a union or generic to capture intent more precisely.
11-22: Insertion logic replicatesTargetSymbolTableapproach.This is fine for consistency. However, if both classes end up sharing duplicate logic, consider factoring out common insert routines into a shared helper to abide by DRY (Don’t Repeat Yourself) principles.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (2)
116-123: Optimize symbol lookup with early returns.The
_lookupSymbolByTypeimplementation can be simplified using early returns.Consider this optimization:
private static _lookupSymbolByType(ident: string, type: TokenType): ISymbol | undefined { const stack = ShaderContentParser._symbolTableStack.stack; for (let length = stack.length, i = length - 1; i >= 0; i--) { const symbolTable = stack[i]; const ret = symbolTable.lookup(ident, type); - if (ret) return ret; + if (ret !== undefined) { + return ret; + } } + return undefined; }
Line range hint
1-500: Add JSDoc documentation for key methods.While the code is well-structured, adding JSDoc documentation for key methods would improve maintainability and help other developers understand the parsing logic better.
Consider adding documentation for methods like:
_lookupSymbolByType_parseRenderStateDeclarationOrAssignment_parseRenderStatePropListExample:
/** * Looks up a symbol by identifier and type in the symbol table stack. * @param ident - The identifier to look up * @param type - The token type to match * @returns The matching symbol or undefined if not found */ private static _lookupSymbolByType(ident: string, type: TokenType): ISymbol | undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/shader-lab/src/codeGen/GLESVisitor.ts(4 hunks)packages/shader-lab/src/codeGen/VisitorContext.ts(7 hunks)packages/shader-lab/src/contentParser/ContentSymbolTable.ts(1 hunks)packages/shader-lab/src/contentParser/ShaderContentParser.ts(7 hunks)packages/shader-lab/src/parser/SemanticAnalyzer.ts(2 hunks)packages/shader-lab/src/parser/ShaderInfo.ts(1 hunks)packages/shader-lab/src/parser/symbolTable/TargetSymbolTable.ts(1 hunks)packages/shader-lab/src/parser/symbolTable/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/codeGen/GLESVisitor.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 62-62: 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] 63-63: 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] 159-159: 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] 216-216: 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] 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] 231-231: 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] 235-235: 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/codeGen/VisitorContext.ts
[error] 68-68: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 83-83: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 98-98: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (17)
packages/shader-lab/src/codeGen/VisitorContext.ts (6)
27-27: LGTM! Property and record additions follow established patterns.
The new mrtStruct property and _referencedMRTList record are well-structured and consistent with the existing attribute and varying handling patterns.
Also applies to: 34-34
38-38: LGTM! Improved encapsulation of symbol table.
The change to make _passSymbolTable private with a public getter improves encapsulation while maintaining backward compatibility.
52-53: LGTM! Complete reset of MRT state.
The reset method properly clears all MRT-related state, maintaining consistency with the existing reset patterns.
64-66: LGTM! Consistent struct check implementation.
The isMRTStruct method follows the established pattern of struct type checking methods.
98-111: Add test coverage for MRT error handling.
The error handling in referenceMRTProp lacks test coverage. Consider adding test cases for:
- Missing MRT properties
- Invalid MRT references
- Error message formatting
Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
124-127: LGTM! Consistent symbol lookup implementation.
The symbol lookup implementation in referenceGlobal is consistent with the rest of the codebase.
packages/shader-lab/src/parser/symbolTable/TargetSymbolTable.ts (3)
11-22: Consider concurrency or re-entrant operations in insert method.
If multiple insertions happen concurrently, there's a risk that stale data might overwrite newer symbols, even with the _compareWith check. Ensure that either higher-level locking or atomic operations protect the table if concurrency is a possibility in this environment.
45-49: Function looks good.
The usage of filter for retrieving function symbols is straightforward and readable.
50-73: Double-check astNode assumptions in _compareWith.
By casting item.astNode to ASTNode.FunctionDefinition, this code presumes it’s never undefined nor a different node type. If you expect symbol entries of type FN to always have a valid FunctionDefinition, this is fine. Otherwise, guard against malformed or missing astNode data to avoid runtime errors.
packages/shader-lab/src/parser/symbolTable/index.ts (1)
5-5: Export update is consistent with new symbol table structure.
Removing the old SymbolTable export and adding TargetSymbolTable aligns the module with the new approach to symbol management.
packages/shader-lab/src/parser/ShaderInfo.ts (1)
2-5: Verify usage of the new TargetSymbolTable.
Changing the property type from SymbolTable to TargetSymbolTable requires that all references to symbolTable in dependent files be updated accordingly. Ensure no references to the old type remain.
✅ Verification successful
Let me generate additional verification scripts to check for any remaining references to the old SymbolTable type.
No references to old SymbolTable type found
The verification shows that all symbol table references in the codebase are using either:
- The new
TargetSymbolTabletype - Other specialized symbol table types (
ContentSymbolTable,BaseSymbolTable) - Utility types and imports (
SymbolTableStack,IBaseSymbol, etc.)
There are no direct usages of the old SymbolTable type in property or variable declarations, indicating that the migration to TargetSymbolTable is complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching codebase for leftover references to SymbolTable
rg -A 2 "symbolTable"
Length of output: 9858
Script:
#!/bin/bash
# Search for any direct references to SymbolTable type (not part of imports)
rg ":\s*SymbolTable\b" --type ts
# Search for imports of SymbolTable
rg "import.*SymbolTable.*from" --type ts
Length of output: 1077
packages/shader-lab/src/contentParser/ContentSymbolTable.ts (1)
24-32: Method is straightforward and clear.
Returning undefined when the symbol is not found is an expected approach for lookups. The code is concise and correct.
packages/shader-lab/src/parser/SemanticAnalyzer.ts (4)
27-31: Great improvement removing the any assertion for curFunctionInfo.
Initialising curFunctionInfo with a clear type is more robust than casting to any.
36-36: Changing errors from GSError[] to Error[] may break specialized error flow.
If the broader codebase relies on GSError-specific properties (e.g., wrappingLineCount), this refactor can introduce type mismatches or runtime property checks. Verify other locations that might depend on GSError.
74-86: Lookup logic is consistent with TargetSymbolTable.
The approach of searching from the innermost scope outward is standard in block-scoped languages. Looks good.
88-92: Confirm consistent error handling strategy in reportError.
Previously, certain code paths used throw with custom GSError. Now, it alternates between pushing to errors and logging. Verify that upstream components handle or display errors as intended in both verbose and non-verbose modes.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
59-59: LGTM! Verify the symbol table refactoring impact.
The migration from SymbolTable to ContentSymbolTable with stack-based scope management looks good. The implementation is consistent across all methods.
Let's verify that all symbol table operations are properly updated:
Also applies to: 63-63, 159-159, 216-216, 226-226, 230-231, 235-235
✅ Verification successful
Let me try a different approach to verify the symbol table refactoring:
Symbol table refactoring is correctly implemented
The verification shows that the symbol table refactoring is consistent and properly implemented:
ContentSymbolTableextendsBaseSymbolTableand provides specialized symbol management for shader content parsing- The migration from
SymbolTabletoContentSymbolTableis complete inShaderContentParser.ts - All symbol table operations (insert, lookup, scope management) are properly updated to work with the new implementation
- The changes don't conflict with other symbol table implementations in the codebase (
TargetSymbolTablefor parsing,ContentSymbolTablefor content parsing)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old SymbolTable implementation
# and verify consistent usage of ContentSymbolTable
# Check for any remaining references to SymbolTable
rg -l "SymbolTable(?!Stack)" --type ts
# Verify ContentSymbolTable usage patterns
ast-grep --pattern 'ContentSymbolTable'
Length of output: 561
Script:
#!/bin/bash
# Check for any remaining references to SymbolTable and related patterns
rg --pcre2 "SymbolTable(?!Stack)" --type ts
# Look for symbol table related operations
rg "symbolTable" --type ts -A 2
# Check the ContentSymbolTable implementation
cat packages/shader-lab/src/contentParser/ContentSymbolTable.ts
Length of output: 13934
| this._errors.length = 0; | ||
| this._symbolTable.clear(); | ||
| this._symbolTableStack.clear(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'this' with class name in static methods.
Using this in static methods can be confusing. For better clarity and maintainability, use the class name directly.
Apply these changes:
static reset() {
- this._errors.length = 0;
- this._symbolTableStack.clear();
+ ShaderContentParser._errors.length = 0;
+ ShaderContentParser._symbolTableStack.clear();
this._newScope();
}
// ... similar changes in other methods ...
private static _dropScope() {
- this._symbolTableStack.dropScope();
+ ShaderContentParser._symbolTableStack.dropScope();
}This pattern should be applied consistently across all static methods in the class.
Also applies to: 159-159, 216-216, 226-226, 231-231, 235-235
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: 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] 63-63: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This pr fullfilled issue #2373
Summary by CodeRabbit
New Features
gl_FragDatafor fragment shaders.Bug Fixes
Tests
Documentation
Refactor
ENonTerminaltoNoneTerminalacross the parser components.