feat(shader-lab): make #define values first-class AST nodes#2974
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds structured expression-style Changes
Sequence DiagramsequenceDiagram
participant Lexer as Lexer
participant Parser as Parser
participant SA as SemanticAnalyzer
participant Pre as Preprocessor
participant GLES as GLESVisitor
participant CGV as CodeGenVisitor
rect rgba(100,150,200,0.5)
note over Lexer,Parser: Lexer emits structured macro tokens when expression-mode detected
Lexer->>Parser: Token stream (MACRO_DEFINE / MACRO_DEFINE_PARAMS? / value / MACRO_DEFINE_END)
Parser->>SA: Build ASTNode.MacroDefine (valueExpression optional)
SA->>Pre: Register macro with valueAst in macroDefineList
end
rect rgba(150,200,120,0.5)
note over GLES,CGV: Pre-pass registers struct-var roles and macro member refs before codegen
GLES->>GLES: _collectAllStructVars() infer/register struct var roles across stages
GLES->>GLES: _preRegisterGlobalMacroRefs() walk macro valueAst and register member refs
GLES->>CGV: Invoke codegen for program
CGV->>CGV: visitMacroDefine() emits expression or legacy `#define`
CGV->>CGV: visitPostfixExpression() resolves struct role via VisitorContext.getStructRole()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/shader-lab/src/codeGen/GLESVisitor.ts (1)
356-364:⚠️ Potential issue | 🟡 MinorSilent skip on falsy
codeGenresult may mask regressions.The previous contract was that every referenced global produces text; now an empty/undefined return is silently swallowed (no
out.push, no warning). If a future bug causes a symbol'scodeGento return""the resulting GLSL will be missing a declaration with no diagnostic. Consider either:
- Only allowing the skip when
sm.astNodeexplicitly opts in (e.g., a sentinel), or- Adding a
//#if_VERBOSELogger.warnwhen the skip is hit, so regressions are at least observable in dev builds.Not blocking, but worth a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/codeGen/GLESVisitor.ts` around lines 356 - 364, The loop currently silently skips when sm.astNode.codeGen(this) returns a falsy value, which can hide missing declarations; update the logic in GLESVisitor where codeGenResult is checked (the call to sm.astNode.codeGen and subsequent handling before out.push) to either: 1) only skip when the AST node explicitly opts in via a sentinel/property (e.g., sm.astNode.allowEmptyCodeGen or similar), or 2) emit a dev-only warning using the existing logger (e.g., Logger.warn or a // `#if` _VERBOSE block) when codeGenResult is falsy and sm.astNode does not opt in; ensure you reference sm.isInMacroBranch and ESymbolType.VAR behavior remains unchanged and keep pushing the text to out only when valid or explicitly allowed.packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
160-177:⚠️ Potential issue | 🟠 MajorPreserve arguments for AST-backed function macros.
visitMacroDefineemits function-like macros with their original parameter list, so filtering struct-role args here can turnGET_UV(o)intoGET_UV()while the emitted directive still expects one parameter. Gate this legacy argument-dropping path behind!node.hasAstValue, or add call-aware rewriting for AST macro parameters.🐛 Proposed guard for AST-form macro calls
- const params = astNodes.filter((node) => { - if (node instanceof ASTNode.AssignmentExpression) { - const variableParam = ParserUtils.unwrapNodeByType<ASTNode.VariableIdentifier>( - node, - NoneTerminal.variable_identifier - ); - if ( - variableParam && - typeof variableParam.typeInfo === "string" && - context.getStructRole(variableParam.typeInfo) - ) { - return false; - } - } - - return true; - }); + const params = node.hasAstValue + ? astNodes + : astNodes.filter((node) => { + if (node instanceof ASTNode.AssignmentExpression) { + const variableParam = ParserUtils.unwrapNodeByType<ASTNode.VariableIdentifier>( + node, + NoneTerminal.variable_identifier + ); + if ( + variableParam && + typeof variableParam.typeInfo === "string" && + context.getStructRole(variableParam.typeInfo) + ) { + return false; + } + } + + return true; + });Also applies to: 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/codeGen/CodeGenVisitor.ts` around lines 160 - 177, The current parameter filtering in CodeGenVisitor removes struct-role arguments by inspecting ASTNode.AssignmentExpression via ParserUtils.unwrapNodeByType and can drop parameters for AST-backed macros that still expect them; modify the filter to skip this legacy argument-dropping when the call has AST-backed values by checking node.hasAstValue and only apply the struct-role exclusion when !node.hasAstValue (also apply the same guard to the analogous filter at the other occurrence mentioned); keep the rest of the logic (using VisitorContext.context and NoneTerminal.variable_identifier) unchanged so function-like macros emitted by visitMacroDefine preserve their original parameter lists when node.hasAstValue is true.
🧹 Nitpick comments (2)
packages/shader-lab/src/common/SymbolTable.ts (1)
58-66: Consider aligning macro-branch semantics with the other accessors.Unlike
getSymbol/_getSymbols, which default to skippingisInMacroBranchentries,forEachunconditionally yields every symbol. The current sole caller (GLESVisitor._collectAllStructVars) doesn't filter onisInMacroBranch, so module-level globals introduced inside conditional macro branches will all be registered into_structVarMap, potentially mixing roles from mutually exclusive branches. If that's intentional, a short doc note would help; otherwise consider anincludeMacro = falseparameter for consistency.- /** Iterate every registered symbol. Order within a name bucket is insertion order. */ - forEach(callback: (symbol: T) => void): void { + /** Iterate every registered symbol. Order within a name bucket is insertion order. */ + forEach(callback: (symbol: T) => void, includeMacro = false): void { for (const entries of this._table.values()) { for (let i = 0, n = entries.length; i < n; i++) { - callback(entries[i]); + const item = entries[i]; + if (!includeMacro && item.isInMacroBranch) continue; + callback(item); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/common/SymbolTable.ts` around lines 58 - 66, The forEach method currently yields all symbols including those with isInMacroBranch, which differs from getSymbol/_getSymbols; update SymbolTable.forEach to accept an optional includeMacro: boolean = false parameter and, when includeMacro is false, skip entries where entry.isInMacroBranch is true; update callers (e.g., GLESVisitor._collectAllStructVars which populates _structVarMap) to pass includeMacro=true if they intentionally want macro-branch symbols or leave the default to exclude them, or add a brief doc comment on forEach explaining the new parameter and default behavior.packages/shader-lab/src/codeGen/VisitorContext.ts (1)
153-171: Nit: role label is redundant withrefListidentity.
_referencePropalready receives the specificlistandrefList; theroleparameter is used solely for the human-readable error string. That's fine, but sincelist,refList, androlemust be kept in sync by every caller, one small simplification would be to pass onlyroleand look up both lists inside (as done inreferenceStructPropByName). Not blocking — current API is explicit and fine to keep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/codeGen/VisitorContext.ts` around lines 153 - 171, The _referenceProp function takes a redundant role parameter that callers must keep in sync with the list/refList pair; simplify by changing callers to pass only the role (as done by referenceStructPropByName) and have _referenceProp derive the correct list and refList internally based on that role (use the same lookup logic as referenceStructPropByName), then remove the role parameter from the signature and update references to use the internally-resolved list/refList and keep the same error message generation via ShaderLabUtils.createGSError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shader-lab/src/codeGen/GLESVisitor.ts`:
- Around line 295-299: The method signature for _extractLocalVarNames currently
spans multiple lines and violates Prettier; collapse the parameter list onto a
single line so the declaration reads as one line (e.g.,
_extractLocalVarNames(node: ASTNode.InitDeclaratorList, context: VisitorContext,
role: StructRole): void) while keeping the same parameter names and types and
leaving the method body unchanged.
In `@packages/shader-lab/src/codeGen/VisitorContext.ts`:
- Around line 111-123: The call inside referenceVarying should be formatted on a
single line like referenceMRTProp to satisfy Prettier: update the return
statement in referenceVarying so the invocation of this._referenceProp uses
one-line argument formatting (same pattern as referenceMRTProp), keeping the
same arguments ( "varying", ident.lexeme, this.varyingList,
this._referencedVaryingList, ident.location ) and preserving the function name
referenceVarying and method _referenceProp.
In `@packages/shader-lab/src/lexer/Lexer.ts`:
- Around line 557-560: Prettier formatting is causing lint failures around the
conditional using Lexer._lexemeTable, Lexer._expressionValueLeaderKeywords and
the variable firstWord; reformat the conditional expressions (and the similar
occurrence around the block referencing the same symbols at the later location)
to match project Prettier rules — e.g., put operators and operands on the same
line or break before the operator consistently so the two multipart conditions
are formatted cleanly and consistently, then run Prettier (or your formatter) to
update the file and commit the result.
- Around line 697-703: When _macroDefineExpectsNameToken is true and we clear
it, only set _macroDefineExpectsParamsToken (or call _scanMacroDefineParams) if
the scanner's next raw character is an immediate '(' with no intervening
whitespace; that is, after handling the macro name check the next raw buffer
char equals '(' before setting this._macroDefineExpectsParamsToken (or invoking
_scanMacroDefineParams), and do not rely on any whitespace-skipping logic or
lookahead that skips spaces; leave behavior unchanged when there is whitespace
(e.g. "#define FOO (1 + 2)" should not become function-like).
---
Outside diff comments:
In `@packages/shader-lab/src/codeGen/CodeGenVisitor.ts`:
- Around line 160-177: The current parameter filtering in CodeGenVisitor removes
struct-role arguments by inspecting ASTNode.AssignmentExpression via
ParserUtils.unwrapNodeByType and can drop parameters for AST-backed macros that
still expect them; modify the filter to skip this legacy argument-dropping when
the call has AST-backed values by checking node.hasAstValue and only apply the
struct-role exclusion when !node.hasAstValue (also apply the same guard to the
analogous filter at the other occurrence mentioned); keep the rest of the logic
(using VisitorContext.context and NoneTerminal.variable_identifier) unchanged so
function-like macros emitted by visitMacroDefine preserve their original
parameter lists when node.hasAstValue is true.
In `@packages/shader-lab/src/codeGen/GLESVisitor.ts`:
- Around line 356-364: The loop currently silently skips when
sm.astNode.codeGen(this) returns a falsy value, which can hide missing
declarations; update the logic in GLESVisitor where codeGenResult is checked
(the call to sm.astNode.codeGen and subsequent handling before out.push) to
either: 1) only skip when the AST node explicitly opts in via a
sentinel/property (e.g., sm.astNode.allowEmptyCodeGen or similar), or 2) emit a
dev-only warning using the existing logger (e.g., Logger.warn or a // `#if`
_VERBOSE block) when codeGenResult is falsy and sm.astNode does not opt in;
ensure you reference sm.isInMacroBranch and ESymbolType.VAR behavior remains
unchanged and keep pushing the text to out only when valid or explicitly
allowed.
---
Nitpick comments:
In `@packages/shader-lab/src/codeGen/VisitorContext.ts`:
- Around line 153-171: The _referenceProp function takes a redundant role
parameter that callers must keep in sync with the list/refList pair; simplify by
changing callers to pass only the role (as done by referenceStructPropByName)
and have _referenceProp derive the correct list and refList internally based on
that role (use the same lookup logic as referenceStructPropByName), then remove
the role parameter from the signature and update references to use the
internally-resolved list/refList and keep the same error message generation via
ShaderLabUtils.createGSError.
In `@packages/shader-lab/src/common/SymbolTable.ts`:
- Around line 58-66: The forEach method currently yields all symbols including
those with isInMacroBranch, which differs from getSymbol/_getSymbols; update
SymbolTable.forEach to accept an optional includeMacro: boolean = false
parameter and, when includeMacro is false, skip entries where
entry.isInMacroBranch is true; update callers (e.g.,
GLESVisitor._collectAllStructVars which populates _structVarMap) to pass
includeMacro=true if they intentionally want macro-branch symbols or leave the
default to exclude them, or add a brief doc comment on forEach explaining the
new parameter and default behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f15326e1-baf6-40ea-a3bb-3cfa28dcacbd
⛔ Files ignored due to path filters (8)
tests/src/shader-lab/expected/define-struct-access-global.frag.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access-global.vert.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access.frag.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access.vert.glslis excluded by!**/*.glsltests/src/shader-lab/shaders/define-struct-access-global.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/define-struct-access.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/global-varying-var.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/macro-member-access-builtin-arg.shaderis excluded by!**/*.shader
📒 Files selected for processing (12)
packages/shader-lab/src/ParserUtils.tspackages/shader-lab/src/Preprocessor.tspackages/shader-lab/src/codeGen/CodeGenVisitor.tspackages/shader-lab/src/codeGen/GLESVisitor.tspackages/shader-lab/src/codeGen/VisitorContext.tspackages/shader-lab/src/common/SymbolTable.tspackages/shader-lab/src/common/enums/Keyword.tspackages/shader-lab/src/lalr/CFG.tspackages/shader-lab/src/lexer/Lexer.tspackages/shader-lab/src/parser/AST.tspackages/shader-lab/src/parser/GrammarSymbol.tstests/src/shader-lab/ShaderLab.test.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2974 +/- ##
===========================================
+ Coverage 77.79% 78.06% +0.26%
===========================================
Files 906 906
Lines 99074 99892 +818
Branches 10030 10181 +151
===========================================
+ Hits 77077 77980 +903
+ Misses 21828 21743 -85
Partials 169 169
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Lift expression-style `#define` values from opaque lexemes into proper
`AssignmentExpression` AST subtrees that flow through the normal visitor
pipeline. Struct member access inside a macro value (e.g.
`#define FSInput_worldNormal v.v_normal.xyz`) now participates in varying
flattening, type inference, and reference tracking the same way inline
expressions do — no regex rewrite, no parallel symbol tracking.
Aligns with how modern shader compilers (glslang, Slang, Clang/DXC)
connect preprocessor output with the main parser.
Lexer
- `#define` with an expression-shaped value emits a token stream:
`MACRO_DEFINE`, `ID`, optional `MACRO_DEFINE_PARAMS`, value tokens,
`MACRO_DEFINE_END`. The `(params)` block is captured as a single
opaque token so the CFG rule stays LALR(1)-friendly.
- Non-expression macros (type/qualifier/partial-syntax forms like
`#define TYPE_ALIAS highp vec3`) fall back to the legacy opaque
`MACRO_DEFINE_EXPRESSION` path, detected by a cheap first-keyword
peek against the existing lexeme table.
Grammar / AST
- New `macro_define` CFG rule and `ASTNode.MacroDefine` node carrying
an optional `AssignmentExpression` value.
- `MacroCallSymbol.hasAstValue` flags AST-form macros so
`VariableIdentifier.semanticAnalyze` can keep the call site's type
as TypeAny instead of leaking the root variable's struct type into
the call site (which broke builtin overload resolution in the
Cocos FSInput pattern).
- `MacroDefineInfo.valueAst` carries the AST on the preprocessor
entry; legacy fields stay untouched for the opaque path.
Codegen
- `visitMacroDefine` emits `#define NAME[(params)] <value>` with the
value produced by `AssignmentExpression.codeGen` — member access
rewriting happens for free inside `visitPostfixExpression`.
- `GLESVisitor._collectAllStructVars` preloads `_structVarMap` with
every variable whose type carries an IO role (entry function
params/locals, module-level globals) before either stage's codegen
so forward-declared globals (e.g. `Varyings o;` appearing after a
`#define` that references `o`) are rewritten correctly in both the
vertex and fragment outputs.
- `visitPostfixExpression` consults `_structVarMap` by the left-side's
bare identifier in addition to the AST's static type, covering the
forward-declaration case without breaking swizzle access on nested
expressions.
- `getStructRole` unifies attribute/varying/mrt classification in the
handful of places that used three parallel `isXxxStruct` checks.
Supporting changes
- `SymbolTable.forEach` public API replaces an `(any)._table` access.
- `ParserUtils.extractDirectIdentLexeme` /
`ParserUtils.parseMacroParamList` host small helpers that shouldn't
live on the visitor.
- Lexer `#define` state machine flags renamed for clarity
(`_macroDefineExpectsNameToken`, `_macroDefineExpectsParamsToken`).
Tests
- Four new shaders + expected snapshots cover the Cocos member-access
patterns: macro values referencing vertex locals, fragment params,
module-level Varyings globals, and builtin-function arguments.
All 1294 tests pass, including the 16 shader-lab cases.
909ee1c to
65e79b3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/shader-lab/src/lexer/Lexer.ts (1)
696-703:⚠️ Potential issue | 🟠 Major
#define FOO (1 + 2)is still mis-tokenized as function-like.Line 702 unconditionally sets
_macroDefineExpectsParamsToken = truewhenever we're inside a#define. Because the nextscanToken()call runs_skipInlineSpaceAndComments()first (line 126), any whitespace between the macro name and(is swallowed, and the(…)is then captured by_scanMacroDefineParams()as if it were a function-like parameter list — corrupting the AST for object-like macros whose value happens to begin with(.Gate the flag on the next char being an immediate
(with no intervening whitespace, as_scanWordleaves the cursor pointing at the first unread character:🐛 Proposed fix
- if (this._inMacroDefineValue) this._macroDefineExpectsParamsToken = true; + if (this._inMacroDefineValue) this._macroDefineExpectsParamsToken = this.getCurChar() === "(";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shader-lab/src/lexer/Lexer.ts` around lines 696 - 703, The flag _macroDefineExpectsParamsToken is being set unconditionally inside the _macroDefineExpectsNameToken branch causing "(...)" after spaced macro names to be mis-parsed as params; change the logic in the block that handles _macroDefineExpectsNameToken (the code around token.set(ETokenType.ID, ...) and where _inMacroDefineValue is checked) to only set _macroDefineExpectsParamsToken when the very next unread character is '(' (i.e. peek the character at the current cursor left by _scanWord / _scanWord's behavior) rather than whenever _inMacroDefineValue is true, so that intervening whitespace (skipped later by _skipInlineSpaceAndComments called from scanToken) does not trigger _scanMacroDefineParams for object-like macros.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/shader-lab/src/lexer/Lexer.ts`:
- Around line 696-703: The flag _macroDefineExpectsParamsToken is being set
unconditionally inside the _macroDefineExpectsNameToken branch causing "(...)"
after spaced macro names to be mis-parsed as params; change the logic in the
block that handles _macroDefineExpectsNameToken (the code around
token.set(ETokenType.ID, ...) and where _inMacroDefineValue is checked) to only
set _macroDefineExpectsParamsToken when the very next unread character is '('
(i.e. peek the character at the current cursor left by _scanWord / _scanWord's
behavior) rather than whenever _inMacroDefineValue is true, so that intervening
whitespace (skipped later by _skipInlineSpaceAndComments called from scanToken)
does not trigger _scanMacroDefineParams for object-like macros.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 481ca2dc-4f7e-43ee-b195-56ef6ffa39c1
⛔ Files ignored due to path filters (8)
tests/src/shader-lab/expected/define-struct-access-global.frag.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access-global.vert.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access.frag.glslis excluded by!**/*.glsltests/src/shader-lab/expected/define-struct-access.vert.glslis excluded by!**/*.glsltests/src/shader-lab/shaders/define-struct-access-global.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/define-struct-access.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/global-varying-var.shaderis excluded by!**/*.shadertests/src/shader-lab/shaders/macro-member-access-builtin-arg.shaderis excluded by!**/*.shader
📒 Files selected for processing (12)
packages/shader-lab/src/ParserUtils.tspackages/shader-lab/src/Preprocessor.tspackages/shader-lab/src/codeGen/CodeGenVisitor.tspackages/shader-lab/src/codeGen/GLESVisitor.tspackages/shader-lab/src/codeGen/VisitorContext.tspackages/shader-lab/src/common/SymbolTable.tspackages/shader-lab/src/common/enums/Keyword.tspackages/shader-lab/src/lalr/CFG.tspackages/shader-lab/src/lexer/Lexer.tspackages/shader-lab/src/parser/AST.tspackages/shader-lab/src/parser/GrammarSymbol.tstests/src/shader-lab/ShaderLab.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/shader-lab/src/parser/GrammarSymbol.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/shader-lab/src/common/SymbolTable.ts
- packages/shader-lab/src/common/enums/Keyword.ts
- tests/src/shader-lab/ShaderLab.test.ts
- packages/shader-lab/src/codeGen/VisitorContext.ts
- packages/shader-lab/src/ParserUtils.ts
| @@ -206,7 +349,9 @@ export abstract class GLESVisitor extends CodeGenVisitor { | |||
| const symbols = _referencedGlobals[ident]; | |||
| for (let i = 0; i < symbols.length; i++) { | |||
There was a problem hiding this comment.
let i = 0, n = symbols.length; i < n; i++
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/src/shader-lab/ShaderLab.test.ts (2)
321-324: Pin the assertion to the rewritten macro-member output.
normalize,dot, andtexture2Dare broad substrings and can pass even if the member-access macro rewrite regresses. Consider asserting a fixture-specific rewritten expression, or the absence of the stale struct-member form, so the test proves the AST macro path is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/shader-lab/ShaderLab.test.ts` around lines 321 - 324, The current assertions on fragment use broad substrings ("normalize", "dot", "texture2D") which can pass even if the macro-member rewrite regresses; update the test in ShaderLab.test.ts to assert the fixture-specific rewritten expression produced by the macro rewrite (use the exact rewritten member-access string that should appear in fragment) and/or assert the absence of the stale struct-member form (e.g., ensure fragment does NOT contain the original "myStruct.member" pattern); reference the test variable fragment and replace the three loose expect(...).to.contain(...) checks with an assertion that matches the exact rewritten output and a negative assertion for the old form so the AST macro path is actually exercised.
354-356: Check duplicate varyings in every generated stage that should declare them.The comment says duplicate varying declarations should be prevented, but the assertion only counts
v_worldPosinvertex. If this varying is expected in the fragment output too, add the same count there so verbose fragment emission cannot regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/shader-lab/ShaderLab.test.ts` around lines 354 - 356, Test only checks for duplicate declaration of "varying vec3 v_worldPos" in the vertex shader (variable vertex and varyingMatches), but misses the fragment shader; update the test to also search the fragment output (e.g., fragment variable/string) for the same "varying vec3 v_worldPos" and assert its count is 1 so duplicate varying emission in the fragment stage is detected as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/src/shader-lab/ShaderLab.test.ts`:
- Around line 321-324: The current assertions on fragment use broad substrings
("normalize", "dot", "texture2D") which can pass even if the macro-member
rewrite regresses; update the test in ShaderLab.test.ts to assert the
fixture-specific rewritten expression produced by the macro rewrite (use the
exact rewritten member-access string that should appear in fragment) and/or
assert the absence of the stale struct-member form (e.g., ensure fragment does
NOT contain the original "myStruct.member" pattern); reference the test variable
fragment and replace the three loose expect(...).to.contain(...) checks with an
assertion that matches the exact rewritten output and a negative assertion for
the old form so the AST macro path is actually exercised.
- Around line 354-356: Test only checks for duplicate declaration of "varying
vec3 v_worldPos" in the vertex shader (variable vertex and varyingMatches), but
misses the fragment shader; update the test to also search the fragment output
(e.g., fragment variable/string) for the same "varying vec3 v_worldPos" and
assert its count is 1 so duplicate varying emission in the fragment stage is
detected as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f49acec-eb47-4340-886d-9b2a01534d37
📒 Files selected for processing (2)
packages/shader-lab/src/parser/AST.tstests/src/shader-lab/ShaderLab.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/parser/AST.ts
d68b8d0 to
db618bd
Compare
…ine-ast-firstclass
db618bd to
251bdad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shader-lab/src/lexer/Lexer.ts`:
- Around line 506-520: The array literal assigned to private static readonly
_expressionValueLeaderKeywords is misformatted per Prettier (each grouped
type-constructor should be one entry per line); reformat the Set initializer so
every string literal occupies its own line (e.g., split "bvec2", "bvec3",
"bvec4" into three separate lines, same for ivec*, uvec*, vec*, mat* entries)
and then run Prettier/ESLint to ensure no prettier/prettier violations remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 29fc428c-2555-4e65-bbf5-9e34f9ca5437
⛔ Files ignored due to path filters (1)
tests/src/shader-lab/shaders/define-ctor-with-member.shaderis excluded by!**/*.shader
📒 Files selected for processing (3)
packages/shader-lab/src/lexer/Lexer.tspackages/shader-lab/src/parser/AST.tstests/src/shader-lab/ShaderLab.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/src/shader-lab/ShaderLab.test.ts
- packages/shader-lab/src/parser/AST.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/shader-lab/ShaderLab.test.ts (1)
267-273: Nit: preferShaderLanguage.GLSLES100over the literal0for thebackendargument.
_parseShaderPasstakes aShaderLanguageenum value ({ vertex, fragment } = shaderLabVerbose._parseShaderPass(..., 0, "")). Using the named constant makes the intent obvious and protects the tests if the enum ordering ever changes. Applies to all four new call sites (Lines 271, 291, 313, 338).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/shader-lab/ShaderLab.test.ts` around lines 267 - 273, Tests call shaderLabVerbose._parseShaderPass with a magic literal backend value (0); replace that literal with the enum constant ShaderLanguage.GLSLES100 in each call site to make intent explicit and resilient to enum reordering—update the four new invocations that pass 0 to instead pass ShaderLanguage.GLSLES100 (references: _parseShaderPass, ShaderLanguage, shaderLabVerbose).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/src/shader-lab/ShaderLab.test.ts`:
- Around line 267-273: Tests call shaderLabVerbose._parseShaderPass with a magic
literal backend value (0); replace that literal with the enum constant
ShaderLanguage.GLSLES100 in each call site to make intent explicit and resilient
to enum reordering—update the four new invocations that pass 0 to instead pass
ShaderLanguage.GLSLES100 (references: _parseShaderPass, ShaderLanguage,
shaderLabVerbose).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d6471a0-0e9b-4c68-abc9-28065b515078
⛔ Files ignored due to path filters (1)
tests/src/shader-lab/shaders/define-ctor-with-member.shaderis excluded by!**/*.shader
📒 Files selected for processing (3)
packages/shader-lab/src/lexer/Lexer.tspackages/shader-lab/src/parser/AST.tstests/src/shader-lab/ShaderLab.test.ts
Type keywords like `vec4`, `mat3`, `float`, etc. can legitimately start a GLSL expression via constructor-call syntax (e.g. `mat3(v.v_tangent, …)`). They were previously misclassified as declaration-style macro leaders and sent down the opaque legacy path, so member access inside such values wasn't rewritten — producing broken GLSL when the macro referenced a varying/attribute struct variable. Extend `_expressionValueLeaderKeywords` to include all scalar/vector/matrix type names. Qualifier keywords (`highp`, `uniform`, `struct`, `in`, …) and sampler types remain excluded so true declaration-style macros stay on the legacy path. New test: `define-ctor-with-member` covers `#define TBN_BLEND mat3(v.v_tangent, ...)`.
251bdad to
b248dc7
Compare
Switch the `#define` expression-path whitelist from a string Set to a `Set<Keyword>`, and drop a helper that became dead when the macro-value codegen moved to AST traversal. - `_expressionLeaderKeywords`: elements are now Keyword enum values. Eliminates the string/Keyword double-truth around `_lexemeTable` and reuses the keyword lookup already done for the main classification. - `VisitorContext.referenceStructPropByName`: removed. The AST path calls `referenceVarying`/`referenceAttribute`/`referenceMRTProp` directly with a proper `location`; the by-name helper with its `undefined as any` location is no longer reachable. - `GLESVisitor._getGlobalSymbol`: hoist `symbols.length` out of the loop header, consistent with the other loops in this file.
…adjacency `#define FOO (1 + 2)` (space before `(`) was being misclassified as a function-like macro `FOO()` with body `1 + 2`, then failing to parse because the CFG's `macro_define` rule requires a value after the params block. Per C99 §6.10.3/3 — which GLSL ES 3.00 §3.4 inherits verbatim — the function-like form requires `(` immediately after the name, with no intervening whitespace. Fix: after scanning the macro name, set `_macroDefineExpectsParamsToken` only when the *current* char is `(`. A space-separated `(` falls through to the normal value token stream, yielding the correct object-like macro with value `(1 + 2)`. Regression test covers both shapes side-by-side in one shader: `OBJ_PAREN (1 + 2)` (object-like) and `FN_LIKE(x) (x + 3.0)` (function-like).
…n-like macro
MacroCallFunction covers two call shapes sharing one AST:
(a) object-like macro whose value is a function name, used as a call
— `#define FN foo` + `FN(varyings, …)`. The driver expands `FN`
to `foo`; `foo` is a ShaderLab function whose IO-struct params
have been flattened. Call site must drop IO-struct args.
(b) true function-like macro — `#define MAX3(a,b,c) …`
+ `MAX3(v.v_normal.x, v.v_normal.y, v.v_normal.z)`. ShaderLab
doesn't expand the macro; the driver does, and the `#define`
fixes parameter count. Args must be preserved verbatim — a
member-access arg unwraps to an IO-struct root identifier but
dropping the arg changes the macro's arity.
Previously `visitMacroCallFunction` applied the shape-(a) filter
uniformly, so any function-like macro call whose arg unwrapped to
an IO-struct variable had that arg silently stripped, yielding
empty-arg-list expansions like `MAX3()` that fail to compile.
Fix: thread `isFunctionLikeMacro` from MacroCallSymbol.semanticAnalyze
through MacroCallFunction. When the `#define` is function-like (has
`isFunction` in macroDefineList), skip the drop-IO-arg filter.
Regression test: `macro-call-struct-arg-repro.shader`
#define MAX3(a, b, c) max(max(a, b), c)
float m = MAX3(v.v_normal.x, v.v_normal.y, v.v_normal.z);
Re: [第八轮 P1]
|
| Call shape | Driver 展开后 | Callee 签名 flatten? | 过滤 IO 实参? |
|---|---|---|---|
| (a) object-like 宏作为函数名 | ShaderLab 函数 foo(…) |
是 | 是 |
| (b) true function-like 宏 | 无 callee,文本替换 | 否 | 否(破坏 arity) |
区分它们的定义端特征就是 isFunction(macroDefineList 里记录的宏是不是 function-like)。
修复:把 isFunction 透传到 call site,按此分叉。
// AST.ts: MacroCallSymbol.semanticAnalyze
this.isFunctionLikeMacro = sa.macroDefineList[name]?.some(info => info.isFunction) ?? false;
// CodeGenVisitor.ts: visitMacroCallFunction
const params = node.isFunctionLikeMacro
? astNodes // shape (b): 保持全部实参
: astNodes.filter(/* 既有 IO struct arg drop */); // shape (a): 与 visitFunctionCall 同规则4. visitFunctionCall 不需要同步 guard
CR 的补充建议 "visitFunctionCall 的同类 filter 也需要 hasAstValue guard" —— 此处不需要。
visitFunctionCall 处理 FunctionCall AST 节点,callee 是确定的 FnSymbol(ShaderLab 函数)。filter 按 callee 形式参数的静态类型 (paramInfoList[i].typeInfo) 过滤,而非按实参表达式的类型。只要 callee 形参是 IO struct 类型就 drop —— 这是对"callee 签名已被 flatten"的正确反应,与实参是不是从 AST-backed macro 展开来的无关。
如果担心 "object-like macro 展开后作为 function name 调用" 的情况,这个其实走的就是 MacroCallFunction 路径(上表 shape (a)),由 visitMacroCallFunction 处理;而一旦进入 visitFunctionCall,callee 就一定是 FnSymbol,filter 总是对的。
5. 验证
- 全量测试 1300/1300 通过
- PBR 中的
FUNCTION_DIFFUSE_IBL(varyings, surfaceData, …)(shape (a))继续 work - 新 repro
MAX3(v.v_normal.x, …)(shape (b))修复 - 已知局限(PR body 列出的 3 类)未变化
感谢这轮细致 review — 发现了一个我们之前没识别到的独立 bug。Commit:fdcb7ea1d。
Review: AST-first
|
| 场景 | Baseline loops | PR loops | Δ | Per-directive |
|---|---|---|---|---|
| 0 defines(基准) | 473 | 473 | 0 | — |
10 × #define X 3.14 |
503 | 703 | +200 | 3 → 23 loops (7.7x) |
10 × #define X v.v_uv |
503 | 743 | +240 | 3 → 27 loops (9x) |
10 × #define X vec4(v.v_uv, i, 1.0) |
503 | 1,373 | +870 | 3 → 90 loops (30x) |
10 × #define X mat3(v.v_normal, v.v_color.xyz, cross(...)) |
503 | 1,983 | +1,480 | 3 → 151 loops (50x) |
100 × #define X 3.14 |
773 | 2,773 | +2,000 | 3 → 23 loops |
500 × #define X 3.14 |
1,973 | 11,973 | +10,000 | 3 → 23 loops |
10 × #define X highp(legacy 路径) |
503 | 503 | 0 | 3 loops(持平 ✓) |
关键观察:
- Baseline 每
#define固定 3 loops(1 shift + 1 reduce forMACRO_DEFINE_EXPRESSION+ 1 forglobal_declaration) - PR 路径每
#define23~151 loops,因为 value 要完整走assignment_expression的 18 层规约链 - Legacy path(
#define X highp)验证分流策略正确——走 legacy 的宏与 baseline 持平 - PBR 47 个有值
#define(源码 + include 的 Common/Shadow/BSDF 等共约 49 条)全部走 AST 路径,保守估计 +940 loops,PR 描述"不变"不准确
Runtime 不受影响(Shader.create 每 pass 只 parse 一次,变体编译走 ShaderMacroProcessor.evaluate 不重 parse,.gsp 加载跳过 shader-lab),但编辑器/预编译工具链每个 shader 会多花 1-10ms 量级。
建议合入前修的最小集
-
Blocker 1/Warning 2 合并修: 分流策略从"首词不是声明关键字 → AST"改成"值含
.成员访问 → AST"。正向识别真正需要 ShaderLab 介入的 value。改完后:#define BEGIN {/#define COMMA ,/#define REPEAT3(x) x,x,x回到 legacy 路径,不 parse error#define PI 3.14/#define MAX 4/#define FOO bar这类纯常量宏也回到 legacy,parser 成本消失- Warning 1(注释盲点)副产品解决
-
Blocker 2 修: 把
CFG.ts里新增的macro_define产生式 + 3 个 token 声明同步到TargetParser.y,跑bison TargetParser.y确认无 shift/reduce 和 reduce/reduce 冲突。 -
Blocker 3 修: 消除 Preprocessor regex 与 MacroDefine.semanticAnalyze 双路径 push。推荐方案:regex collect 跳过 AST-form 的 name(需要 Lexer 提前标记),或者 AST 路径覆写同名条目而非 append。
-
PR 描述修正: PBR parser steps 重测更新;"不变"改成真实数字。
Warning 1 做完 Blocker 1 会自动修;Nit 1/Nit 2 可以后续独立 PR 迭代。
参考
- OpenGL ES Shading Language Specification 3.00 — Khronos Registry(Galacean 目标版本,§3.4 Preprocessor)
- OpenGL ES Shading Language 3.20 — Khronos Registry
- OpenGL Shading Language 4.60 — Khronos Registry(桌面 GLSL,含
##token paste 定义) - Vulkan Documentation Project — GLSL Basics(预处理器章节)
- ANGLE
DirectiveParser.cpp(Chrome WebGL 的 GLSL ES 预处理器实现) - ANGLE
MacroExpander.cpp
|
谢谢非常精准的双 bug 报告。已修复,commit Fact-check:3 个互相叠加的 bug报告说"两个 bug 必须一起修"——确认。但 fix 到一半发现还有第三处需要动: Bug 1(PR 引入):
|
…ease Three interlocked bugs caused `(v).v_uv` and `v . v_uv` to skip varying flatten in release builds: 1. `_defineHasValue` only treated `.` as member-access when its left-side run was an identifier-leading alnum sequence — wrongly excluded `(v).v_uv` (run preceded by `)`) and `v . v_uv` (run empty due to whitespace). Inverted the rule: any `.` is member-access except `digit.digit` (numeric literal `3.14`, `1.0e-5`). 2. `Expression.semanticAnalyze` and `AssignmentExpression.semanticAnalyze` were wrapped in `// #if _VERBOSE` — type propagation got stripped in release builds so outer `(expr).field` codegen saw `TypeAny` and skipped flatten. Both are release-required for codegen correctness, not verbose-only logging. 3. `extractDirectIdentLexeme` only forwarded through PostfixExpression and PrimaryExpression(len=1) — `(v)` returned null, missing the `_structVarMap` lookup that macro-path codegen relies on (since macro-time `v` has no symbol-table binding, type fallback is TypeAny). Generalized to forward through any single-child ExpressionAstNode + the `( expr )` form, covering the entire precedence chain in both verbose and release builds. Adds regression `paren-member-access-repro.shader` covering inline + macro forms of `(v).v_uv`, `v . v_uv`, `((v)).v_uv` in both release and verbose builds. 28/28 shader-lab tests pass. Release AST node count unchanged (types of `pool.get()` calls untouched; only method bodies re-enabled).
ae5be79 to
072c276
Compare
|
谢谢精准的双 bug 报告。已修复,commit Fact-check:3 个互相叠加的 bug报告说"两个 bug 必须一起修"——确认。但 fix 到一半发现还有第三处需要动: Bug 1(PR 引入):
|
…an#2980) Remove the regex-based pre-scan in Preprocessor that registered `#define` entries into `macroDefineList`. The Lexer now fills the list inline as it tokenizes — single source of truth, no drift. Two interpreters of the same source had been silently disagreeing: - Preprocessor's `_macroRegex` didn't understand block comments or `\`-line-continuation, so `/* #define MAX_LIGHTS 4 */` got registered as a real macro, and `#define LONG \\\n a*b` truncated its value. - Lexer's `_defineHasValue` peek had its own corner cases (commit `5b7f8ae37` patched one for `#define HP /* comment */ highp`). - The `_expressionLeaderKeywords` whitelist conflated "can lead a constructor call" with "is a complete expression", breaking FXAA type-alias macros (commit `992956ffe` patched that). Each was a one-off symptom of the same architectural fact: two analyzers parsing the same source independently will drift. Approach -------- `#define` is registered exactly once, by the entity that already parses every byte of the source: the Lexer. - `_scanDirectives` records the directive's start offset on `#define`. - The two `#define` paths funnel through one helper: - AST path: `_emitMacroDefineEnd` slices the source between the recorded start and the directive-terminating newline. - Legacy path: `_scanDirectives` slices the same range after `_scanUtilBreakLine`. - `_registerMacroDefine(directive)` runs a single regex on that slice, builds a `MacroDefineInfo`, and pushes (with the existing duplicate guard). Preprocessor now only handles `#include` expansion. `_macroRegex`, `_parseMacroDefines`, `_registerDefine`, `_referenceReg`, `_isExist`, `_mergeMacroDefineLists`, and the dual-field `_chunkCache` are gone. The chunk cache shrinks to a path → expanded-string Map. Net diff (excluding tests): Preprocessor.ts 227 → 92 (−135) Lexer.ts 781 → 820 (+39) ShaderLab.ts 1-line caller update Total (−96 lines) Performance ----------- Same-session 4 rounds × 50 samples vs commit `072c2766d` (release build, median-of-medians): PBR (complex) 40.60 → 38.50 (−5.2%) waterfull (medium) 7.30 → 7.05 (−3.4%) multi-pass 2.00 → 1.95 (−2.5%) macro-pre 1.10 → 1.00 (−9.1%) Single-pass eliminates the Preprocessor's full-file `_macroRegex` exec loop; per-directive regex on a small slice is cheaper. Tests ----- 30/30 shader-lab tests pass, including two new regressions for Issue galacean#2980: - define-in-comment-repro.shader (block-commented #define) - define-line-continuation-repro.shader (`\`-continuation in value)
Nit:
|
| Entry | valueAst | referenceName | 来源 |
|---|---|---|---|
| #0 | AST 子树 | "" |
#ifdef 分支 |
| #1 | undefined | "u_globalLightDir" |
#else 分支 |
调用点 LIGHT_INPUT:
hasAstValue = .some(... valueAst != null) = true(被 #0 触发)referenceSymbolNames = ["u_globalLightDir"](来自 Chore/workflow #1)- 符号表查
u_globalLightDir→ 找到 uniform vec3 - 但
hasAstValue=true→ 跳过typeInfo = symbols[0].dataType?.type typeInfo = undefined→ 调用点类型 = TypeAny
行为不一致
| 实际编译变体 | 应有类型推导 | 实际类型推导 |
|---|---|---|
USE_INTERPOLATED_NORMAL 定义 |
TypeAny(按设计 skip 是对的,AST 路径自带类型) | TypeAny ✓ |
USE_INTERPOLATED_NORMAL 未定义 |
vec3(应当从 u_globalLightDir 推) |
TypeAny ❌ |
第二种场景:active 分支是 legacy,本应走类型推导 fallback,被 .some() 拉成"按 AST 处理",typeInfo 默认 TypeAny。
下游影响
依赖 typeInfo 的几处 codegen 决策都会受影响:
- builtin 函数重载解析:
normalize(LIGHT_INPUT)当 typeInfo 是 TypeAny 时,重载选择行为依赖 shader-lab 对 TypeAny 的容忍策略——能 match 上是侥幸,匹配不上则报 "no matching overload" - IO struct flatten:
getStructRole(typeInfo.typeLexeme)在 TypeAny 时返回 undefined → 跳过本应触发的成员访问改写 - 类型驱动的 codegen 分支:任何
if (typeInfo === SOMETHING)都会走错路径
具体出哪种症状取决于调用点上下文。关键问题不是"哪种症状最常见",而是"系统对用户做出了一个它无法保证的承诺,且失败时无诊断"。
警告系统也漏
理论上 shader-lab 应该在以下情况警告用户:
- 同名宏跨
#ifdef分支定义时形态不一致(一边 AST 一边 legacy) .some()把 flag 拉真后,对应 active 分支可能行为异常
实际:两个警告都没有。MacroCallSymbol.semanticAnalyze 直接 .some() 后用 flag,没有任何 sanity check。
修复方向
选项 A — 严格 fix:parser 跟踪 #ifdef 嵌套,每个 MacroDefineInfo 记录定义所在的 branch path,调用点按 active branch 过滤。代价:parser 状态机改动较大。
选项 B — 防御式 warning:在 MacroCallSymbol.semanticAnalyze 检测 defList 同时含 AST 形态和 legacy 形态时,emit warning:
const hasAst = defList?.some((info) => info.valueAst != null) ?? false;
const hasLegacy = defList?.some((info) => info.valueAst == null) ?? false;
if (hasAst && hasLegacy) {
// #if _VERBOSE
sa.reportWarning(this.location,
`Macro "${macroName}" has mixed AST/legacy definitions across #ifdef branches; ` +
`call-site type inference may be unreliable.`);
// #endif
}
this.hasAstValue = hasAst;不修底层语义,但至少让用户知道这条调用点行为可能有问题——把"静默错误"降级为"显式 warning"。
选项 C — 文档列入 Known limitation:明确告诉用户"同名宏在不同 #ifdef 分支里 value 形态必须一致(要么都含成员访问、要么都不含),否则调用点类型推导可能不准确"。把缺陷转移给文档,但仍然没有任何编译期检查保证用户遵守。
关键问题
.some() 在跨分支场景下没有定义清晰的语义。要么 fix(选项 A)、要么 warn(选项 B)、要么明确写入文档(选项 C),但当前实现既不 fix 也不 warn 也不 doc——这是设计缺陷。
…cean#2980) Same `#define` repeated in disjoint `#ifdef` branches used to conflate at the call site: `MacroCallSymbol.hasAstValue` was set from `defList.some(valueAst)`, so any AST-form entry across any branch silently disabled legacy type inference — even when the active branch's entry was the legacy form. Result: call-site `typeInfo` stranded as `TypeAny`, downstream codegen (builtin overload selection, varying flatten) emitted incorrect GLSL. Root cause: `macroDefineList` was branch-flat. No data was tracking which `#ifdef` branch a `#define` was registered in, so consumers couldn't filter to the entries actually reachable from a call site. Fix: make `macroDefineList` branch-aware end-to-end. - `BranchConstraint` / `BranchSignature` types in common/BaseToken - Lexer maintains a branch stack across `#ifdef`/`#ifndef`/`#else`/`#endif`, stamps every emitted token's `branch` field, attaches a snapshot to every registered `MacroDefineInfo` - `Lexer.sameBranch` / `Lexer.isVisibleFrom` as static helpers - `MacroCallSymbol.semanticAnalyze` filters `defList` by `isVisibleFrom` against the call site's own branch signature, then derives `hasAstValue` / `isFunctionLikeMacro` / `referenceSymbolNames` from only the visible entries - `MacroDefine.semanticAnalyze` upgrade matches branch signatures so AST `valueExpression` lands on the right branch's entry Tests: define-mixed-form-repro (AST/legacy mixed across `#ifdef`), define-branch-scoped-ast (both AST forms, different members), define-nested-ifdef (nested branches register under combined signatures). 176/176 shader-lab tests pass.
新增 commit
|
| Shader | dev/2.0 | PR HEAD | 提升 |
|---|---|---|---|
| PBR (complex) | 49.50ms | 40.75ms | −17.7% (−8.75ms) |
| waterfull (medium) | 10.00ms | 7.30ms | −27.0% (−2.70ms) |
| macro-pre | 1.30ms | 1.10ms | −15.4% (−0.20ms) |
| multi-pass | 2.40ms | 2.00ms | −16.7% (−0.40ms) |
数值跟 PR body 里之前的 −21.9%/−28.6% 略有差异(同 session 取样的随机波动 ±5%),趋势一致。本 commit 引入分支栈维护 + 每 token branch tagging,理论上有微小开销,但实测没有引入回退:4 个 shader 全部仍然显著快于 dev/2.0 baseline。
测试
176/176 shader-lab 测试全过,含 3 个新增 regression:
define-mixed-form-repro.shader(AST/legacy 跨分支)define-branch-scoped-ast.shader(同 AST 形态、不同 member)define-nested-ifdef.shader(嵌套#ifdef组合签名)
Fact-check:4 个
|
Fact-check 第二轮:剩余 P3 项目把上一轮 review 里没实测的 P3 也走一遍。结论是 5 条 P3 里 4 条是伪问题,可关闭;只有「代码冗余」一条是真的,但确实只是冗余、不是 bug。 ✅ 关闭:「TrivialNode elision 下游兼容性」(原 review 担忧
|
| 问题 | 实测后评级 | 状态 |
|---|---|---|
_defineHasValue 不剥离注释 |
🟠 P1 | 待修 |
_defineHasValue 不跟随 \\\n |
🟠 P1 | 待修 |
_scanMacroDefineParams 不认 \\\n |
🟠 P1 | 待修 |
_defineDirectiveReg 对 \\\n NO MATCH |
🟡 P2 | 待修(driver 救场,但内部状态退化) |
parseMacroParamList 实现重复 |
🟢 P3 | 可作为 follow-up |
| TrivialNode elision 下游兼容性 | 关闭(伪问题) | — |
| Lexer stateful flag 复用 | 关闭(伪问题) | — |
| release 启用类型传播 | 关闭(伪问题,意图行为且无回归) | — |
| 测试断言过宽 | 关闭(与 coderabbitai 重复) | — |
净增的待修 issue:3 P1 + 1 P2,全部围绕 \\\n 行连续 + 注释 / . 检测的 lexer 边界。
Bug:commit
|
Reviewer fact-check identified 4 issues where the `#define` scan path
diverged from C/GLSL preprocessor rules. All four share the same root
cause: directive-scanning helpers each rolled their own
"scan-to-end-of-line" loop without honoring `\` + newline
line-continuation or comments.
- P1-1: `_defineHasValue` traversed raw chars, so a `.` inside a
block comment (`#define HP /* a.b */ highp`) wrongly routed the
directive to the AST path; `highp` is not a valid expression
starter and parse failed.
- P1-2: `_defineHasValue` bailed at the first `\n`, so a line-
continued `.field` (`#define UV foo \\n .field`) was missed and
the directive misrouted to the legacy path; `.field` then leaked
out as a stray top-level token.
- P1-4: `_scanUtilBreakLine` and `_scanMacroDefineParams` likewise
stopped at the first `\n`, so multi-line function-like macro
headers (`#define MAX3( \\n a, b, c \\n ) …`) lost the trailing
physical lines and pushed `\` `\n` into the params lexeme,
breaking the grammar.
- P2-3: `_defineDirectiveReg`'s value group rejects newlines, so a
directive slice still containing `\` + `\n` would NO MATCH and
`_registerMacroDefine` silently skipped registration. Driver
text-substitution masked the issue, but ShaderLab's internal
invariant was broken (call site was tagged ID instead of
MACRO_CALL, raising spurious "v not declared" warnings).
Fix consolidates two layers of duplicated scanning into shared atoms:
BaseLexer
+ _skipBlockComment / _skipLineComment ← single source of truth
for comment scanning
skipCommentsAndSpace ← uses the atoms
Lexer
+ _skipLineContinuation ← single source of truth
for `\` + newline
+ _skipNonSemantic ← uses comment atoms +
continuation atom
+ _lineContinuationReg ← regex form for
one-shot string fold
_scanUtilBreakLine ← uses _skipLineContinuation
_scanMacroDefineParams ← uses _skipLineContinuation
_registerMacroDefine ← folds before regex match
_defineHasValue ← uses _skipNonSemantic
Also fixes a latent BaseLexer block-comment bug: an unterminated
`/* …` would advance `index` two past the source end, harmless at
top-level but fragile. The new `_skipBlockComment` clamps to `len`.
Tests: 4 new regression fixtures (define-comment-with-dot,
define-line-continuation-member-access, define-line-continuation-no-dot,
define-multiline-params). 180/180 shader-lab tests pass including
PrecompileBenchmark.
`tokenize`'s branch-stack switch only handled `#ifdef`/`#ifndef`/`#else`/
`#endif`. `#if expr` and `#elif` were silently dropped: `#if` opened a
conditional level without pushing onto the stack, so the matching
`#endif` would pop the wrong level — the outer `#ifdef A` constraint —
leaving any `#define` after the inner `#endif` registered with an empty
branch signature instead of `[{A, true}]`.
Real shaders nest these constantly. `packages/shader/src/shaders/`
alone has 37 `#if` directives, with structures like Fog.glsl's
`#if SCENE_FOG_MODE != 0` wrapping `#if SCENE_FOG_MODE == 1`, and
BlendShape.glsl's chains of `#if defined(...)`. Anything inside such a
nest emits `#define`s with a corrupted branch field, breaking
per-branch visibility filtering at call sites.
Fix: `#if` pushes a shared sentinel constraint (`name === ""`,
`defined === true`) — `isVisibleFrom` ignores empty-name entries in
its polarity check, so the sentinel is conservatively visible
everywhere, but it occupies one stack slot so `#endif` pops the right
depth. `#elif` doesn't change depth (it's another arm at the same
level) so no case is needed there.
Sentinel is shared across all `#if` opens to avoid per-token
allocation. `#else` flipping a sentinel's `defined` is harmless since
`isVisibleFrom` ignores empty-name entries either way.
Test: `define-if-stack-balance.shader` covers `#ifdef A / #if expr /
#endif / #define X / #endif` and `#ifdef A / #if F / #elif G / #endif /
#define Y / #endif`. 38/38 shader-lab tests pass.
|
@cptbtptpbcptdtptp Fact-checked 你两轮 review 中的所有 P1/P2 项,5 项都是真 bug,全部已修。 修复 commit
第一性原理修法4 个 修复抽出 3 层公共原子:
测试覆盖5 个新 regression fixture:
38/38 shader-lab 测试 + 全 PrecompileBenchmark 通过。 性能(5 轮 × 50 samples,median-of-medians)
新增的 5 个 fix(含 sentinel push 每个 剩下的 follow-up
|
增量 CR —
|
| Repro | 上轮(e31c245) |
现在(ca950ef) |
|---|---|---|
#define HP /* a.b */ highp |
parse 死 | ✅ 通过,输出 #define HP /* a.b */ highp |
#define UV foo \\\n .field |
parse 死 | ✅ 通过,输出 #define UV foo.field |
#define LONG_VAL v.v_uv \\\n + v.v_uv 调用点 |
调用点退化为 ID + spurious warning | ✅ 通过,输出 #define LONG_VAL v_uv + v_uv(varying flatten 生效,无 warning) |
#define MAX3( \\\n a, b, c \\\n ) … |
parse 死 | ✅ 通过,参数表正常识别 |
4 个 P1/P2 全修,加上新发现的 #if 嵌套 stack balance bug,5 个根因 fix 一并落地。
实现质量观察
- 抽
_skipBlockComment/_skipLineComment到BaseLexer、_skipLineContinuation/_skipNonSemantic到Lexer— 之前散在_defineHasValue/_skipInlineSpaceAndComments/_scanUtilBreakLine/_scanMacroDefineParams四处的临时实现,现在都走单 source of truth。这正是 review 里提到的「分散度」问题,作者直接做了正确的解。 _lineContinuationReg配合_registerMacroDefine的 fold-before-match — 优雅修了 P2-3 的 regex NO MATCH。- 顺手修的 BaseLexer block-comment unterminated 越界 — baseline 长期潜在 bug,新代码用
_skipBlockComment把len钳位掉,没增加额外测试但风险消除。 _IF_SENTINEL处理#if exprstack balance — 之前#if不入栈但#endif出栈,深嵌套(Fog.glsl / BlendShape.glsl 这类 chain)下分支签名彻底错乱。sentinel 用name === ""让isVisibleFrom跳过 polarity 检查,既保 stack 平衡又保留「empty signature 处处可见」的语义。共享 sentinel 实例避免每次#if分配。#elif不入栈正确(同层另一臂)。#else翻转 sentinel 的defined用新对象赋值而非 mutate 共享对象——细节正确,避免共享 mutate 副作用。
潜在 nit(非阻塞)
_scanMacroDefineParams跳\\\n时只跳行连续字符本身,不归一化前后空白,所以 c4 的输出是#define MAX3( a, b, c ) max(...)——driver 不在乎,但视觉冗余。可在 buffer push 时把行连续位置塞个空格、或者把多个空白折叠。Cosmetic only。_scanMacroDefineParams的循环结构是「先尝试跳行连续;不是行连续才 push 当前 char」。但字符\本身在 GLSL ES#define内非法——若行连续判定失败(例如\后不是换行而是 ID),buffer 会包含\。罕见,等同 baseline 行为。
测试套件
tests/src/shader-lab/ShaderLab.test.ts:38/38 PR 自带测试 + 4 个我的 verify case → 42/42 全绿。
PrecompileBenchmark.test.ts:6/6 全绿。
性能(10 runs verbose build)
| Shader | baseline | e31c245 |
ca950ef |
当前 vs baseline |
|---|---|---|---|---|
| PBR | 54.80ms | 50.00ms | 55.40ms | 持平 |
| waterfull | 12.00ms | 8.60ms | 9.60ms | −20% |
| multi-pass | 2.80ms | 2.30ms | 2.90ms | 持平 |
| macro-pre | 1.60ms | 1.30ms | 1.40ms | −12.5% |
新 commit 加的 comment/line-cont 处理 + sentinel 让 PBR 的加速被吃掉一些,但仍处 baseline 量级。其他 shader 维持加速。没有性能劣化。
整体判断
这一轮修复扎实——根因 fix(不是 patching),抽离单 source of truth,顺手治了一个 baseline 长期 bug,测试覆盖到位(4 个新 regression fixture 命名一一对应 reviewer claim)。
✅ 可以合并。
verify 脚本:
git clone --depth=20 -b feat/shaderlab-define-ast-firstclass https://github.com/hhhhkrx/runtime engine-pr-2974
cd engine-pr-2974 && pnpm install --prefer-offline && pnpm run b:module
# 添加 4 个 _verify-*.shader 到 tests/src/shader-lab/shaders/
# 添加对应 it() block 到 ShaderLab.test.ts
HEADLESS=true npx vitest run tests/src/shader-lab/ShaderLab.test.ts -t "verify-c"
Bug:
|
`#elif` opens a new arm at the same depth, but its actual condition is
"none of the previous arms held AND <elif expr>". Without a switch case,
the new arm inherits the previous arm's branch tag — exactly the
opposite of where it's active:
#ifdef A
#define X1 ... // branch=[{A, true}] ✓
#elif B
#define X2 ... // branch=[{A, true}] ✗ — X2 is only active when !A
#endif
The bug shows up at call sites: `MacroCallSymbol.semanticAnalyze` filters
defs by `isVisibleFrom`, so a callsite under `#ifdef A` would wrongly
treat X2 as visible (X2 inherits `[A=true]`), and a callsite under
`#ifndef A` would wrongly treat X2 as excluded. Engine shaders use this
pattern: FragmentPBR.glsl:188 has `#ifdef RENDERER_HAS_NORMAL / ... /
#elif defined(HAS_DERIVATIVES)`. User shaders are likely to as well.
Fix: replace the top constraint with the `#if` sentinel on every `#elif`.
Drops the precision of the previous arm's polarity (the new arm is no
longer constrained on `A`), but never wrongly inherits the wrong
polarity. Considered flipping polarity instead — works for the first
`#elif` of a chain, but `#ifdef A / #elif B / #elif C` would ping-pong
A's polarity. Sentinel is uniformly correct.
This was missed in `ca950efca`, where the commit message claimed
"`#elif` doesn't change depth, no case is needed" — depth is unchanged,
but polarity is part of the stack state and must be cleared.
Test: `define-elif-polarity.shader` covers `#ifdef A / #elif defined(B)
/ #elif defined(C)` chains. 39/39 shader-lab tests pass.
|
@cptbtptpbcptdtptp 你这条 review 完全准确,bug 已修:commit `b435e83c7`。 实测复现(修前): `ca950efca` 那条 commit message 我写的 "`#elif` doesn't change depth, no case is needed" 是错的——深度对,但 polarity 也是栈状态的一部分,没翻转就让 `#elif` arm 继承了前一 arm 的 tag。 修法采用你的简单方案(任何 `#elif` 把 top 替换为 sentinel)。也想过精确方案(第一次 `#elif` 翻 polarity 保留 `[A=false]`,后续 `#elif` 才降级),但:
新增 regression:`define-elif-polarity.shader` 覆盖 3-arm `#elif defined(...)` chain。39/39 测试通过。 |
GuoLei1990
left a comment
There was a problem hiding this comment.
Review (Round 20) — incremental on b435e83c7
已关闭问题清单
| 问题 | 关闭原因 |
|---|---|
| Prettier lint (VisitorContext.ts:117, GLESVisitor.ts:299) | 已修复 (commit 65e79b3) |
cptbtptpbcptdtptp: let i=0,n=symbols.length pattern |
已修复 (commit 0a056ae) |
| coderabbitai: test assertions too broad | P3 建议,不阻塞 |
| referenceStructPropByName 死代码 (P3) | 已修复 (commit 0a056ae) |
[P1] Lexer.ts — _macroDefineExpectsParamsToken 无条件置 true |
已修复 (commit 79511dc) |
[P1] CodeGenVisitor.ts — visitMacroCallFunction struct-arg 过滤对 function-like macro 错误生效 |
已修复 (commit fdcb7ea) |
| zhuxudong Blocker 2: TargetParser.y 未同步 | 已修复 (commit 828ac81) |
| zhuxudong Blocker 1 (部分): 非 alpha 首字符的 non-expression replacement list | 已修复 (commit 8f589ab) |
zhuxudong Warning 1: _defineHasValue keyword peek 漏过注释 |
已修复 (commit 8f589ab + 5b7f8ae) |
zhuxudong Blocker 3: macroDefineList 双路径冗余 + 假 warning |
已修复 (commit b09fd50) |
| [P2] Blocker 3 残留:双路径冗余产生假 warning | 已修复 (commit b09fd50) |
| [P2] 性能:纯常量宏不必要走 AST 路径 | 已修复 (commit 992956f) |
| [P1] function-like macro body 含 top-level comma parse error | 已修复 (commit 992956f) |
| zhuxudong New Blocker: FXAA type-alias parse error | 已修复 (commit 992956f) |
[P1] _chunkOutputCache 静态 Map 无限增长 |
非回归,P1→P2 降级 |
[P1] _preRegisterGlobalMacroRefs double codegen |
保持 P2 |
[P2] _defineHasValue 命名误导 |
保持 P3,不阻塞 |
[P2] MacroDefine.semanticAnalyze upgrade-in-place 隐式时序 |
已改善 (3668d8c) |
[P2] 移除 _VERBOSE 守卫导致 release 多做类型传播 |
功能正确性必要,TrivialNode elision 已抵消 |
[P3] MacroCallFunction.init() 创建新数组 |
仍存在但不阻塞 |
[P3] _scanMacroDefineParams buffer push + join |
仍存在但不阻塞 |
[P3] #define FOO .5 leading-dot false positive |
仍存在但不阻塞 |
[P2] _isExist 去重丢失 value 字段比较 |
已消除 (3668d8c) |
zhuxudong Nit: .some() cross-branch mixed AST/legacy |
已修复 (commit e31c245) |
cptbtptpbcptdtptp P1-1: _defineHasValue 不剥离注释 → AST 误路由 |
已修复 (commit a7a4948) |
cptbtptpbcptdtptp P1-2: \+\n line continuation 不被 _defineHasValue 跟随 |
已修复 (commit a7a4948) |
cptbtptpbcptdtptp P1-4: _registerMacroDefine regex 对含 \+\n 的 directive text NO MATCH |
已修复 (commit a7a4948) |
cptbtptpbcptdtptp P2-3: _scanMacroDefineParams 将 \ \n 推入 params buffer |
已修复 (commit a7a4948) |
zhuxudong Bug: #if/#elif 未在 _branchStack 中 push/replace → 栈错配 |
已修复 (commit ca950ef) |
[P1] console.log 调试代码残留 (Lexer.ts:842) |
已修复 (commit ca950ef) |
zhuxudong Bug: #elif arm 继承前一 arm 的 branch tag → 极性错误 |
已修复 (commit b435e83 — sentinel 降级) |
总结
增量审查(第二十轮,最终轮)。审查 commit b435e83c7(fix: degrade #elif arm to sentinel branch)。
b435e83c7 — #elif sentinel 降级
问题:ca950efca 中 #if push sentinel + #endif pop 已经修正了栈深度,但 #elif 被遗漏。在 #ifdef A / #define X1 / #elif B / #define X2 / #endif 中,X2 继承了上一 arm 的 [A=true] tag — 这恰恰是 X2 不活跃的条件。zhuxudong 在 review comment 中精确报告了此 bug,并指出 FragmentPBR.glsl:188 的 #ifdef RENDERER_HAS_NORMAL / ... / #elif defined(HAS_DERIVATIVES) 是典型的引擎内部 repro case。
修复策略对比:
| 策略 | 描述 | 适用范围 | 结论 |
|---|---|---|---|
Flip polarity(类似 #else) |
[A=true] → [A=false] |
仅第一个 #elif 正确;#ifdef A / #elif B / #elif C 会在 [A=true] 和 [A=false] 之间 ping-pong |
✗ |
| Sentinel 降级 | 替换栈顶为 {name:"", defined:true} |
对所有链长度均正确 — empty-name 在 isVisibleFrom 中透明 |
✓(选择此方案) |
| 完整条件建模 | 为 #elif expr 建模表达式 |
精确但需完整 constant expression evaluator | 过重,不值得 |
Sentinel 降级是最小正确方案。代价是精度损失:#elif arm 下的 #define 的 branch stamp 不再携带具体约束(因为 sentinel name="" 被 isVisibleFrom 忽略),但绝不会产生 false negative(不会错误排除可达定义)。False positive(让不可达定义通过)在 #ifdef A / #elif B 的场景中发生 — callsite 在 #ifdef A 下时,X2(branch=sentinel)不会被排除,而理想情况下应该排除(因为 #elif B 隐含 !A)。但这只是精度损失,不是正确性问题 — 最终 GLSL 的 #ifdef guard 仍然保证运行时正确性,这里只是编译期 codegen 可能多生成一些不可达 arm 的代码,不影响运行结果。
实现细节分析:
case Keyword.MACRO_ELIF:
if (this._branchStack.length > 0) {
this._branchStack[this._branchStack.length - 1] = Lexer._IF_SENTINEL;
}
break;-
length > 0guard:防御畸形输入(裸#elifwithout prior#if/#ifdef)下标溢出。与#else分支的if (top)guard 一致。✅ -
共享
_IF_SENTINEL引用安全性:#elif写入 sentinel 引用到数组槽位。#else在同一轮(如果#elif后跟#else)会读取 sentinel 并创建新对象{ name: top.name, defined: !top.defined }替换槽位 — 不修改 sentinel 本身。_branchStack.slice()在 L194 复制引用而非深拷贝,但 sentinel 作为 frozen-by-convention 的共享对象,引用复制是安全的。✅ -
时序:
#eliftoken 自身的 branch stamp 在 L194 处发生(switch 之前),携带的是上一 arm 的签名。#elif之后的 token 看到 sentinel。这与#ifdef的行为对称(#ifdeftoken 不含自己打开的约束),语义正确。✅ -
#ifdef A / #elif B / #else / #endif场景:#ifdef Aarm:[A=true]✅#elif Barm: sentinel(降级后) — 保守正确#elsearm: flip sentinel{name:"", defined:true}→{name:"", defined:false}— 两个极性都被isVisibleFrom忽略,透明 ✅#endif: pop,栈深度正确 ✅
-
JSDoc 更新:L196-200 精简了旧注释中关于 "
#elifdoesn't change depth, no case is needed" 的错误论断。L212-220 的新注释精确解释了 ping-pong 问题和 sentinel 降级的设计理由。L227-229 更新#else注释覆盖#elif链场景。清晰。✅
测试覆盖:
新增 define-elif-polarity.shader(43 行)。测试 #ifdef USE_BR_A / #elif defined(USE_BR_B) / #elif defined(USE_BR_C) 三 arm 链,每个 arm 包含一个 #define。测试结构覆盖了 ping-pong 场景(链长度 > 2)。在 shaderLabRelease 和 shaderLabVerbose 双模式下验证。39/39 tests pass。✅
Commit message 质量:这是全 PR 中最好的 commit message。精确描述了 bug 的 root cause(继承而非覆盖)、为什么 polarity flip 不够(ping-pong)、为什么选择 sentinel degradation、以及回溯指出 ca950efca commit message 中的错误论断。对未来维护者极有价值。
Nit
-
Nit — ShaderLab.test.ts:475-477 多余空行
define-elif-polarity和frag-return-vec4之间有 3 个多余空行(应为 1 个)。这个 nit 从 Round 19 就存在,Round 20 新增测试注册后从 2 行增长到 3 行。无功能影响,建议清理。
P2(重述,无变更)
- P2 —
_chunkOutputCache静态 Map 无限增长:Preprocessor.ts:36。编辑器场景 concern,运行时安全。 - P2 —
_preRegisterGlobalMacroRefsdouble codegen:GLESVisitor.ts。宏数量小,绝对成本可接受。
P3(重述,无变更)
- P3 —
_branchStack.slice()per-token 分配:Lexer.ts:194。copy-on-write 可优化但实际嵌套深度极浅。 - P3 —
_defineHasValue命名误导:建议_defineNeedsAstPath()。 - P3 —
_scanMacroDefineParamsbuffer push + join:连续子串可用 slice 替代。
总体评价
APPROVE。
b435e83c7 关闭了 zhuxudong 报告的最后一个语义 bug:#elif arm 继承前一 arm 的 branch tag。Sentinel 降级是三种方案中唯一对所有链长度均正确的设计(flip 会 ping-pong,完整表达式建模过重)。实现极其精简 — 3 行核心代码 + 15 行注释。
全 PR 共 21 commits, ~4200 行 diff, 39 个测试,全部通过。无 P0/P1。剩余 P2/P3 均不阻塞。
20 轮 review 共发现并关闭 31 个问题(含 7 个 P1)。PR 从初始的双解析器架构一路演进到当前的 Lexer 单一真相源 + 正向 . 路由 + BranchSignature 精确过滤 + sentinel stack balance,每一步都向更正确、更简单的方向收敛。最后两个 commit(ca950efca + b435e83c7)将 #if/#elif/#endif 栈管理做到了 minimal-correct — 不建模表达式,只保证深度和极性不被继承。
Ship it. 🚀
Bring in commit a6f0504 (PR galacean#2974) promoting `#define` values into the AST pipeline so macro-as-type-alias and struct-member references stop poisoning downstream type inference. Conflict resolution: - Map all changes from `packages/shader-lab/` → `packages/shader-compiler/` (and `tests/src/shader-lab/` → `tests/src/shader-compiler/`) per our earlier rename (commit ee6e8c0). - Adopt dev/2.0's simplified `Preprocessor` (#include expansion only; `#define`/`#ifdef` move into the Lexer's tokenizing pass) on top of our prior `basePath` removal — `Preprocessor.parse(source)` is now single-arg and looks up include keys verbatim. - Take dev/2.0's `MacroDefineInfo` shape (with `valueAst` + `branch` + `referenceName`) — replaces the legacy `MacroValueType` machinery. - Take dev/2.0's `_referenceProp` helper consolidation in VisitorContext. - Take dev/2.0's `canElide` optimization in lalr/Utils. - Take dev/2.0's `_skipBlockComment` / `_skipLineComment` split in BaseLexer; drop the now-dead `ShaderCompilerUtils.skipComment` import. - Apply the rename consistently to merged content: `ShaderLab*` → `ShaderCompiler*` for class/utils references; `shaderLab*` → `shaderCompiler*` for test-local instance variable names. - Drop the trailing `""` basePath argument from new `_parseShaderPass` call sites — our signature is 4-arg now. Existing FXAA_11 workaround (commit a69041d) still on top — to be reverted in the next commit now that the proper macro AST fix lands.
TL;DR
本 PR 把
#define分成两类处理:#define UV v.v_uv、#define PI 3.14、#define SAMPLE(x,y) texture2D(x,y))→ 提升为 AST 子树。和内联代码走同一条 visitor 管线,varying flatten、类型推导、引用追踪全部免费复用。#define HP highp、#define TEX2D_PARAM(x) mediump sampler2DShadow x)→ 保持原样,走既有的不透明 lexeme 路径,由 GLSL driver 负责展开。一句话:表达式宏变成一等 AST 节点参与常规编译流程,声明式宏维持原路径。分类由 Lexer 里一次便宜的关键字表 peek 完成。
这是 #2967 regex-scan-lexeme 方案的 AST-first 替代方案。
Trade-off 一句话
2. AST-form 宏值成为一等公民,为未来 pre-compile / 多后端(WGSL 等)奠定基础
3. 顺带修正了 release build 里 27k 个 TrivialNode 空壳包装的历史浪费
4.
#define路由从 keyword 白名单改为成员访问检测,简单常量宏跳过 expression 优先级链Runtime 零影响是关键判断:用户游戏帧率不受任何影响。variant 切换走
ShaderMacroProcessor.evaluate、.gspbytecode 旁路 shader-lab、Shader.create每 pass 只 parse 一次。编译时净提升 19-37% 来自两个结构性优化:
#define路由按成员访问而非 keyword 白名单:AST 路径的存在意义是处理含.成员访问的宏(varying flatten);其他形态(简单常量、type alias、构造器调用、qualifier 片段)driver 文本替换即可,无需进 18 层 expression 优先级链。vs
dev/2.0baseline 一览(release build,50 samples × 4 rounds,median-of-medians,同 session 采集):AST 节点数 −50%(PBR 从 ~51k 降到 ~25k)。runtime 零影响。
宏处理流程:改造前 vs 改造后
改造前(dev/2.0 baseline + #2967 方案)
flowchart TD A["#define NAME value"] --> B[Lexer:整条指令<br/>打包成一个不透明 token<br/>MACRO_DEFINE_EXPRESSION] B --> C[Parser:作为一个 token 规约<br/>内部无结构] C --> D[语义分析:MacroDefineInfo.value = 字符串<br/>调用点按字符串根<br/>做朴素标识符查找来推类型] D --> E[预 codegen:4 次扫描构建<br/>_structVarMap +<br/>_globalStructVarMap] E --> F["Codegen:对 macro lexeme 跑<br/>/\b(\w+)\.(\w+)\b/g 正则<br/>按文本剥除前缀"] F --> G[Emit:宏文本内联输出<br/>可能产出 '{ #define X Y'<br/>同一行 — GLSL ES 3.0 §3.4 拒绝] style A fill:#fff8dc style F fill:#ffcccc style G fill:#ffcccc改造后(本 PR)
flowchart TD A["#define NAME value"] --> B{"Lexer peek:<br/>value 首词是不是<br/>_expressionLeaderKeywords<br/>以外的 GLSL 关键字?"} B -->|是 — 声明式宏<br/>例如 highp、uniform、struct| L[发射一个不透明 token<br/>MACRO_DEFINE_EXPRESSION] B -->|否 — 表达式宏<br/>非关键字或白名单 28 个之一| C[发射 token 流:<br/>MACRO_DEFINE · ID ·<br/>MACRO_DEFINE_PARAMS? ·<br/>value tokens · MACRO_DEFINE_END] C --> D[Parser:新的 macro_define CFG 规则<br/>value → assignment_expression<br/>→ 完整 AST 子树] D --> E[语义分析:<br/>MacroDefineInfo.valueAst = 子树<br/>MacroCallSymbol.hasAstValue = true] E --> F[预 codegen:单次扫描<br/>_collectAllStructVars<br/>→ pass 作用域的 _structVarMap] F --> G[Codegen:AST 遍历走<br/>visitPostfixExpression —<br/>和内联代码同一路径<br/>重写 v.v_uv → v_uv] G --> H["Emit:visitMacroDefine 输出<br/>'\\n#define NAME value\\n'<br/>— 独占物理行"] L --> M[逐字输出<br/>由 driver 负责展开] style A fill:#fff8dc style B fill:#e0f0ff style C fill:#d4f4dd style D fill:#d4f4dd style E fill:#d4f4dd style F fill:#d4f4dd style G fill:#d4f4dd style H fill:#d4f4dd style L fill:#f0f0f0 style M fill:#f0f0f0关键差异
AssignmentExpressionAST 子树(表达式宏)v.v_uv→v_uv)/\b(\w+)\.(\w+)\b/g正则visitPostfixExpression— 与内联v.v_uv同一代码路径一句话总结。 baseline 从 lex 到 emit 始终把宏 value 当作文本,所以任何语义操作都需要外挂的文本重写。本 PR 从 step 2 起把 value 提升进既有 AST 管线,step 3–6 直接复用内联代码的机器。声明式宏(value 不是表达式形状)按设计继续走不透明路径 ——
highp、uniform、struct等本就无法被解析为assignment_expression,交给 driver 展开是正确行为。为什么要做
#define的 replacement list 里写成员访问(#define UV v.v_uv)、构造器调用(#define TBN mat3(v.t, v.b, v.n))、函数调用(#define SAMPLE(x,y) texture2D(x,y))等表达式,是 GLSL ES §3.4 预处理器规范允许的标准写法,不是某种引擎特有的风格。ShaderLab 作为 GLSL 超集 DSL,应当正确处理这些写法;baseline 的正则改写方案在多种情况下失效。典型的用户可见问题(和 #2967 目标一致):
normalize(FSInput_worldNormal)在 builtin 重载解析中失败,因为宏调用点的类型泄漏成Varyings#define里的v.v_uv没被重写 → 最终 GLSL 里报 undeclared identifierVaryings o;被错误地输出为uniform Varyings o;两个 PR 都修了上面这些。差别在于怎么修。
与 #2967 对比
机制对比
AssignmentExpressionAST 子树/\b(\w+)\.(\w+)\b/g正则visitPostfixExpression— 与内联代码同一路径MacroValueType.MemberAccess标记 +_isMemberAccessMacro跳过类型解析的 hackMacroCallSymbol.hasAstValue标记;AST 子树自然驱动调用点的类型_structVarMapper-function +_globalStructVarMap跨 stage)_buildGlobalStructVarMap+_collectStructVars+_collectStructVarsFromBody+_extractVarNamesFromInitDeclaratorList(四次遍历)_collectAllStructVars(单次遍历)_forEachMacroMemberAccess+_walkMacroChildrenAssignmentExpression.codeGen自然触发既有的referenceVarying/Attribute/MRTPropvoid main() { #define FRAG_UV v_uv(同一行,违反 GLSL ES 3.0 §3.4)#define独占物理行相对于 #2967 修复的具体 bug
void main() { #define X Y同行输出被 HEADLESS ANGLE 拒绝(GLSL ES 3.0 §3.4:#前只能是空白)define-struct-accessCI 失败_collectStructVars按函数清空_structVarMap→ 函数体内#define引用模块级Varyings o;时重写失效referenceStructPropByName对未知属性静默剥前缀 → driver 报 "undeclared identifier",定位困难#define GET_UV(input) input.v_uv会把input当模块符号查_buildGlobalStructVarMap的symOut数组跨 rootName 查询未重置 → 旧符号污染后续匹配系统性消除的 bug 类别
除上述 5 个具体 bug 外,AST-first 方案从构造上消除了整类正则文本重写的固有失效。正则
/\b(\w+)\.(\w+)\b/g零语义感知 —— 无法区分 struct 字段访问 / swizzle / 函数参数 / 字符串内容。正则方案的具体失效模式(不完全列举):#define MSG "failed at v.v_uv"→ 正则重写字符串内容#define X v_uv // see o.v_uv→ 正则重写注释文本#define GET(v) v.something,其中v恰好与某个 fragment stage 参数同名v.v_normal.xyz—— 哪一段是 struct 字段、哪一段是 swizzle?x/r/s与 swizzle.x/.r/.s冲突v与局部int v,正则只看文本mix(a.xyz, b.xyz, v.v_blend)—— 每个匹配独立、无交叉引用arr[i].field—— 正则不理解索引#ifdef分支重定义:同名宏在不同分支、展平视角下歧义合计 30+ 种失效模式。AST codegen 全部正确处理,因为驱动重写的是语义不是文本。这些模式在 GLSL ES 规范里都是合法的预处理器用法(
#define的 replacement list 允许任意 token 序列),ShaderLab 作为 GLSL 超集 DSL 应当全部正确处理。复杂 shader(PBR、water 等)稳定命中多条。性能
使用
tests/src/shader-lab/PrecompileBenchmark.test.ts对shaderLab._precompile()端到端计时。测试模式:release build(
NODE_ENV=release,// #if _VERBOSE块被 jscc 剥掉)—— 这是用户实际跑的模式。verbose build 下 TrivialNode elision 不触发(astTypePool非空),本 PR 的性能提升基本不存在(节点数照常创建)。测量方法:50 samples per round × 4 independent rounds,每个 sample 是单次
_precompile()全链路(Preprocessor + Lexer + Parser + CodeGen + Encoder)。每轮取 median,再对四轮 median 取 median-of-medians。Runtime 零影响 —
Shader.create每 pass 只 parse 一次,variant 编译走ShaderMacroProcessor.evaluate不重 parse,.gspbytecode 旁路 shader-lab。以下是编译时(编辑器 / pre-compile 工具链)成本:dev/2.0 baseline vs 本 PR(release build,median-of-medians, ms)
四轮各自 median 一致性(PBR 例):base 49.30 / 49.40 / 50.00 / 50.50(σ ≈ 0.5ms),HEAD 38.70 / 38.80 / 38.80 / 39.00(σ ≈ 0.1ms)。Δ 信号远超 run-to-run 抖动。
收益来源:两个结构性优化叠加
94fb096c9):消除 27k 个 release build 下的空壳 wrapper 节点 → AST parser 和 CodeGen 各 −25% 量级。#define成员访问路由(commit992956ffe):所有简单常量宏(#define PI 3.14、#define HALF_MIN 6.103e-5)和 type alias 宏(#define FxaaFloat2 vec2)从 AST 路径回到 legacy 路径,跳过 18 层 expression 优先级规约链。macro-pre 的 −33% 主要来自这条。成本来源:分阶段 breakdown(PBR, median ms)
AST 节点数对比
PBR 编译创建的 AST 节点总数:
关键:TrivialNode elision
AST parser 和 CodeGen 双双 −25% 来自一个 16 行的结构性修复。
问题:GLSL 语法的 expression precedence 链(
logical_or_expression→logical_xor_expression→ ... →primary_expression)有 ~15 层单产生式(LHS → single RHS 仅表达优先级/结合性)。每层在 LALR 规约时被 CFG 里// #if _VERBOSE ASTNode.XxxExpression.pool // #endif标注 typed class。release build 下// #if _VERBOSE被 strip,astTypePool为undefined→ fallback 到通用TrivialNode。这些 TrivialNode 没有 visitor、没有 type 字段、没有副作用,纯粹是 children 的 pass-through 包装。PBR 在 release 下产生 27k 个这种空壳。修复:在
GrammarUtils.createProductionWithOptions的 reducer closure 里,当astTypePool为空 + RHS 单 NonTerminal 时,直接把 child 压入 semantic stack,不创建 TrivialNode 包装。parser 的 GOTO 表按reduceProduction.goal驱动(不看 stack 上 node 类型),所以状态机行为完全不变。行业标杆:glslang / GCC / Clang / Rust parser 都采用"precedence-only productions 不实体化 AST 节点"的做法(operator precedence parsing / 规约时 fold)。本 PR 让 ShaderLab 的 release build 对齐这个行业做法。
影响范围:
astTypePool非空,永不触发 elision)instanceof TrivialNode依赖(grep 过),collapse 完全透明instanceof TreeNode守卫防御单 Token RHS(如unary_operator → PLUS)代码改动:
packages/shader-lab/src/lalr/Utils.ts+14/-4 行。对标 #2967
详细分阶段数据和权衡讨论:#2974 (comment)
代码规模
排除测试,仅
packages/shader-lab/src/,对比 dev/2.0:macro_define产生式 + 3 个 Keyword token净增 +645 行做了三件大事 + 多个 review 修复:
#define一等公民 AST 化(解决 30+ 失效模式)加的去向(+874)
#definetoken 流状态机 +_defineHasValue成员访问路由MacroDefinecodegen + 宏值参与 IO-struct flattenMacroDefine注册 /MacroCallSymbol语义分析 /isFunctionLikeMacroMacroCallFunction两种 call shape 分支(review 修复)MacroDefineInfo重构 + 缓存合并(净 −57)macro_define/macro_call_function等产生式MACRO_DEFINE*token删的主要去向(−229)
MacroValueTypeenum、_isNumber、双正则、双缓存(被referenceName+_chunkCache取代)_expressionLeaderKeywords白名单 + 旧分支判断(被成员访问检测取代)referenceStructPropByName死代码 + 其他冗余visitFunctionCall分支被新结构吸收跨层分布是必要的
CFG 核心增量只是 1 条产生式(两个 rhs 共 9 个 token 的语法扩展)。其余行数都是让这条规则能跑通所需的 Lexer / AST / Codegen 最小适配 —— 每层做它本该做的事。AST-first 的代价是结构化改动跨多层,收益是每个新场景零增量(regex 方案则需要每个新场景加特判 + 新 bug 风险)。
实现要点
Lexer(
packages/shader-lab/src/lexer/Lexer.ts)#definevalue 的首词与既有关键字表对比。是关键字 且 不在 expression-starter 白名单 → 声明式宏,发射MACRO_DEFINE_EXPRESSION(legacy 不透明)。否则 → 表达式宏,发射MACRO_DEFINE、ID、可选MACRO_DEFINE_PARAMS、value tokens、MACRO_DEFINE_END。MACRO_DEFINE_PARAMS把完整的(params)块作为一个不透明 token 捕获,让 CFG 规则保持 LALR(1) 兼容(避开与function_call_parameter_list的 shift/reduce 冲突)。Grammar / AST(
packages/shader-lab/src/lalr/CFG.ts、parser/AST.ts)macro_defineCFG 规则,复用既有assignment_expression非终结符作为 value。ASTNode.MacroDefine持有可选的AssignmentExpression子树。MacroCallSymbol.hasAstValue在semanticAnalyze中赋值;VariableIdentifier.semanticAnalyze利用这个标记对 AST 形式的宏保留调用点类型为TypeAny,让 builtin 重载解析正常工作。MacroDefineInfo.valueAst在预处理器入口挂载 AST;legacy 字段对不透明路径保持不变。Codegen(
packages/shader-lab/src/codeGen/*.ts)visitMacroDefine调用valueExpression.codeGen(this)—— 成员访问重写在visitPostfixExpression里递归发生,与内联代码同一路径。GLESVisitor._collectAllStructVars在两个 stage codegen 前预填_structVarMap,覆盖所有类型为角色 struct 的变量(入口函数参数/局部变量 + 模块级全局)。让前向声明的全局(Varyings o;出现在引用它的#define之后)也能被正确重写。visitPostfixExpression在 AST 静态类型之外,额外检查_structVarMap的 bare 左侧 identifier,这样前向声明场景也能工作,且不破坏嵌套访问(swizzle)。getStructRole统一了 4 处散落的attribute/varying/mrt3 个并列isXxxStruct判断。辅助变更
SymbolTable.forEach公开 API 替代(as any)._table访问。ParserUtils.extractDirectIdentLexeme/parseMacroParamList放置小型 helper。致谢
测试场景(shader 源码和
it(...)块结构)来自 @zhuxudong 的 #2967 —— 场景覆盖#define成员访问等 GLSL 规范合法用法,没必要重造。预期 GLSL 快照按 AST-first 的输出格式重新生成。#2967 的实现代码未被复用。Test plan
pnpm build && pnpm test— 1301/1301 通过(包含 dev/2.0 的 fix(shader-lab): resolve generic return type for texture/builtin functions #2966 合并进来)#define成员访问测试 + 1 个构造器-value 测试 + 3 个回归测试 + 4 个 dev/2.0 测试含 fix(shader-lab): resolve generic return type for texture/builtin functions #2966 generic-return-type)TargetParser.y— 仅 1 条 dangling-else shift/reduce(baseline 已存在),无新增冲突Review 中发现并修复的问题(按时间顺序)
vec4(v.x)、mat3(v.t, v.b, v.n))被误归 legacyb248dc763Set<string>存在双真相 → 改用Set<Keyword>0a056ae99#define FOO (1 + 2)被误判为 function-like(空格前()79511dce1MAX3(v.x, v.y, v.z)实参被 filter 错误丢弃 → 按isFunction分流fdcb7ea1d#define PAREN (/#define COMMA ,等非表达式值走 AST 失败8f589ab69TargetParser.y未随 CFG 同步,bison 无法验证828ac81e1MacroDefineInfo双路径冗余导致每次访问发假 warningb09fd5019!!valueRaw守卫8cd50348e_expressionLeaderKeywords白名单把vec2等类型 keyword 误判为合法 expression leader → FXAA portability 宏炸;改为成员访问检测正向路由992956ffe_defineHasValue单字符 lookback 把v0.x误判为小数点 → 走完整 alnum run 看开头0d7f3528b#define处理统一收到 Lexer 单 pass 注册3668d8c5a已知局限
ShaderLab scope 下,本 PR 覆盖了绝大多数
#define用法,但以下三个场景仍未覆盖。它们既不是本 PR 引入的回归,也都不是 #2967 解决的场景 —— 都是独立的长期问题:1. 函数式宏的 struct 形参
根因:ShaderLab 不展开宏,交给 GLSL driver 展开。driver 展开时 ShaderLab 的 IO flatten 已完成(顶层
v已被消解成varying vec2 v_v_uv),于是v.v_uv找不到v。形参input和调用点实参v跨 scope,ShaderLab 阶段无法关联。Workaround:用普通 GLSL 函数替代函数式宏:
对比 Slang:Slang 采用
Preprocessor → Lexer → Parserpipeline,在 Parser 之前完全展开宏。展开后 AST 里就是v.v_uv,v能被语义分析正确识别。要让 ShaderLab 学 Slang 需要重写整个宏处理子系统(实现完整 C preprocessor 语义 + 维护源码 source map),ROI 低于独立建议用户用普通函数替代。2. 多行
\续行#define含成员访问当前行为:Lexer 的
_defineHasValuepeek 遇到\+ newline 显式返回 false → 走 legacy → 成员访问不被 flatten。设计决策:注释声明 "rare; simpler to stay opaque"。真实 shader 里多行
#define含成员访问几乎不出现。Workaround:合成一行即可走 AST 路径。
3. 跨
#ifdef分支的同名宏重定义 AST/legacy 混合潜在隐患:
hasAstValue/isFunctionLikeMacro判断基于some(...),任何一个分支是 AST 形式,调用点就按 AST 形式处理。真实影响:极低。同名宏在不同分支里值的"形态"通常一致(要么都是表达式要么都是声明),真实代码里此类混合未见。
未来:如果需要,可在
MacroCallSymbol上做按分支精细判断,独立改动。关于 #2967
两个 PR 都能修用户可见的 bug。如果倾向 AST-first 方案,#2967 可以关闭。否则 #2967 仍需修 CI blocker(
{与#define同行问题)和 4 项 CodeRabbit findings —— 而正则重写仍会持续暴露于上文列出的失效类别。Summary by CodeRabbit
New Features
#definedirectives and function-like macro parameters; macro values now participate in semantic analysis and codegen, improving generated GLSL.Bug Fixes
#defineforms are preserved verbatim where needed; struct-typed globals used as attribute/varying/MRT no longer emit incorrectuniformdeclarations.Tests