Improve ShaderLab compilation error log and package build
#2364
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (19)
tests/src/shader-lab/test-case/source/frag.txt (1)
Line range hint
18-18: Revise theminmacro implementationThe current implementation of the
minmacro doesn't correctly return the minimum of two values. WhenX < Y, it returnsTTinstead ofX.Consider revising the macro to correctly return the minimum value:
#define min(X, Y) ((X) < (Y) ? (X) : (Y))This change ensures that the macro always returns the smaller of the two input values.
packages/shader-lab/src/preprocessor/MacroDefine.ts (2)
28-41: LGTM: Improved method implementation with better error handlingThe new
expandFunctionBodymethod is a significant improvement:
- It addresses the past review comment by clarifying the method's purpose.
- Error handling is more detailed and consistent with the new
GSErrorstrategy.- The conditional compilation for verbose mode allows for flexible behavior.
- The core logic of macro expansion is clearly implemented.
Consider enhancing the error message to include the expected and actual number of arguments:
- throw new GSError(GSErrorName.PreprocessorError, "mismatched function macro", this.location, ""); + throw new GSError(GSErrorName.PreprocessorError, `mismatched function macro: expected ${this.args.length} arguments, got ${args.length}`, this.location, "");This would provide more specific information for debugging.
Line range hint
1-41: LGTM: Improved overall class structure and consistencyThe updates to the
MacroDefineclass significantly improve code quality and maintainability:
- Modern TypeScript features are utilized effectively.
- Error handling is standardized and more informative.
- The changes align well with the PR objectives of improving error logging.
- The class now has a more consistent and robust implementation.
Consider adding JSDoc comments to the class and its methods to improve documentation. This would help developers understand the purpose and usage of the class, especially given its importance in macro processing.
packages/shader-lab/src/ShaderLab.ts (6)
22-25: Consider potential concurrency issues with static _processingPassText.The addition of the static
_processingPassTextproperty is noted. While it's likely useful for debugging or verbose mode, be cautious about potential concurrency issues in multi-threaded environments. Consider making it thread-local or instance-specific if concurrent access is a possibility.
Line range hint
26-41: Refactor suggestion: Use class name instead ofthisin static methods.The refactoring of
createPositionandcreateRangemethods to use thesetmethod is a good improvement. However, usingthisin a static context can be confusing. Consider replacingthiswith the class nameShaderLabin both methods.Apply this diff to refactor the code:
static createPosition(index: number, line?: number, column?: number): ShaderPosition { - const position = this._shaderPositionPool.get(); + const position = ShaderLab._shaderPositionPool.get(); position.set(index, // #if _VERBOSE line, column // #endif ); return position; } static createRange(start: ShaderPosition, end: ShaderPosition): ShaderRange { - const range = this._shaderRangePool.get(); + const range = ShaderLab._shaderRangePool.get(); range.set(start, end); return range; }🧰 Tools
🪛 Biome
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
69-77: LGTM: Improved error handling in preprocessor step.The addition of error handling logic for the preprocessor step is a good improvement. It enhances error reporting and handling in verbose mode.
Consider adding unit tests to cover this new error handling logic, especially for scenarios where
PpParser._errorscontains errors. This will help ensure the robustness of the error accumulation and logging process. Would you like assistance in creating these tests?
89-97: LGTM: Enhanced error handling in parser step.The addition of error handling logic for the parser step is a good improvement. It enhances error reporting and handling in verbose mode.
Consider adding unit tests to cover this new error handling logic, especially for scenarios where
parser.errorscontains errors or whenprogramisundefined. This will help ensure the robustness of the error accumulation and logging process. Would you like assistance in creating these tests?
107-112: LGTM: Improved error handling in code generation step.The addition of error handling logic for the code generation step is a good improvement. It enhances error reporting and handling in verbose mode.
Consider adding unit tests to cover this new error handling logic, especially for scenarios where
codeGen.errorscontains errors. This will help ensure the robustness of the error accumulation and logging process. Would you like assistance in creating these tests?
132-144: LGTM: New centralized error logging method.The addition of the
_logErrorsmethod is a good practice for centralizing error logging.There's a minor typo in the error message. Consider applying this small fix:
- Logger.error(`${errors.length} errors occur!`); + Logger.error(`${errors.length} errors occurred!`);packages/shader-lab/src/codeGen/CodeGenVisitor.ts (2)
45-50: LGTM: Error handling added for attribute and varying referencesThe addition of error handling for attribute and varying structure references is well-implemented and aligns with the PR objectives for improved error logging. The conditional error pushing ensures that errors are only stored in verbose mode.
Consider extracting the common error handling logic into a separate method to reduce code duplication:
private handleReferenceError(error: GSError | null): void { // #if _VERBOSE if (error) { this.errors.push(error); } // #endif }Then, you can use this method in both places:
this.handleReferenceError(context.referenceAttribute(prop)); // ... this.handleReferenceError(context.referenceVarying(prop));This refactoring would improve code maintainability and reduce the risk of inconsistencies in error handling.
Also applies to: 53-58
193-199: LGTM: Centralized error reporting method addedThe
reportErrormethod is a good addition that centralizes error handling and supports different compilation modes. It aligns well with the PR objectives for different error handling in Editor and Release versions.However, there's a small improvement to make:
- Remove the unnecessary comment at the end of the method.
Apply this diff to remove the comment:
protected reportError(loc: ShaderRange | ShaderPosition, message: string): void { // #if _VERBOSE this.errors.push(new GSError(GSErrorName.CompilationError, message, loc, ShaderLab._processingPassText)); // #else throw new Error(message); - // #endif }This change will improve code cleanliness without affecting functionality.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (3)
69-69: Error reset in static methodThe
resetmethod now clears the_errorsarray. This ensures a clean slate for each parsing operation.Consider using the class name instead of
thisfor clarity in static methods:- this._errors.length = 0; + ShaderContentParser._errors.length = 0;🧰 Tools
🪛 Biome
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
186-196: New error handling in _parseRenderStateDeclarationOrAssignmentThis new error handling block creates a
GSErrorand pushes it to the_errorsarray in verbose mode. It's a good improvement for detailed error reporting.Consider throwing the error in non-verbose mode for consistency:
// #if _VERBOSE this._errors.push(error); return; // #else + throw error; // #endif
Line range hint
1-527: Overall improvements and suggestions for consistencyThe changes to
ShaderContentParsersignificantly enhance error handling, especially in verbose mode. The introduction ofGSErrorand the_errorsarray provides a robust mechanism for collecting and reporting errors during parsing.However, there are some inconsistencies in error handling patterns across different methods. To improve the code:
Standardize error handling across all methods, following this pattern:
// #if _VERBOSE this._errors.push(error); return; // #else throw error; // #endifRemove
scanner.scanToCharacter(";");calls in error handling blocks to avoid skipping important content.Replace
thiswithShaderContentParserin static methods for clarity, as suggested by static analysis tools.Consider adding a method to retrieve and clear errors, which could be useful for testing and debugging.
These changes will make the code more consistent, maintainable, and easier to reason about.
packages/shader-lab/src/preprocessor/PpParser.ts (3)
Line range hint
67-101: Enhanced error handling in parse methodThe changes to the
parsemethod improve error handling and context:
- The updated return type (
string | null) allows for better error signaling.- Initializing
_scanningTextand_scanningFileprovides more context during parsing.- The error check in verbose mode ensures that parsing stops if errors are encountered.
These improvements enhance the robustness of the parsing process.
Consider adding a comment explaining the significance of returning
nullto improve code clarity:static parse(scanner: PpScanner): string | null { this._scanningText = scanner.source; this._scanningFile = scanner.file; + // Parse the input, returning null if errors are encountered in verbose mode while (!scanner.isEnd()) { // ... existing code ... } // #if _VERBOSE if (this._errors.length > 0) return null; // #endif return PpUtils.expand(this.expandSegments, scanner.source, scanner.sourceMap); }🧰 Tools
🪛 Biome
[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)
[error] 68-68: 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] 69-69: 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] 71-71: 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] 71-71: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
110-114: Centralized error reporting with new reportError methodThe addition of the
reportErrormethod is a great improvement:
- It centralizes error reporting, promoting consistency across the parser.
- Utilizes
ShaderLabUtils.createGSErrorfor standardized error creation.- Stores errors in the
_errorsarray for later processing in verbose mode.This change will make error handling more maintainable and extensible.
Consider making the method more flexible by allowing an optional
ErrorOptionsparameter:private static reportError( loc: ShaderRange | ShaderPosition, message: string, source: string, file: string, options?: { errorName?: GSErrorName, severity?: 'error' | 'warning' } ) { const error = ShaderLabUtils.createGSError( message, options?.errorName || GSErrorName.PreprocessorError, source, loc, file, options?.severity ); this._errors.push(error); }This would allow for more granular control over error reporting when needed.
140-146: Enhanced verbose mode with improved source mappingThe additions for verbose mode and source mapping are excellent improvements:
BlockInfoobjects now provide detailed context for preprocessor operations.- Source mapping in expanded macro chunks allows for better tracing of preprocessor output.
- Consistent implementation across various methods ensures comprehensive debugging information.
These enhancements will greatly aid in debugging complex shader preprocessing scenarios.
To improve code clarity, consider extracting the verbose mode checks into a helper method:
private static createVerboseBlock(file: string, blockRange: ShaderRange, sourceMap?: PpSourceMap): BlockInfo | undefined { // #if _VERBOSE return new BlockInfo(file, blockRange, sourceMap); // #else return undefined; // #endif }This would allow you to replace the repeated conditional compilation blocks with a single method call, reducing code duplication and improving readability.
Also applies to: 168-176, 203-211, 221-231, 235-245, 479-481, 489-491, 499-501, 514-516, 533-541, 553-564, 578-587, 616-625, 633-643, 695-702, 708-716
packages/shader-lab/src/common/BaseScanner.ts (1)
Line range hint
73-94: Inconsistent_advancemethod behavior with_VERBOSEflagIn the
_advance()method, line and column tracking is performed regardless of the_VERBOSEflag, but_lineand_columnare only defined when_VERBOSEis enabled. This could lead to errors when_VERBOSEis not defined.Conditionally perform line and column updates based on the
_VERBOSEflag:+ // #if _VERBOSE if (this.getCurChar() === "\n") { this._line += 1; this._column = 0; } else { this._column += 1; } + // #endif this._currentIndex++;Alternatively, define
_lineand_columnunconditionally if line and column tracking is required in both modes.packages/shader-lab/src/codeGen/GLESVisitor.ts (1)
73-73: Improve clarity of error messageThe error message:
this.reportError(paramInfo.astNode.location, `Not found attribute struct "${paramInfo.typeInfo.type}".`);could be rephrased for better readability and grammar. Consider changing it to:
this.reportError(paramInfo.astNode.location, `Attribute struct "${paramInfo.typeInfo.type}" not found.`);This small adjustment enhances the professionalism and clarity of the error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- packages/shader-lab/src/GSError.ts (1 hunks)
- packages/shader-lab/src/ShaderLab.ts (3 hunks)
- packages/shader-lab/src/ShaderLabUtils.ts (1 hunks)
- packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3 hunks)
- packages/shader-lab/src/codeGen/GLESVisitor.ts (5 hunks)
- packages/shader-lab/src/codeGen/VisitorContext.ts (3 hunks)
- packages/shader-lab/src/common/BaseScanner.ts (7 hunks)
- packages/shader-lab/src/common/BaseToken.ts (2 hunks)
- packages/shader-lab/src/common/ShaderPosition.ts (1 hunks)
- packages/shader-lab/src/contentParser/Scanner.ts (1 hunks)
- packages/shader-lab/src/contentParser/ShaderContentParser.ts (17 hunks)
- packages/shader-lab/src/lexer/Lexer.ts (4 hunks)
- packages/shader-lab/src/preprocessor/MacroDefine.ts (2 hunks)
- packages/shader-lab/src/preprocessor/PpParser.ts (29 hunks)
- packages/shader-lab/src/preprocessor/PpScanner.ts (7 hunks)
- tests/src/shader-lab/ShaderLab.test.ts (5 hunks)
- tests/src/shader-lab/test-case/source/frag.txt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/shader-lab/src/GSError.ts
- packages/shader-lab/src/codeGen/VisitorContext.ts
- packages/shader-lab/src/common/BaseToken.ts
- packages/shader-lab/src/common/ShaderPosition.ts
- packages/shader-lab/src/lexer/Lexer.ts
- packages/shader-lab/src/preprocessor/PpScanner.ts
🧰 Additional context used
🪛 Biome
packages/shader-lab/src/ShaderLab.ts
[error] 27-27: 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/ShaderLabUtils.ts
[error] 6-34: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 31-31: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 69-69: 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] 131-131: 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] 137-137: 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] 138-138: 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] 160-160: 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] 161-161: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 50-50: 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] 68-68: 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] 69-69: 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 (26)
tests/src/shader-lab/test-case/source/frag.txt (2)
Line range hint
7-9: LGTM: Conditional compilation for shadow calculationsThe addition of the
SCENE_IS_CALCULATE_SHADOWSpreprocessor directive is a good practice for optimizing shadow-related code paths. It ensures that shadow calculations are only included when bothSCENE_SHADOW_TYPEandRENDERER_IS_RECEIVE_SHADOWSare defined.
Line range hint
20-26: LGTM: Efficient shadow coordinate handlingThe conditional compilation for shadow coordinates is well-implemented. It correctly handles different scenarios:
- When shadows are not calculated, no extra code is included.
- For single cascade shadows, it declares a varying variable
v_shadowCoord.- For multiple cascade shadows, it includes an external "ShadowCoord" file.
This approach optimizes the shader code based on the specific shadow requirements of the scene.
packages/shader-lab/src/contentParser/Scanner.ts (2)
29-36: Appropriate use of _VERBOSE flag for conditional compilationThe
scanToCharactermethod is correctly wrapped with#if _VERBOSEand#endifdirectives. This addresses the previous comment about forgetting to add_VERBOSE.This approach effectively separates debug/verbose code from the release version, which can help reduce the final package size for release builds while maintaining helpful debugging tools in verbose mode.
28-36: Summary of changes and alignment with PR objectivesThe introduction of the
scanToCharactermethod, conditionally compiled based on the_VERBOSEflag, aligns well with the PR objectives of improving error logging and creating separate Release and Editor versions of the package. This approach allows for more detailed debugging in the Editor version while maintaining a smaller package size for the Release version.The implementation addresses previous concerns about infinite loops and properly utilizes conditional compilation. Consider the suggested modification to handle the end-of-source scenario more explicitly in the
scanToCharactermethod to further improve its robustness.packages/shader-lab/src/preprocessor/MacroDefine.ts (3)
3-3: LGTM: Updated imports for new error handling strategyThe import changes align with the shift from using
ParserUtils.throwtoGSErrorfor error handling, as mentioned in the AI summary. This change is consistent with the overall modifications in the file.
14-14: LGTM: Added private property for regex replacementThe addition of
_replaceRegexas a private property is appropriate. It's likely used for efficient macro expansion, which aligns with the class's purpose.
16-25: LGTM: Improved constructor with readonly properties and efficient initializationThe constructor changes enhance encapsulation and immutability by using
public readonlyfor parameters. The initialization of_replaceRegexin the constructor is efficient and appropriate. These changes also address the past review comment suggesting the use ofpublic readonly macro: BaseToken.packages/shader-lab/src/ShaderLab.ts (1)
19-20: LGTM: Improved object pooling initialization.The use of
ShaderLabUtils.createObjectPoolfor initializing static properties is a good improvement. This change likely enhances object pooling and memory management.packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3)
3-3: LGTM: Import statements updated correctlyThe new imports for
ShaderPosition,ShaderRange, and the conditional imports forGSErrorNameandGSErrorare correctly added. These changes align with the PR objectives for improved error handling and support for verbose mode.Also applies to: 6-6, 9-11
19-21: LGTM: Error storage for verbose mode addedThe addition of the
readonly errors: GSError[]property, conditionally included for verbose mode, is a good implementation. It provides a mechanism to store compilation errors, which aligns with the PR objectives for enhanced error logging in the Editor version.
Line range hint
1-199: LGTM: Implementation aligns well with PR objectivesThe changes in this file successfully implement the improved error handling and logging for ShaderLab compilation errors as described in the PR objectives. Key points:
- The code now supports both Editor (verbose) and Release (non-verbose) versions.
- In the Editor version, error messages are more descriptive, providing specific details about compilation errors.
- The Release version immediately terminates and throws an error upon encountering issues.
- The implementation allows for centralized error reporting and collection.
These changes enhance the developer experience when working with ShaderLab in the Editor version while maintaining a strict error handling approach in the Release version. The implementation is well-structured and consistent throughout the file.
To further improve the implementation, consider:
- Adding unit tests to cover the new error handling paths, especially for the
reportErrormethod and the conditional error collection invisitPostfixExpression.- Updating the documentation to reflect these new error handling capabilities and the differences between Editor and Release versions.
To ensure proper test coverage, run the following script:
Would you like assistance in generating additional unit tests or updating the documentation?
tests/src/shader-lab/ShaderLab.test.ts (3)
1-5: Import statements have been updated to reflect new ShaderLab implementationsThe import statements have been modified to remove unused imports and add new ones for ShaderLabVerbose and ShaderLabRelease. This change aligns with the updated testing strategy that involves both verbose and release versions of ShaderLab.
107-108: Appropriate initialization of both ShaderLab versionsThe creation of separate instances for ShaderLabVerbose and ShaderLabRelease is a good approach. This allows for comprehensive testing of both implementations, ensuring that they behave consistently across the test suite.
Line range hint
1-260: Overall improvements with some areas for refinementThe changes to this test file significantly enhance the test coverage by including both ShaderLabVerbose and ShaderLabRelease implementations. This dual-testing approach helps ensure consistency and compatibility between the two versions.
Key points for improvement:
- Ensure all test cases validate against both ShaderLab versions for consistency (see the "include" test case).
- Refactor the compilation-error test to use public APIs, avoid TypeScript ignores, and remove debugging code.
These refinements will further improve the robustness and maintainability of the test suite.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (4)
26-29: New imports and conditional compilationThe new imports for
GSError,GSErrorName, andShaderLabUtilsare conditionally included based on the_VERBOSEflag. This approach allows for more detailed error handling in verbose mode while keeping the release version lean.
56-56: New static error collectionThe addition of
static _errors: GSError[] = [];provides a centralized way to collect errors during parsing. This is a good practice for aggregating errors, especially useful in verbose mode.
117-117: Updated position trackingThe code now uses
scanner.getCurPosition()instead ofscanner.curPosition. This change appears to be for consistency or to use a getter method instead of directly accessing a property.Also applies to: 126-126, 133-133, 139-139, 157-157, 162-162
331-338: Error handling in _parseRenderQueueAssignmentThis error handling block uses a different pattern, throwing an error in non-verbose mode. This is good for consistency.
packages/shader-lab/src/preprocessor/PpParser.ts (2)
Line range hint
25-39: Improved class structure and error trackingThe changes to the
PpParserclass enhance its functionality and maintainability:
- Switching to a named export improves module management.
- The new static properties
_errors,_scanningText, and_scanningFileprovide better context for error tracking and debugging.These improvements will make the parser more robust and easier to maintain.
🧰 Tools
🪛 Biome
[error] 42-42: 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] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
134-135: Improved error reporting consistency and clarityThe changes to error reporting throughout the file are commendable:
- Consistent use of the new
reportErrormethod instead of throwing errors directly.- Enhanced error messages provide more context, making debugging easier.
- Errors are now collected rather than immediately thrown, allowing for more comprehensive error reporting.
These improvements will significantly enhance the developer experience when working with the shader preprocessor.
Also applies to: 309-310, 335-336, 359-360, 381-382, 403-403, 448-448, 452-452, 461-466, 603-603
packages/shader-lab/src/ShaderLabUtils.ts (1)
6-7: Class contains only static members; consider refactoring into a module with functions.The
ShaderLabUtilsclass contains only static members. To follow best practices and improve code maintainability, consider refactoring this into a module with exported functions instead of a class.🧰 Tools
🪛 Biome
[error] 6-34: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/shader-lab/src/common/BaseScanner.ts (2)
141-145:⚠️ Potential issueMissing error handling when
_VERBOSEis not definedThe
throwErrormethod always usesShaderLabUtils.createGSError, which may rely on_lineand_column. If_VERBOSEis not defined, this could cause issues in error reporting.Ensure that error creation handles both
_VERBOSEand non-_VERBOSEmodes appropriately. ModifythrowErrorto conditionally create the error:throwError(pos: ShaderPosition | ShaderRange, ...msgs: any[]) { - const error = ShaderLabUtils.createGSError(msgs.join(" "), GSErrorName.ScannerError, this._source, pos); + let error; + // #if _VERBOSE + error = ShaderLabUtils.createGSError(msgs.join(" "), GSErrorName.ScannerError, this._source, pos); + // #else + error = new Error(msgs.join(" ")); + // #endif throw error; }This ensures that error handling remains functional regardless of the
_VERBOSEflag.Likely invalid or redundant comment.
109-125: Ensure consistent position tracking in comment skipping methodsIn
skipCommentsAndSpace(), the methodgetCurPosition()is used to track positions. Ensure that position tracking works correctly when_VERBOSEis not defined, considering that_lineand_columnmay be undefined.Run the following script to check for any usage of
_lineand_columnoutside_VERBOSEblocks:Ensure that all usages of
_lineand_columnare properly guarded by_VERBOSEchecks.packages/shader-lab/src/codeGen/GLESVisitor.ts (3)
9-9: Import statement forEKeywordis appropriateThe addition of the import statement for
EKeywordensures thatEKeyword.VOIDcan be referenced later in the code. This is necessary for the return type checks.
21-23: Documentation comment added forGLESVisitorAdding the
@internalJSDoc comment appropriately marks theGLESVisitorclass as internal. This helps in generating accurate documentation and guides users on the intended usage of this class.
32-34: Ensure_VERBOSEis defined and preprocessed correctlyThe use of the preprocessor directive
// #if _VERBOSEsuggests conditional compilation based on a_VERBOSEflag. Ensure that_VERBOSEis defined somewhere in the build process or codebase, and that the build system properly handles these directives. Otherwise, this block might be incorrectly included or omitted during compilation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/shader-lab/src/common/BaseScanner.ts (7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/common/BaseScanner.ts
[warning] 120-120: packages/shader-lab/src/common/BaseScanner.ts#L120
Added line #L120 was not covered by tests
[warning] 126-126: packages/shader-lab/src/common/BaseScanner.ts#L126
Added line #L126 was not covered by tests
[warning] 139-139: packages/shader-lab/src/common/BaseScanner.ts#L139
Added line #L139 was not covered by tests
[warning] 145-146: packages/shader-lab/src/common/BaseScanner.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/shader-lab/src/preprocessor/PpParser.ts (1)
134-135: Typo in error message and improved error reportingThe use of the
reportErrormethod here ensures consistent error handling, which is great. However, there's a small typo in the error message:- `Shader slice "${includedPath}" not founded.` + `Shader slice "${includedPath}" not found.`Please correct "not founded" to "not found".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/shader-lab/src/preprocessor/PpParser.ts (28 hunks)
🧰 Additional context used
🪛 Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 50-50: 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] 68-68: 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] 69-69: 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 (19)
packages/shader-lab/src/preprocessor/PpParser.ts (19)
Line range hint
25-40: Improved error handling and parsing contextThe changes in this segment enhance the
PpParserclass by introducing new static properties for error tracking and parsing context. The use of conditional compilation for the_errorsproperty is a good approach to provide detailed debugging information in verbose mode while keeping the release version optimized.The switch from default export to named export is also a positive change, as it promotes better module management and explicit imports.
🧰 Tools
🪛 Biome
[error] 42-42: 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] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
67-69: Improved parsing context and error handlingThe changes to the
parsemethod enhance error handling by allowing anullreturn to indicate parsing errors. Setting_scanningTextand_scanningFileprovides better context for error reporting throughout the parsing process.Regarding the static analysis warning about using
thisin a static context: This is a valid use case in TypeScript for accessing static members within a class. If you prefer to make the code more explicit, you could replacethiswithPpParser, but it's not strictly necessary.🧰 Tools
🪛 Biome
[error] 68-68: 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] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
99-101: Enhanced error handling in verbose modeThis addition improves error handling in verbose mode by returning
nullwhen errors are encountered during parsing. This approach provides a clear signal to the caller that the parsing process failed, allowing for better error management and debugging in development environments.
110-114: Centralized error reportingThe new
reportErrormethod is a great addition that centralizes error reporting logic. This approach improves code maintainability and ensures consistent error object creation across the parser. It also makes it easier to modify error handling behavior in the future if needed.
Line range hint
140-148: Enhanced debugging information in verbose modeThe addition of
BlockInfocreation and usage in verbose mode is a valuable improvement. This change provides more detailed context for debugging, allowing for better tracking of code blocks during the preprocessing stage. This extra information can be crucial for identifying and resolving complex issues in shader code.
Line range hint
168-178: Consistent enhancement of debugging informationThis change follows the same pattern as previous additions, creating and using a
BlockInfoobject in verbose mode. This consistency in providing enhanced debugging information throughout the parser is commendable. It ensures that detailed context is available for all parsed blocks, which is crucial for effective debugging and error tracking in complex shader code.
Line range hint
203-213: Consistent approach to enhanced debuggingThis change continues the pattern of adding detailed debugging information in verbose mode. The consistent use of
BlockInfoobjects across different parsing scenarios is praiseworthy. This approach ensures that comprehensive context is available throughout the entire parsing process, greatly aiding in debugging and error analysis for complex shader code.
221-232: Continued consistency in debugging enhancementsThis change maintains the established pattern of adding detailed debugging information in verbose mode. The consistent approach to creating and using
BlockInfoobjects across various parsing scenarios is commendable. This uniformity in providing enhanced debugging context throughout the parser will significantly aid in troubleshooting and analyzing complex shader code issues.
Line range hint
235-246: Consistent implementation of debugging enhancementsThis change follows the established pattern of adding detailed debugging information in verbose mode. The consistent creation and use of
BlockInfoobjects across different parsing scenarios demonstrate a thorough approach to enhancing the parser's debugging capabilities. This uniformity will greatly benefit developers when troubleshooting complex shader code issues.
309-310: Improved error reporting for relational expressionsThis change enhances error reporting by using the centralized
reportErrormethod. The error message now provides more specific information about invalid operators in relational expressions. This improvement will make it easier for developers to identify and fix issues in shader code, particularly when dealing with complex expressions.
335-336: Consistent improvement in error reporting for shift expressionsThis change continues the pattern of enhancing error reporting by using the centralized
reportErrormethod. The error message now provides specific information about invalid operators in shift expressions. This consistency in improving error reporting across different expression types will significantly aid developers in identifying and resolving issues in shader code.
359-360: Consistent enhancement of error reportingThis change maintains the pattern of improving error reporting by utilizing the centralized
reportErrormethod. The error message now provides specific information about invalid operators. This consistency in enhancing error reporting across various parts of the parser will greatly assist developers in quickly identifying and addressing issues in shader code.
381-382: Continued consistency in error reporting enhancementsThis change follows the established pattern of improving error reporting by using the centralized
reportErrormethod. The error message provides specific information about invalid operators, maintaining consistency with previous improvements. This ongoing effort to enhance error reporting throughout the parser will significantly improve the debugging experience for developers working with shader code.
403-403: Consistent improvement in error reportingThis change maintains the pattern of enhancing error reporting by utilizing the centralized
reportErrormethod. The error message provides specific information about invalid operators, aligning with previous improvements. This consistency in improving error reporting across various parts of the parser will greatly aid developers in quickly identifying and resolving issues in shader code.
448-452: Enhanced error reporting for macro-related issuesThese additions improve error reporting for macro-related issues by providing more specific error messages. The use of the
reportErrormethod for both invalid function macro usage and invalid const macro is consistent with the centralized error reporting approach. These enhancements will help developers quickly identify and resolve macro-related problems in shader code, improving the overall debugging experience.
461-466: Improved error reporting for invalid tokensThis addition enhances error reporting by providing a specific error message for invalid tokens encountered during parsing. The use of the
reportErrormethod is consistent with the centralized error reporting approach implemented throughout the parser. This improvement will help developers quickly identify and address syntax issues in shader code, further enhancing the debugging capabilities of the parser.
Line range hint
479-503: Enhanced debugging capabilities with source mappingThe addition of conditional compilation blocks for verbose mode in the method signatures and return types is a valuable improvement. Including the
sourceMapproperty in verbose mode provides crucial debugging information, likely offering a mapping between the original and processed shader code. This enhancement will significantly aid developers in tracing issues back to their source in the original shader code, greatly improving the debugging experience for complex shader processing scenarios.
603-603: Consistent improvement in error reporting for macro redefinitionThis change continues the pattern of enhancing error reporting by utilizing the centralized
reportErrormethod. The error message now provides specific information about redefined macros, particularly for those starting with "GL_". This consistency in improving error reporting across various parts of the parser, including macro handling, will greatly assist developers in identifying and resolving issues in shader code.
691-692: Comprehensive debugging enhancements for macro expansionThese changes consistently apply the pattern of adding detailed debugging information in verbose mode throughout the macro expansion process. The creation and use of
BlockInfoobjects, along with their inclusion in theexpandSegmentsarray, provide valuable context for debugging complex shader code.This thorough approach to enhancing debugging capabilities across various stages of macro expansion will significantly aid developers in tracing and resolving issues related to macro processing in shader code. The consistency in implementing these improvements is commendable and will result in a more robust and developer-friendly preprocessing system.
Also applies to: 694-697, 699-701, 707-709, 712-714
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/shader-lab/src/index.ts (1)
Line range hint
1-21: Consider updating documentation for new exports and modes.The changes successfully implement the new Verbose mode and additional exports. To ensure clear understanding for users and maintainers:
- Update the package documentation to explain the difference between Release and Verbose modes.
- Clearly document the purpose and usage of the newly exported
BaseScannerandGSErrorentities.- If not already done, update any relevant README or API documentation to reflect these changes.
Would you like assistance in drafting documentation updates for these changes?
tests/src/shader-lab/Scanner.test.ts (2)
19-40: LGTM: Comprehensive test cases for various comment scenarios.The additional test cases effectively cover multiple multi-line comments and comments without leading spaces, enhancing the overall test coverage.
Consider adding a test case for inline comments (e.g.,
// single line comment) to ensure complete coverage of all comment types. Here's a suggested implementation:it("should handle inline comments", () => { const source = `// This is an inline comment nextToken`; const scanner = new BaseScanner(source); scanner.skipCommentsAndSpace(); const token = scanner.scanToken(); expect(token?.lexeme).to.equal("nextToken"); });
5-42: Well-structured test suite with good coverage.The test suite is well-organized and provides good coverage for the
skipCommentsAndSpace()method of theBaseScannerclass.Consider enhancing the test suite with the following:
- Edge cases: Test with empty input, very long comments, or nested comment-like structures.
- Error scenarios: Verify behavior with unclosed multi-line comments.
- Performance: Add a test with a large input to ensure efficient handling of extensive comments and whitespace.
Example for testing an unclosed multi-line comment:
it("should handle unclosed multi-line comments", () => { const source = `/* This comment is not closed nextToken`; const scanner = new BaseScanner(source); scanner.skipCommentsAndSpace(); const token = scanner.scanToken(); expect(token?.lexeme).to.equal("nextToken"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/shader-lab/src/index.ts (1 hunks)
- tests/src/shader-lab/Scanner.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/shader-lab/src/index.ts (3)
3-5: LGTM: New internal export for BaseScanner added.The addition of the BaseScanner export is correctly implemented. The
@internalJSDoc tag appropriately indicates that this export is intended for internal use only.
16-17: LGTM: Mode initialization updated to "Verbose".The change from "Editor" to "Verbose" mode under the
_VERBOSEdirective is consistent with the overall changes in this PR. This update aligns well with the improved error logging objectives.
7-9: Approve changes with a suggestion for directive consistency.The changes align with the PR objectives, moving from
_EDITORto_VERBOSEdirective and adding comprehensive error exports. However, ensure that all relevant parts of the codebase consistently use the_VERBOSEdirective instead of_EDITOR.To verify the consistent use of directives, run the following script:
✅ Verification successful
Adjust search commands to target TypeScript file extensions.
The previous
rgcommands failed becausetypescriptis not a recognized file type. Please run the updated scripts below to verify the usage of_EDITORand_VERBOSEdirectives within.tsand.tsxfiles.
Directive consistency verified.
All
_EDITORdirectives have been successfully replaced with_VERBOSE. No remaining_EDITORdirectives found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining _EDITOR directives that should be _VERBOSE # Test: Search for _EDITOR directives. Expect: No results if all have been updated. rg --type typescript '#if\s+_EDITOR' # Test: Confirm _VERBOSE directives are used. Expect: Results showing _VERBOSE usage. rg --type typescript '#if\s+_VERBOSE'Length of output: 148
Script:
#!/bin/bash # Description: Check for any remaining _EDITOR directives that should be _VERBOSE # Test: Search for _EDITOR directives. Expect: No results if all have been updated. rg '#if\s+_EDITOR' --glob '*.ts' --glob '*.tsx' # Test: Confirm _VERBOSE directives are used. Expect: Results showing _VERBOSE usage. rg '#if\s+_VERBOSE' --glob '*.ts' --glob '*.tsx'Length of output: 10338
tests/src/shader-lab/Scanner.test.ts (1)
7-17: LGTM: Well-structured test case for skipping multi-line comments.This test case effectively verifies that the
skipCommentsAndSpace()method correctly handles multi-line comments and proceeds to the next token.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
packages/shader-lab/src/preprocessor/MacroDefine.ts (1)
29-40: LGTM: Improved macro expansion with better error handling.The new
expandFunctionBodymethod is a good improvement over the old_expandmethod. It provides better error handling withGSErrorand uses the pre-computed_replaceRegexfor efficient expansion.Minor suggestion: Consider adding a comment explaining the expansion logic, especially the use of
_replaceRegex, to improve code readability.You could add a comment like this before the
replacecall:// Replace each argument placeholder in the macro body with the corresponding provided argument const expanded = this.body.lexeme.replace(this._replaceRegex, (_, m) => { // ... });packages/shader-lab/src/parser/ShaderTargetParser.ts (2)
118-127: Approve: Enhanced error handling with a suggestion.The new error handling logic provides more detailed information about unexpected tokens, which is valuable for debugging. The use of
ShaderLabUtils.createGSErrorensures consistent error creation, and the conditional compilation keeps the verbose error handling out of production builds.Consider adding a comment explaining why
nullis returned after pushing the error, to improve code clarity for future maintainers.
Line range hint
132-143: Approve: New debugging method with performance suggestion.The
_printStackmethod is a valuable addition for debugging in verbose mode. The use of conditional compilation ensures it doesn't impact production builds.Consider using an array to collect the string parts and then joining them at the end, which could be more efficient for large stacks:
private _printStack(nextToken: BaseToken) { const parts: string[] = []; for (let i = 0; i < this._traceBackStack.length - 1; i++) { const state = <ENonTerminal>this._traceBackStack[i++]; const token = this._traceBackStack[i]; parts.push(`State${state} - ${(<BaseToken>token).lexeme ?? ParserUtils.toString(token as GrammarSymbol)};`); } parts.push(`State${this._traceBackStack[this._traceBackStack.length - 1]} --- ${nextToken.lexeme}`); Logger.info(parts.join(' ')); }This approach could be more performant, especially for larger stack traces.
🧰 Tools
🪛 Biome
[error] 116-116: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
packages/shader-lab/src/codeGen/GLESVisitor.ts (1)
53-61: LGTM: Improved type checking and error reportingThe changes enhance type checking for return types and attribute structs, aligning with the new error reporting mechanism. The condition for non-void return types is now more explicit, improving code clarity.
Consider updating the error message on line 61 to be consistent with the one on line 56:
- this._reportError(returnType.location, "main entry can only return struct."); + this._reportError(returnType.location, "main entry can only return struct or void.");This change would make the error message consistent with the actual allowed return types.
Also applies to: 73-73
packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
Line range hint
26-524: Overall improvement with room for further refinementThe changes to the
ShaderContentParserclass significantly enhance its error handling capabilities and position tracking accuracy. The introduction of structured error objects and a centralized error collection mechanism improves debugging and error reporting.However, there are still some inconsistencies in error handling across different methods. To further improve the code:
- Standardize the error handling approach across all methods.
- Consider implementing a unified error handling method to reduce code duplication.
- Add unit tests to cover the new error handling scenarios.
These refinements will make the code more maintainable and robust.
Would you like assistance in creating a unified error handling method or generating unit tests for the new error scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3 hunks)
- packages/shader-lab/src/codeGen/GLESVisitor.ts (5 hunks)
- packages/shader-lab/src/contentParser/ShaderContentParser.ts (17 hunks)
- packages/shader-lab/src/parser/ShaderTargetParser.ts (3 hunks)
- packages/shader-lab/src/preprocessor/MacroDefine.ts (2 hunks)
🧰 Additional context used
🪛 Biome
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 69-69: 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] 131-131: 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] 137-137: 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] 138-138: 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] 160-160: 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] 161-161: 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 (17)
packages/shader-lab/src/preprocessor/MacroDefine.ts (2)
3-4: LGTM: Updated imports align with new error handling approach.The new imports for
GSError,GSErrorName, andShaderLabUtilsare consistent with the improved error handling strategy mentioned in the PR objectives. The removal ofParserUtilsimport (as noted in the AI summary) also aligns with this change.
32-34: LGTM: Improved error handling for macro expansion.The error handling in
expandFunctionBodyis well-implemented. It checks for argument count mismatch and throws aGSErrorwith a clear message, error name, and location. This aligns with the PR objectives of improving error logging and provides valuable information for debugging.packages/shader-lab/src/parser/ShaderTargetParser.ts (3)
11-17: Approve: Improved imports and conditional compilation.The new imports and conditional compilation directives enhance the module's functionality and allow for efficient builds. The use of
// #if _VERBOSEensures that verbose error reporting is only included when necessary, potentially reducing the package size for non-verbose builds.
39-44: Approve: Newerrorsgetter with a reminder.The addition of the
errorsgetter is a good improvement for error reporting in verbose mode. The use of conditional compilation ensures it doesn't impact non-verbose builds.However, there's still a potential typo in the property name
sematicAnalyzer. Consider renaming it tosemanticAnalyzerfor clarity and consistency.
118-127: Approve: Removal of unreachable code.The previously flagged unreachable code has been successfully removed, improving code clarity and addressing the issue raised in a past review comment. This change eliminates potential confusion and aligns with best practices for code maintenance.
packages/shader-lab/src/codeGen/GLESVisitor.ts (4)
9-9: LGTM: Import changes reflect updated dependenciesThe addition of
EKeywordimport and removal ofLoggerimport align with the changes in error handling approach. This modification suggests a shift towards using enum values and a different error reporting mechanism.
21-23: LGTM: Improved documentationThe addition of the
@internaldocumentation comment enhances code clarity and follows best practices for documenting internal APIs.
32-34: LGTM: Enhanced error handling for verbose modeThe addition of the
#if _VERBOSEblock improves error handling capabilities in verbose mode, aligning with the PR objective of enhancing error logging for the Editor version.
Line range hint
1-160: Overall assessment: Changes align well with PR objectivesThe modifications in this file successfully address the PR objectives by:
- Improving error logging for ShaderLab compilation errors in the Editor version.
- Implementing conditional compilation to support separate Editor and Release versions.
- Enhancing type checking and error reporting in the
vertexMainmethod.These changes contribute to a more robust and maintainable codebase, balancing the trade-offs between performance optimization and code clarity as discussed in the PR comments.
packages/shader-lab/src/codeGen/CodeGenVisitor.ts (5)
3-3: LGTM: Appropriate imports added for error handlingThe new imports for
ShaderPosition,ShaderRange,GSErrorName, andGSErrorare correctly added and properly wrapped in preprocessor directives. This allows for conditional imports based on the build configuration, which aligns with the PR objectives of supporting different compilation modes.Also applies to: 10-11
19-21: LGTM: Error collection mechanism addedThe addition of the
errorsarray as a readonly property is a good approach for collecting errors in verbose mode. This aligns well with the PR objectives of improving error logging for the Editor version.
45-50: LGTM: Improved error handling for attribute and varying structuresThe changes to
visitPostfixExpressionmethod enhance error handling for attribute and varying structure references. The implementation is consistent and aligns with the PR objectives by collecting errors only in verbose mode.Also applies to: 53-58
193-199: LGTM: Centralized error reporting mechanismThe new
_reportErrormethod effectively centralizes error reporting logic. It handles different compilation modes appropriately, creating aGSErrorin verbose mode and throwing a standardErrorotherwise. This implementation aligns well with the PR objectives of supporting different compilation modes and improving error handling.
Line range hint
1-199: LGTM: Comprehensive improvements to error handling and reportingThe changes to
CodeGenVisitor.tssuccessfully implement improved error handling and reporting mechanisms. The use of preprocessor directives to support different compilation modes is consistent and aligns well with the PR objectives. The newerrorsproperty and_reportErrormethod provide a robust framework for collecting and reporting errors in the Editor version, while maintaining efficient compilation for the Release version.These changes enhance the overall quality of the ShaderLab compilation process and should significantly improve the debugging experience for developers using the Editor version.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (3)
26-29: LGTM: Enhanced error handling mechanismThe addition of
GSErrorandGSErrorNameimports, along with the new static_errorsarray, introduces a more structured approach to error handling. This improvement will facilitate better error reporting and debugging in the shader compilation process.Also applies to: 56-57
69-69: LGTM: Proper error state resetThe addition of
this._errors.length = 0;in theresetmethod ensures that the error collection is cleared before each parsing operation. This prevents the accumulation of stale errors across multiple parse calls, maintaining the integrity of error reporting.🧰 Tools
🪛 Biome
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
117-117: LGTM: Improved position trackingThe replacement of
scanner.curPositionwithscanner.getCurPosition()across various methods is a positive change. Using a getter method ensures that the most up-to-date position is always retrieved, improving the accuracy of error reporting and code analysis.Also applies to: 126-126, 133-133, 139-139, 157-157, 162-162, 354-354, 373-373, 382-382, 388-388, 396-396, 402-402, 420-420, 425-425, 464-464, 472-472, 478-478, 501-501, 519-519, 524-524
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/shader-lab/src/preprocessor/MacroDefine.ts (3)
17-18: Approve new private properties with a minor suggestion.The addition of
_replaceRegexand_argsTextListas private properties is a good optimization. Initializing_replaceRegexin the constructor is more efficient than doing so in each method call.Consider adding JSDoc comments to these properties to improve code documentation:
/** Regular expression for replacing macro arguments */ private _replaceRegex?: RegExp; /** List of argument lexemes for quick access */ private readonly _argsTextList: string[];
32-41: ApproveexpandFunctionBodymethod with a suggestion for clarity.The new
expandFunctionBodymethod is a good improvement over the previous_expandmethod. It includes proper error checking and usesShaderLabUtils.createGSErrorfor standardized error creation. The regex replacement logic is concise and efficient.To improve clarity, consider adding a comment explaining the regex replacement logic:
// Replace each argument placeholder with its corresponding value const expanded = this.body.lexeme.replace(this._replaceRegex, (_, m) => { return args[this._argsTextList.indexOf(m)]; });This will help future maintainers understand the purpose of the replacement at a glance.
Line range hint
1-41: Approve overall improvements with a suggestion for documentation.The changes to the
MacroDefineclass significantly improve its functionality, efficiency, and error handling. These modifications align well with the PR objectives of enhancing error logging and package build processes.To complete these improvements, consider updating the class-level JSDoc comment to reflect the new behavior and usage of the
MacroDefineclass. This will help maintain up-to-date documentation for developers using this class.Example:
/** * Represents a macro definition in the shader preprocessor. * Handles both simple and function-like macros, providing methods for expansion and error handling. */ export class MacroDefine { // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/shader-lab/src/preprocessor/MacroDefine.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/shader-lab/src/preprocessor/MacroDefine.ts (1)
3-6: Verify the conditional compilation approach for imports.The
GSErrorNameimport is wrapped in a conditional compilation block for verbose mode. This approach might lead to different behavior between verbose and non-verbose modes. Consider the following:
- Ensure that
GSErrorNameis only used within conditionally compiled code blocks to prevent runtime errors in non-verbose mode.- Verify that the error handling logic remains consistent across both modes.
To check for potential issues, run the following script:
This will help identify any usage of
GSErrorNamethat might cause issues in non-verbose mode.✅ Verification successful
Conditional Compilation for
GSErrorNameVerified Successfully.No usage of
GSErrorNamefound outside the conditional compilation blocks, ensuring consistent behavior between verbose and non-verbose modes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for GSErrorName usage outside of conditional compilation blocks rg 'GSErrorName' --type typescript -g '!**/MacroDefine.ts' -C 5Length of output: 208
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/shader-lab/src/preprocessor/MacroDefine.ts (2)
9-10: LGTM: New private properties optimize macro expansion.The addition of
_replaceRegexand_argsLexemesproperties improves performance by avoiding recreation of these values on each macro expansion.Consider adding JSDoc comments to explain the purpose of these properties, especially
_argsLexemes, as it might not be immediately clear to other developers.
28-35: LGTM: ImprovedexpandFunctionBodymethod with better error handling.The new
expandFunctionBodymethod is a good replacement for the previous_expandmethod. It includes proper error checking and uses the new error handling approach withShaderLabUtils.createGSError.Consider adding a comment explaining the regex replacement logic, as it might not be immediately clear to other developers how it maps the arguments to their replacements in the macro body.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/shader-lab/src/preprocessor/MacroDefine.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/shader-lab/src/preprocessor/MacroDefine.ts (2)
3-6: LGTM: New imports enhance error handling capabilities.The addition of
ShaderLabUtilsandGSErrorNameimports improves error handling. The conditional compilation forGSErrorNameis a good practice for optimizing bundle size in non-verbose builds.
8-36: LGTM: Improved overall class structure.The changes to the
MacroDefineclass have resulted in a more streamlined and efficient structure. The removal of the_expandmethod and the introduction ofexpandFunctionBodyimprove readability and maintainability.The class now has a clearer separation of concerns, with the constructor handling initialization and
expandFunctionBodyfocusing on the expansion logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
packages/shader-lab/src/GSError.ts (1)
5-17: LGTM: Class declaration and constructor look good.The
GSErrorclass is well-structured and follows TypeScript best practices. The use ofpublic readonlyfor constructor parameters is a concise way to declare and initialize class properties.Consider adding a default value for
wrappingLineCountto make it easier to understand its purpose:static wrappingLineCount = 2; // Number of context lines to include before and after the errorpackages/shader-lab/src/codeGen/VisitorContext.ts (1)
63-75: LGTM: Improved error handling and type safetyThe changes to
referenceAttributemethod significantly improve error handling and type safety:
- Using
BaseTokeninstead ofstringprovides more context for error reporting.- The new error handling approach using
ShaderLabUtils.createGSErroris more consistent and informative.- Early return for already referenced attributes is efficient.
Consider explicitly returning
undefinedwhen the attribute is found for consistency:- if (this._referencedAttributeList[ident.lexeme]) return; + if (this._referencedAttributeList[ident.lexeme]) return undefined;This change would make the method's behavior more explicit and consistent with its return type.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (7)
26-30: LGTM! Improved error handling setupThe new imports and the addition of the
_errorsarray enhance the error management capabilities of theShaderContentParserclass. The conditional compilation for verbose mode is a good approach for debugging.For consistency with other static properties, consider using PascalCase for the
_errorsarray:- static _errors: GSError[] = []; + static Errors: GSError[] = [];This change would align with the naming convention used for
_engineType.Also applies to: 57-57
187-196: Improve consistency in error handlingThe new error handling using
ShaderLabUtils.createGSErroris good. However, consider throwing the error in non-verbose mode for consistency with other methods:// #if _VERBOSE this._errors.push(error); return; +// #else +throw error; // #endifThis change ensures that execution stops in both modes while still collecting the error in verbose mode.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 187-187: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L187
Added line #L187 was not covered by tests
[warning] 194-195: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L194-L195
Added lines #L194 - L195 were not covered by tests
244-254: Standardize error handling across methodsThe error handling here follows a similar pattern but with slight inconsistencies. Consider standardizing the approach:
- Remove
scanner.scanToCharacter(";");as it might skip important content.- Throw the error in non-verbose mode for consistent behavior.
Apply this pattern to all similar error handling blocks:
// #if _VERBOSE this._errors.push(error); -scanner.scanToCharacter(";"); return; +// #else +throw error; +// #endifThis standardization will improve code consistency and error handling reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 244-244: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L244
Added line #L244 was not covered by tests
[warning] 251-253: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L251-L253
Added lines #L251 - L253 were not covered by tests
261-273: Align error handling across all parsing methodsThe error handling in these methods follows the same pattern as the previous ones. Apply the same standardization approach to these blocks as well:
// #if _VERBOSE this._errors.push(error); -scanner.scanToCharacter(";"); return; +// #else +throw error; +// #endifThis change will ensure consistent error handling throughout the class.
Also applies to: 301-313, 332-341
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 262-262: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L262
Added line #L262 was not covered by tests
[warning] 269-271: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L269-L271
Added lines #L269 - L271 were not covered by tests
488-497: Align error handling for main entry reassignmentThe error handling for main entry reassignment differs from the pattern used elsewhere. Consider aligning it with the standard approach:
// #if _VERBOSE -Logger.error(error.toString()); -throw error; +this._errors.push(error); +return; +// #else +throw error; +// #endifThis change maintains consistency with the error handling pattern used in other parts of the class.
Line range hint
70-525: Enhance overall error handling consistencyWhile the new error handling approach is a significant improvement, there are still some inconsistencies across different methods. To further enhance the code:
- Standardize the error handling pattern across all methods, using the suggested approach in previous comments.
- Consider creating a private static method for error handling to reduce code duplication, e.g.:
private static _handleError(error: GSError) { // #if _VERBOSE this._errors.push(error); return; // #else throw error; // #endif }
- Replace all error handling blocks with calls to this method.
This refactoring will significantly improve code consistency, reduce duplication, and make future maintenance easier.
Line range hint
1-525: Overall improvement in error handling with room for further enhancementThe changes to the
ShaderContentParserclass significantly improve its error handling capabilities:
- More detailed error information is now available.
- Multiple errors can be collected in verbose mode, aiding in debugging complex shaders.
- There's a clear separation between verbose and non-verbose error handling.
These improvements enhance the overall functionality and debuggability of the shader parsing process. However, to fully capitalize on these changes:
- Implement the suggested consistency improvements across all error handling blocks.
- Consider adding unit tests to cover the new error handling scenarios, especially for the verbose mode.
- Update the class documentation to reflect the new error handling behavior, particularly the difference between verbose and non-verbose modes.
These additional steps will ensure that the new error handling mechanism is robust, well-tested, and clearly documented for future maintenance.
packages/shader-lab/src/preprocessor/PpParser.ts (5)
1-14: Approve new imports and conditional compilationThe new imports and conditional compilation for _VERBOSE mode enhance error handling and debugging capabilities. This is a positive change that improves the module's functionality.
Consider the impact on bundle size due to the conditional compilation. Ensure that the _VERBOSE mode is only enabled in development environments to keep the production bundle lean.
Line range hint
25-40: Approve class structure improvements and new static propertiesThe changes from default to named export and the addition of new static properties enhance the class structure, error tracking, and parsing context. These improvements are beneficial for debugging and maintaining the code.
Consider replacing
thiswithPpParserin static methods to improve code clarity and address the static analysis warning. For example:- this._errors.length = 0; + PpParser._errors.length = 0; - this._scanningText = scanner.source; + PpParser._scanningText = scanner.source; - this._scanningFile = scanner.file; + PpParser._scanningFile = scanner.file;This change makes the static nature of these properties more explicit.
🧰 Tools
🪛 Biome
[error] 42-42: 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] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Line range hint
67-101: Approve improved error handling and debugging capabilitiesThe changes to the
parsemethod, including the null return type and new error handling logic for _VERBOSE mode, significantly improve error handling and debugging capabilities.Consider the thread safety implications of using static properties for
_scanningTextand_scanningFile. If this class might be used in a multi-threaded environment, consider passing these values as method parameters instead of using static properties. This would improve thread safety and make the method's dependencies more explicit.- static parse(scanner: PpScanner): string | null { - this._scanningText = scanner.source; - this._scanningFile = scanner.file; + static parse(scanner: PpScanner, scanningText: string, scanningFile: string): string | null { + const _scanningText = scanningText; + const _scanningFile = scanningFile; // ... rest of the method }This change would make the method more robust in different execution contexts.
🧰 Tools
🪛 Biome
[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)
[error] 68-68: 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] 69-69: 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] 71-71: 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] 71-71: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
134-135: Approve improved error handling for missing shader slicesThe addition of error reporting for missing shader slices and the early return improve the robustness of the
_parseIncludemethod.Consider a minor improvement in the error message:
- this.reportError(id.location, `Shader slice "${includedPath}" not founded.`, scanner.source, scanner.file); + this.reportError(id.location, `Shader slice "${includedPath}" not found.`, scanner.source, scanner.file);This corrects the grammatical error in the message ("not founded" to "not found").
309-310: Approve comprehensive error reporting improvementsThe addition of multiple error reporting instances throughout the parsing logic significantly enhances the error handling capabilities of the parser. These changes provide more specific and contextual error messages, which will greatly aid in debugging and development.
For consistency and maintainability, consider creating constants for common error messages. This would make it easier to update error messages in the future and ensure consistency across the codebase. For example:
const ERROR_MESSAGES = { INVALID_OPERATOR_RELATION: "Invalid operator in relation expression.", INVALID_OPERATOR_SHIFT: "Invalid operator in shift expression.", INVALID_OPERATOR: "Invalid operator.", INVALID_FUNCTION_MACRO: "Invalid function macro usage", INVALID_CONST_MACRO: (macro: string) => `Invalid const macro: ${macro}`, // ... other error messages }; // Usage this.reportError(opPos, ERROR_MESSAGES.INVALID_OPERATOR_RELATION, scanner.source, scanner.file);This approach would centralize error messages and make them easier to manage and update.
Also applies to: 335-336, 359-360, 381-382, 403-403, 448-448, 452-452
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/shader-lab/src/GSError.ts (1 hunks)
- packages/shader-lab/src/ShaderLabUtils.ts (1 hunks)
- packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3 hunks)
- packages/shader-lab/src/codeGen/VisitorContext.ts (3 hunks)
- packages/shader-lab/src/common/BaseScanner.ts (7 hunks)
- packages/shader-lab/src/contentParser/ShaderContentParser.ts (17 hunks)
- packages/shader-lab/src/parser/SemanticAnalyzer.ts (4 hunks)
- packages/shader-lab/src/parser/ShaderTargetParser.ts (3 hunks)
- packages/shader-lab/src/preprocessor/PpParser.ts (28 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/parser/ShaderTargetParser.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/GSError.ts
[warning] 23-23: packages/shader-lab/src/GSError.ts#L23
Added line #L23 was not covered by tests
[warning] 27-27: packages/shader-lab/src/GSError.ts#L27
Added line #L27 was not covered by testspackages/shader-lab/src/codeGen/CodeGenVisitor.ts
[warning] 57-57: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L57
Added line #L57 was not covered by tests
[warning] 196-196: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L196
Added line #L196 was not covered by tests
[warning] 198-198: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L198
Added line #L198 was not covered by testspackages/shader-lab/src/codeGen/VisitorContext.ts
[warning] 83-83: packages/shader-lab/src/codeGen/VisitorContext.ts#L83
Added line #L83 was not covered by testspackages/shader-lab/src/common/BaseScanner.ts
[warning] 136-136: packages/shader-lab/src/common/BaseScanner.ts#L136
Added line #L136 was not covered by tests
[warning] 142-143: packages/shader-lab/src/common/BaseScanner.ts#L142-L143
Added lines #L142 - L143 were not covered by testspackages/shader-lab/src/contentParser/ShaderContentParser.ts
[warning] 140-140: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L140
Added line #L140 was not covered by tests
[warning] 163-163: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L163
Added line #L163 was not covered by tests
[warning] 187-187: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L187
Added line #L187 was not covered by tests
[warning] 194-195: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L194-L195
Added lines #L194 - L195 were not covered by tests
[warning] 244-244: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L244
Added line #L244 was not covered by tests
[warning] 251-253: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L251-L253
Added lines #L251 - L253 were not covered by tests
[warning] 262-262: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L262
Added line #L262 was not covered by tests
[warning] 269-271: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L269-L271
Added lines #L269 - L271 were not covered by tests
[warning] 302-302: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L302
Added line #L302 was not covered by tests
[warning] 309-311: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L309-L311
Added lines #L309 - L311 were not covered by tests
[warning] 332-332: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L332
Added line #L332 was not covered by tests
[warning] 339-340: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L339-L340
Added lines #L339 - L340 were not covered by tests
🪛 Biome
packages/shader-lab/src/ShaderLabUtils.ts
[error] 9-37: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 70-70: 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] 132-132: 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] 138-138: 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] 139-139: 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] 161-161: 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] 162-162: 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/parser/SemanticAnalyzer.ts
[error] 73-73: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 50-50: 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] 68-68: 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] 69-69: 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 (32)
packages/shader-lab/src/ShaderLabUtils.ts (4)
1-7: LGTM: Imports are well-organized and appropriate.The imports are correctly structured, including both external and local modules. The conditional import for
GSErroraligns well with its usage in the code.
12-22: ApprovecreateObjectPooland simplifyclearAllShaderLabObjectPool.The
createObjectPoolmethod is well-implemented and serves its purpose effectively.For
clearAllShaderLabObjectPool, consider simplifying the loop using afor...ofstatement as suggested in a previous review:static clearAllShaderLabObjectPool() { for (const pool of ShaderLabUtils._shaderLabObjectPoolSet) { pool.clear(); } }This change enhances readability and aligns with modern TypeScript practices.
24-36: LGTM:createGSErrormethod is well-implemented.The
createGSErrormethod effectively uses conditional compilation to provide different behavior based on the_VERBOSEflag. This approach allows for more detailed error information in verbose mode while keeping the release version lean.Note: The static analysis tool flagged line 34 as unreachable code. This is a false positive due to the conditional compilation. In the final compiled code, only one of the conditional branches will exist, making all the code reachable.
🧰 Tools
🪛 Biome
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
9-10: 🛠️ Refactor suggestionConsider refactoring to a module with functions.
The
ShaderLabUtilsclass currently only contains static members, which goes against best practices and triggers a static analysis warning. To improve code organization and maintainability, consider refactoring this into a module with exported functions instead of a class, as suggested in a previous review.Here's a suggested refactoring:
import { ClearableObjectPool, IPoolElement } from "@galacean/engine"; const shaderLabObjectPoolSet: ClearableObjectPool<IPoolElement>[] = []; export function createObjectPool<T extends IPoolElement>(type: new () => T) { // Implementation here } export function clearAllShaderLabObjectPool() { // Implementation here } export function createGSError( message: string, errorName: GSErrorName, source: string, location: ShaderRange | ShaderPosition, file?: string ) { // Implementation here }This approach maintains the same functionality while addressing the static analysis warning and potentially improving code readability and maintainability.
🧰 Tools
🪛 Biome
[error] 9-37: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/shader-lab/src/GSError.ts (2)
1-4: LGTM: Preprocessor directive and imports look good.The use of the
_VERBOSEpreprocessor directive is a good practice for controlling the inclusion of this file in different build configurations. The imports are relevant to the functionality of theGSErrorclass.
60-65: LGTM: GSErrorName enum is well-defined.The
GSErrorNameenum provides a clear categorization of error types. Its placement outside the_VERBOSEblock is appropriate, making it available in all builds, which is likely necessary for use elsewhere in the codebase.packages/shader-lab/src/parser/SemanticAnalyzer.ts (5)
3-3: LGTM: Import changes align with improved error handling.The changes to the imports, including the addition of
GSErrorName,ShaderLab, and the conditional import ofGSError, are consistent with the PR objectives to enhance error handling and support different compilation modes.Also applies to: 8-11
26-27: LGTM: Error array type updated for consistency.The change from
SemanticError[]toGSError[]for theerrorsarray, along with the conditional compilation directive, is consistent with the overall changes in error handling and the introduction of verbose mode.
45-47: LGTM: Reset method updated for verbose mode.The addition of the conditional compilation block to clear the
errorsarray in theresetmethod is consistent with the changes in error handling for verbose mode.
68-72: LGTM: Improved error handling for verbose mode.The changes to the
errormethod for verbose mode align well with the PR objectives. The use ofGSErrorwith additional context fromShaderLab._processingPassTextenhances error reporting. Returning the error instead of throwing it allows for more flexible error handling in the calling code.
73-74:⚠️ Potential issueRemove unreachable code in non-verbose mode.
The non-verbose error handling branch contains unreachable code due to the conditional compilation directive. This issue was also identified in previous reviews.
To resolve this, remove the entire
#elsebranch as it will never be executed in the compiled code. Apply this diff:// #if _VERBOSE const err = new GSError(GSErrorName.CompilationError, param.join(""), loc, ShaderLab._processingPassText); this.errors.push(err); return err; - // #else - throw new Error(param.join("")); // #endifThis change will eliminate the unreachable code warning while maintaining the intended functionality for both verbose and non-verbose modes.
🧰 Tools
🪛 Biome
[error] 73-73: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/codeGen/VisitorContext.ts (3)
5-11: LGTM: New imports enhance error handling and type safetyThe new imports, including
GSErrorName,BaseToken,ShaderLab, andShaderLabUtils, are well-aligned with the changes in thereferenceAttributeandreferenceVaryingmethods. The conditional import ofGSErrorfor verbose mode is a good practice for managing different build configurations.
41-41: LGTM: Explicit private constructorThe addition of an explicit private constructor is a good practice. It reinforces the singleton pattern and makes the class's design intent clearer.
78-90: LGTM: Consistent improvements in error handling and type safetyThe changes to
referenceVaryingmethod are consistent with those inreferenceAttributeand offer similar benefits:
- Using
BaseTokeninstead ofstringprovides more context for error reporting.- The new error handling approach using
ShaderLabUtils.createGSErroris more consistent and informative.- Early return for already referenced varyings is efficient.
Consider explicitly returning
undefinedwhen the varying is found for consistency:- if (this._referencedVaryingList[ident.lexeme]) return; + if (this._referencedVaryingList[ident.lexeme]) return undefined;This change would make the method's behavior more explicit and consistent with its return type.
The static analysis tool indicates that line 83 is not covered by tests. Let's verify the test coverage for this method:
Please review the test coverage and consider adding tests to cover the error case in the
referenceVaryingmethod if they are missing.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-83: packages/shader-lab/src/codeGen/VisitorContext.ts#L83
Added line #L83 was not covered by testspackages/shader-lab/src/codeGen/CodeGenVisitor.ts (5)
3-3: LGTM: Imports updated for new error handling functionalityThe new imports for
ShaderPosition,ShaderRange,GSErrorName, andGSErrorare correctly added to support the enhanced error handling capabilities. The conditional import ofGSErrorbased on the_VERBOSEflag aligns well with the PR objectives of creating distinct versions for Release and Editor.Also applies to: 10-13
20-22: LGTM: Newerrorsproperty for improved error loggingThe addition of the readonly
errorsproperty of typeGSError[]enhances the class's ability to collect and store errors during code generation in verbose mode. This change aligns perfectly with the PR objectives of improving error logging for the Editor version.
46-51: LGTM: Enhanced error handling invisitPostfixExpressionThe additions to the
visitPostfixExpressionmethod significantly improve error handling for attribute and varying structure references. The conditional error collection based on the_VERBOSEflag aligns well with the PR objectives of implementing different behaviors for Release and Editor versions. These changes contribute to the overall improvement in error logging as mentioned in the PR summary.Also applies to: 54-59
194-200: LGTM: Centralized error reporting with_reportErrormethodThe new
_reportErrormethod effectively centralizes error reporting logic, enhancing code maintainability. Its conditional behavior based on the_VERBOSEflag aligns perfectly with the PR objectives of implementing different error handling for Release and Editor versions. This implementation is consistent with the PR summary's description of error handling improvements.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 196-196: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L196
Added line #L196 was not covered by tests
[warning] 198-198: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L198
Added line #L198 was not covered by tests
57-57: Improve test coverage for conditional compilation pathsThe static analysis tool indicates that some lines in conditional blocks (57, 196, 198) are not covered by tests. These lines are crucial for the different behavior in Release and Editor versions. To ensure robustness and prevent potential regressions, consider adding tests that cover both compilation modes.
To verify the current test coverage and identify gaps, you can run the following command:
This script will help identify existing tests and potential gaps in coverage for the different compilation modes.
Also applies to: 196-196, 198-198
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-57: packages/shader-lab/src/codeGen/CodeGenVisitor.ts#L57
Added line #L57 was not covered by testspackages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
70-70: LGTM! Proper error state resetClearing the
_errorsarray in theresetmethod ensures that errors from previous parsing operations don't persist. This is a good practice for maintaining the clean state of the parser between operations.🧰 Tools
🪛 Biome
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/shader-lab/src/preprocessor/PpParser.ts (6)
110-114: Approve centralized error reporting methodThe addition of the
reportErrormethod is a positive change. It centralizes error reporting, improving consistency across the class and leveraging the new GSError type for more structured error handling.This change enhances the overall error handling strategy of the class.
Line range hint
140-148: Approve enhanced debugging capabilitiesThe addition of the BlockInfo object and its inclusion in expandSegments for _VERBOSE mode enhances the debugging capabilities of the parser. This change provides more context for error reporting and debugging, which is valuable for development and troubleshooting.
These changes will significantly aid in diagnosing issues during development.
461-466: Approve improved error reporting for invalid tokensThe addition of error reporting for invalid tokens in constant parsing is a valuable improvement. Including the problematic character in the error message provides crucial information for debugging.
This change will make it easier to identify and fix issues related to unexpected tokens in constant expressions.
Line range hint
479-503: Approve enhanced debugging with source mappingThe addition of the sourceMap property to the return type in _VERBOSE mode is a valuable enhancement. This change will provide better context for mapping preprocessed code to the original source, significantly aiding in debugging and development processes.
These changes demonstrate a thoughtful approach to improving the development experience while maintaining a lean production build.
Line range hint
553-565: Approve consistent implementation of debugging enhancementsThe addition of conditional compilation for _VERBOSE mode and the creation of BlockInfo objects across various methods demonstrate a consistent approach to enhancing debugging capabilities. These changes provide more detailed context for debugging and error tracking throughout the parsing process.
This consistent implementation will greatly improve the ability to diagnose and fix issues during development, while keeping the production build optimized.
Also applies to: 578-587, 616-625, 633-644
691-715: Approve improvements in range creation and debugging capabilitiesThe use of
ShaderLab.createRangefor creating range objects improves consistency throughout the parser. The addition of conditional compilation for _VERBOSE mode and the creation of BlockInfo objects further enhance the debugging capabilities of the parser.These changes contribute to a more robust and developer-friendly parsing system, with improved consistency and better debugging tools.
packages/shader-lab/src/common/BaseScanner.ts (6)
28-31: Conditional compilation of_lineand_columnWrapping
_lineand_columnunder the_VERBOSEflag ensures that line and column tracking is only included in verbose builds. This optimizes performance by avoiding unnecessary property initialization in production environments.
41-46: RefactoredgetCurPosition()with conditional parametersThe modification of
getCurPosition()to include_lineand_columnonly when_VERBOSEis defined is appropriate. This ensures that position information is accurate in verbose mode while maintaining efficiency in production builds.
85-93: Optimized_advance()method with conditional line and column updatesIncluding the line and column updates within the
_VERBOSEconditional block in the_advance()method is a good approach. It prevents unnecessary computations in non-verbose builds, enhancing performance.
136-136: Add unit tests for the new error handling logicThe invocation of
throwErrorinscanTextintroduces a new error handling path that is not currently covered by tests, as indicated by the code coverage report. Adding unit tests for this code path will ensure that errors are handled correctly and improve overall test coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 136-136: packages/shader-lab/src/common/BaseScanner.ts#L136
Added line #L136 was not covered by tests
141-144: Introduction ofthrowErrormethod enhances error handlingThe new
throwErrormethod centralizes error creation and throwing logic, improving maintainability and consistency in error handling throughout theBaseScannerclass.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 142-143: packages/shader-lab/src/common/BaseScanner.ts#L142-L143
Added lines #L142 - L143 were not covered by tests
51-57:⚠️ Potential issuePotential access of undefined properties
_lineand_columnThe getters
lineandcolumnaccess_lineand_column, which are only defined under the_VERBOSEflag. When_VERBOSEis not defined, accessing these properties will result inundefinedor cause errors.Consider wrapping the getters with the
_VERBOSEconditional compilation to prevent access when the properties are not defined:+ // #if _VERBOSE get line() { return this._line; } get column() { return this._column; } + // #endifLikely invalid or redundant comment.
packages/shader-lab/src/GSError.ts
Outdated
| const lines = source.split("\n"); | ||
|
|
||
| let diagnosticMessage = `${this.name}: ${message}\n\n`; | ||
| const lineSplit = "|···"; | ||
|
|
||
| for (let i = start.line - GSError.wrappingLineCount, n = end.line + GSError.wrappingLineCount; i <= n; i++) { | ||
| diagnosticMessage += lineSplit + `${lines[i]}\n`; | ||
|
|
||
| if (i < start.line || i > end.line) continue; | ||
|
|
||
| let remarkStart = 0; | ||
| let remarkEnd = lines[i].length; | ||
| let paddingLength = lineSplit.length; | ||
| if (i === start.line) { | ||
| remarkStart = start.column; | ||
| paddingLength += start.column; | ||
| } else if (i === end.line) { | ||
| remarkEnd = end.column; | ||
| } | ||
| const remarkLength = Math.max(remarkEnd - remarkStart, 1); | ||
|
|
||
| diagnosticMessage += " ".repeat(paddingLength) + "^".repeat(remarkLength) + "\n"; | ||
| } | ||
|
|
||
| return diagnosticMessage; | ||
| } |
There was a problem hiding this comment.
Add bounds checking to prevent potential runtime errors.
The error message formatting logic is comprehensive and provides clear visual indication of the error location. However, there's a potential issue with accessing lines[i] without bounds checking, which could lead to runtime errors if i is out of bounds.
To prevent potential runtime errors, add bounds checking before accessing lines[i]:
for (let i = start.line - GSError.wrappingLineCount, n = end.line + GSError.wrappingLineCount; i <= n; i++) {
+ if (i >= 0 && i < lines.length) {
diagnosticMessage += lineSplit + `${lines[i]}\n`;
// ... rest of the loop body ...
+ }
}This change ensures that we only access valid indices of the lines array, preventing potential runtime errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lines = source.split("\n"); | |
| let diagnosticMessage = `${this.name}: ${message}\n\n`; | |
| const lineSplit = "|···"; | |
| for (let i = start.line - GSError.wrappingLineCount, n = end.line + GSError.wrappingLineCount; i <= n; i++) { | |
| diagnosticMessage += lineSplit + `${lines[i]}\n`; | |
| if (i < start.line || i > end.line) continue; | |
| let remarkStart = 0; | |
| let remarkEnd = lines[i].length; | |
| let paddingLength = lineSplit.length; | |
| if (i === start.line) { | |
| remarkStart = start.column; | |
| paddingLength += start.column; | |
| } else if (i === end.line) { | |
| remarkEnd = end.column; | |
| } | |
| const remarkLength = Math.max(remarkEnd - remarkStart, 1); | |
| diagnosticMessage += " ".repeat(paddingLength) + "^".repeat(remarkLength) + "\n"; | |
| } | |
| return diagnosticMessage; | |
| } | |
| const lines = source.split("\n"); | |
| let diagnosticMessage = `${this.name}: ${message}\n\n`; | |
| const lineSplit = "|···"; | |
| for (let i = start.line - GSError.wrappingLineCount, n = end.line + GSError.wrappingLineCount; i <= n; i++) { | |
| if (i >= 0 && i < lines.length) { | |
| diagnosticMessage += lineSplit + `${lines[i]}\n`; | |
| if (i < start.line || i > end.line) continue; | |
| let remarkStart = 0; | |
| let remarkEnd = lines[i].length; | |
| let paddingLength = lineSplit.length; | |
| if (i === start.line) { | |
| remarkStart = start.column; | |
| paddingLength += start.column; | |
| } else if (i === end.line) { | |
| remarkEnd = end.column; | |
| } | |
| const remarkLength = Math.max(remarkEnd - remarkStart, 1); | |
| diagnosticMessage += " ".repeat(paddingLength) + "^".repeat(remarkLength) + "\n"; | |
| } | |
| } | |
| return diagnosticMessage; | |
| } |
| override toString(): string { | ||
| let start: ShaderPosition, end: ShaderPosition; | ||
| const { message, location, source } = this; | ||
| if (!source) { | ||
| return message; | ||
| } | ||
|
|
||
| if (location instanceof ShaderPosition) { | ||
| start = end = location; | ||
| } else { | ||
| start = location.start; | ||
| end = location.end; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test Coverage Missing for toString Method Scenarios
The toString method in GSError.ts lacks test coverage for the following scenarios:
- When
sourceis falsy. - When
locationis an instance ofShaderPosition.
Please add test cases to cover these scenarios to ensure consistent return types and comprehensive validation.
🔗 Analysis chain
Ensure consistent return type and improve test coverage.
The toString method handles different location types well. However, there are two points to consider:
- The early return when
sourceis falsy might lead to inconsistent return types. Consider modifying it to always return a string:
if (!source) {
return `${this.name}: ${message}`;
}- Static analysis indicates that lines 23 and 27 are not covered by tests. Consider adding test cases to cover these scenarios:
- When
sourceis falsy - When
locationis an instance ofShaderPosition
- When
To verify the test coverage, you can run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for GSError.ts
# Test: Look for test files related to GSError
fd -e test.ts -e spec.ts GSError
# Test: Check for test cases covering falsy source and ShaderPosition
rg -i 'test|describe|it.*gsError.*source.*falsy' $(fd -e test.ts -e spec.ts)
rg -i 'test|describe|it.*gsError.*ShaderPosition' $(fd -e test.ts -e spec.ts)
Length of output: 185777
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: packages/shader-lab/src/GSError.ts#L23
Added line #L23 was not covered by tests
[warning] 27-27: packages/shader-lab/src/GSError.ts#L27
Added line #L27 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/shader-lab/src/common/BaseScanner.ts (2)
41-48: LGTM: Improved method naming and conditional compilation.The renaming from
curPositiontogetCurPositionenhances method naming consistency. The conditional compilation for_VERBOSEmode aligns with the PR objective of producing distinct versions.Consider using template literals for better readability:
return ShaderLab.createPosition( this._currentIndex, // #if _VERBOSE this._line, this._column // #endif );
143-146: LGTM: New centralized error handling method.The
throwErrormethod provides a centralized way to create and throwGSErrors, which should lead to more consistent error handling across the codebase.However, the static analysis indicates that this method is not covered by tests. Consider adding unit tests to ensure proper functionality and maintain code coverage.
Would you like assistance in writing unit tests for the
throwErrormethod?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 144-145: packages/shader-lab/src/common/BaseScanner.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/shader-lab/src/common/BaseScanner.ts (7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/common/BaseScanner.ts
[warning] 138-138: packages/shader-lab/src/common/BaseScanner.ts#L138
Added line #L138 was not covered by tests
[warning] 144-145: packages/shader-lab/src/common/BaseScanner.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
🔇 Additional comments (6)
packages/shader-lab/src/common/BaseScanner.ts (6)
2-2: LGTM: New import for error handling.The addition of
GSErrorNameimport aligns with the PR objective of improving error logging for ShaderLab compilation errors.
51-59: LGTM: Added getters for line and column.The new getters for
lineandcolumnimprove encapsulation and provide a clean interface for accessing these properties in verbose mode. The conditional compilation ensures these are only available when needed.
112-112: LGTM: Updated method call for consistency.The change from
curPositiontogetCurPositionmaintains consistency with the earlier method renaming.Also applies to: 117-117, 119-119, 125-125
138-138: LGTM: Improved error handling in scanText.The use of the new
throwErrormethod aligns with the PR objective of improving error handling. This change likely provides more detailed and consistent error reporting across the codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 138-138: packages/shader-lab/src/common/BaseScanner.ts#L138
Added line #L138 was not covered by tests
176-176: LGTM: Updated method call for consistency.The change from
curPositiontogetCurPositionmaintains consistency with the earlier method renaming.Also applies to: 179-179
87-95: LGTM: Simplified line and column tracking.Removing the conditional compilation around line and column updates simplifies the code and ensures consistent tracking. However, consider the performance impact of always updating these values, especially for large files.
You might want to benchmark this change to ensure it doesn't significantly affect performance in non-verbose mode.
✅ Verification successful
Verified: Line and column tracking consistent.
The removal of conditional compilation around
_lineand_columninBaseScanner.tsensures consistent tracking, aligning with their usage inLexer.ts. No additional impacts detected based on the current codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of _line and _column outside of _VERBOSE mode rg --type typescript -g '!*.test.ts' '\b_line\b|\b_column\b' packages/shader-lab/srcLength of output: 121
Script:
#!/bin/bash # Verify the usage of _line and _column outside of _VERBOSE mode rg --type ts -g '!*.test.ts' '\b_line\b|\b_column\b' packages/shader-lab/srcLength of output: 895
Script:
#!/bin/bash # Examine the context of _line and _column usage in Lexer.ts and other relevant files ast-grep --lang typescript --pattern $'_line = $_' packages/shader-lab/src/lexer/Lexer.ts ast-grep --lang typescript --pattern $'_column = $_' packages/shader-lab/src/lexer/Lexer.tsLength of output: 183
Script:
#!/bin/bash # Search for assignments to _line and _column in Lexer.ts and BaseScanner.ts rg --type ts -g '!*.test.ts' 'this\._line\s*=' packages/shader-lab/src/lexer/Lexer.ts rg --type ts -g '!*.test.ts' 'this\._column\s*=' packages/shader-lab/src/lexer/Lexer.ts rg --type ts -g '!*.test.ts' 'this\._line\s*=' packages/shader-lab/src/common/BaseScanner.ts rg --type ts -g '!*.test.ts' 'this\._column\s*=' packages/shader-lab/src/common/BaseScanner.tsLength of output: 463
packages/shader-lab/src/GSError.ts
Outdated
| const lineSplit = "|···"; | ||
|
|
||
| for (let i = start.line - GSError.wrappingLineCount, n = end.line + GSError.wrappingLineCount; i <= n; i++) { | ||
| diagnosticMessage += lineSplit + `${lines[i]}\n`; |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
change rollup config to produce 2 version
ShaderLabpackage dist:ReleaseandEditor.Improve the error log when compiation error of ShaderLab occur in
Editorversion.related issue: #2248
Summary by CodeRabbit
Summary by CodeRabbit
New Features
PreprocessorError,CompilationError, andScannerError.verboseversion of ShaderLab for improved diagnostic information during development.Chores