feat(shader): add neutral IR and standalone diagnostics - #3054
feat(shader): add neutral IR and standalone diagnostics#3054zhuxudong wants to merge 160 commits into
Conversation
- move createPosition/createRange + their pools to ShaderCompilerUtils - move pass-text error context to ShaderCompilerUtils.processingPassText - add ICodeGenVisitor interface so AST no longer imports concrete CodeGenVisitor - parser/lexer/codegen now depend on ShaderCompilerUtils, not ShaderCompiler entry prep for extracting shared shader-parser package (c3); no behavior change, 197 tests green
- copy ClearableObjectPool/IPoolElement into local common/ObjectPool - add local no-op Logger (engine-core Logger is also disabled by default) - copy render-state enums into common/enums/RenderStateEnums (values mirror engine-core) - parser/lexer/lalr/sourceParser now engine-core-free; engine-math (Color) kept as foundation dep deviates from RFC: Color kept as engine-math dep, render-state enums copied; 197 tests green
…mpiler - move lexer/preprocessor/parser/lalr/AST/sourceParser + utils into @galacean/engine-shader-parser - shader-compiler depends on it; cross-package imports go through the package barrel - shader-parser ships one always-full build (jscc _VERBOSE=true), external to shader-compiler - shader-parser drops stripInternal so compiler/analyzer can use internal parser APIs pure relocation; 197 shader-compiler tests green
…gnostics - new @galacean/engine-shader-analyzer drives the parse + collects diagnostics, skips codegen - restores diagnostics the runtime compiler discards (parity verified vs verbose compiler) - harvest approach: checks stay in shader-parser (single source), no visitor duplication - Phase 1 returns GSError verbatim; structured API + new checks are Phase 2 harvest deviates from RFC's DiagnosticVisitor plan; 199 tests green
- analyzer now runs codegen too, capturing codegen-level diagnostics (struct/MRT/gl_FragData) - ungate codegen error collection so the single release build always collects them - remove ShaderCompiler._logErrors + calls: the compiler compiles, never reports - delete the verbose build variant (/verbose export, rollup push, stub dir) - shader-compiler drops stripInternal + exports GLES visitors so analyzer can drive codegen completes Phase 1: diagnostics live in the analyzer; 200 tests green
- drop unused ObjectPool.garbageCollection (pools reuse via clear(), never GC) - remove dead verboseMode branches from root rollup (no verbose build remains) - collapse duplicate glslValidate calls left by the verbose→release test switch - drop an obsolete warning-spy guard (the warning no longer exists; macro asserts cover it) - tighten comments: drop task-context and a claim of a non-existent sync test
- remove unused abstract ObjectPool base class (only ClearableObjectPool extends it; inline the two fields) - replace indirect ReturnType<typeof ShaderSourceParser.parse> with IShaderSource
- Diagnostic interface (severity, code, range, message, source, relatedSource) - DiagnosticCode registry: C0 (parser/codegen), A1 (ShaderLab), B1/B2 (RenderState) - gseErrorToDiagnostic converts GSError to structured Diagnostic - ShaderAnalyzer.analyze() returns AnalysisResult.diagnostics: Diagnostic[] - heuristic code mapping from GSErrorName + message content - tests verify structured output for all 3 diagnostic sources
- reportWarning routed to Logger.warn, a noop since Phase 1 decoupled Logger - the "declared before used" warning was silently dropped as a result - now push CompilationWarn to errors[] (gated by _VERBOSE, like reportError) - analyzer surfaces it as a C0-07 warning diagnostic; drop unused Logger import
- a failed function lookup is signature-keyed, conflating unknown names and wrong-arg calls - both surfaced as one opaque "No overload function type found" message - re-probe by name alone (+ builtin registry) to split the two cases - unknown names now report a distinct "Undefined function" (C0-09); wrong-args keeps C0-06
- shader-parser always builds _VERBOSE=true, so its 88 #if _VERBOSE blocks were dead scaffolding - the guarded code (diagnostics, line/column tracking) already shipped in every build - strip all markers + drop the 2 dead #else console.error fallbacks - dist and behavior identical to before; 202 shader tests stay green
- rollup _VERBOSE jscc is now a no-op (zero #if _VERBOSE left repo-wide) — remove it + the import - VisitorContext location: any -> BaseToken["location"]; IRenderState drops the pointless | any - BaseLexer throwError msgs: any[] -> unknown[] (only ever join()'d) - map the "referenced X not found" codegen error to a dedicated C0-22 instead of the C0-08 fallback
- SymbolTable.insert silently overwrote a same-scope duplicate via a now-noop Logger.warn - insert() now returns whether it replaced an equal symbol; decl sites surface it as a C0-10 warning - macro-branch siblings stay exempt (insert skips isInMacroBranch entries) — covered by a test - applies to local (SingleDeclaration/InitDeclaratorList) and global (VariableDeclaration) vars
- add PostfixExpression.semanticAnalyze: a `.field` on a known vector is validated as a swizzle - catches out-of-range components (.z on vec2), mixed sets (.xr), bad chars, length > 4 - only fires when the base type is a concrete vecN — struct members and unresolved bases skip - ParserUtils.swizzleError holds the rule; first C1 (GLSL type) layer check
- AssignmentExpression flags `a = b` when b's type cannot convert to a's - ParserUtils.isAssignable models GLSL ES3 implicit conversions (int->float, ivecN->vecN) - so valid coercions (float = int) are NOT flagged; only definite conflicts surface - fires only when both operand types are concrete; compound RHS / structs are skipped
- JumpStatement.semanticAnalyze checks `return expr` against the function's declared return type - reuses ParserUtils.isAssignable, so implicit conversions (return int from a float fn) pass - skips void returns (C0-04 covers those) and unresolved/compound expressions
- ShaderTargetParser singleton: a failed parse (syntax error) left _traceBackStack dirty - the next parse was then corrupted: a valid shader got a spurious diagnostic - this hits the runtime compiler too (ShaderCompiler/ShaderAnalyzer share the singleton) - clear _traceBackStack each parse; reset SymbolTableStack._macroLevel in clear() too - regression test: a broken analyze() must not corrupt the following valid one
- registerRule(rule) runs user rules after the built-in checks on every analyze() - rules get source + parsed structure + positionAt(); report() namespaces the code as <name>/<code> - a throwing rule surfaces a <name>/rule-error warning instead of crashing analysis - analyze() restructured so rules run even when structure parsing fails (built-in path unchanged)
- src/shader-playground.ts: a live editor + diagnostics panel driven by ShaderAnalyzer.analyze() - no engine init (analyzer is standalone); the sample shows C0-09/10 and C1-01/02/03 - also demos registerRule via a "demo/no-discard" custom rule - wire engine-shader-analyzer into examples deps + vite optimizeDeps exclude
- upgrade shader-parser's local Logger to a real controllable one (enable/disable, off by default) - keeps zero engine-core dependency (local copy mirroring engine-core's API) - route runtime console.* through it: error prints -> Logger.error, version banner -> Logger.info - the compiler version banner no longer prints on every import (silent unless logging enabled) - remove dead debug dumpers printStatePool / _printStack (uncalled) and their console - bundler CLI keeps console (build-time terminal output); shader-analyzer had no bare console
- local common/Logger.ts existed to avoid an engine-core dep, but core never imports shader pkgs - core injects shader-compiler (no import), so there was never a cycle to avoid - depend on engine-core and use its Logger; redirect 4 parser + 1 compiler imports, drop the copy - logging now unifies with the engine's Logger; 1428 tests pass, compiledShaders byte-identical
- the diagnostic package now logs each diagnostic via the engine Logger, off by default - severity-mapped: error->error, warning->warn, info->info, hint->debug - add @galacean/engine-core dep; analyze() logs after collecting all diagnostics - enable Logger to see every syntax/semantic problem in the console while analyzing
…cal copy - common/ObjectPool.ts was a local copy of core's pool (made to avoid the now-cycle-free core dep) - core's ClearableObjectPool/IPoolElement are behavior-identical (same get/clear logic) - redirect the 6 import sites to @galacean/engine-core; drop the local copy + its re-export - 213 shader tests pass, compiledShaders byte-identical to dev/2.0
…al copy - common/enums/RenderStateEnums.ts was a hand-synced local copy of core's 8 render-state enums - drop it; ShaderSourceParser imports them from @galacean/engine-core (merged into its core import) - no re-export consumer; design types render state by number, so no enum-identity issue at boundary - compiledShaders byte-identical (render-state serialization unchanged); 213 tests pass
- rollup.config.js: drop the dead jscc plugin (no #if _VERBOSE left) + its stale verbose comments - also drop a dangling src/enums/README.md reference in that file's header - convert.ts: drop the "Phase 2 ... DiagnosticVisitor" promise (no DiagnosticVisitor was built) - Preprocessor/Lexer: fix comments referencing the removed verbose build / wrong package
- drop ShaderInstructionEncoder's hand-synced local copy of the directive enum - core now exports ShaderPreprocessorDirective publicly (values unchanged Text=0..Undef=10) - compiledShaders stay byte-identical to dev/2.0; tsc clean across the 3 shader packages
…e registry - gSErrorNameToCode returns DiagnosticCode.* refs, dropping 34 duplicated raw "C0-xx" literals - return type is DiagnosticCodeValue so tsc rejects any code absent from the registry - remove the never-passed defaultSeverity param (all five callers use the default)
- add SemanticWalker that walks the built AST and derives diagnostics from node type clues - PostfixExpression still produces the type clue; swizzle check (C1-01) leaves its semanticAnalyze - establishes parser-produces-clues / analyzer-judges pattern; first step of diagnostics decoupling - compiledShaders byte-identical; full suite 1429 pass
- IntegerConstantExpressionOperator.compute is now optional; absence = unknown-operator clue - C0-02 judgment leaves parser semanticAnalyze for the walker - compiledShaders byte-identical; full suite 1429 pass
- remove SemanticWalker; swizzle (C1-01) and operator (C0-02) judgments go back to parser - analyzer-side instanceof was a hack; judgment belongs internalized in parser clue computation - correct model: error-as-clue in parser + analyzer generic collection - compiledShaders byte-identical; 1429 tests pass
- Block source and include failures before shader code generation. - Preserve macro branch, function identity, and AST ownership invariants. - Add regression coverage for review findings.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shader-analyzer/src/ShaderAnalyzer.ts (1)
121-155: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDeep-clone every pass unconditionally, even when the pass (or a sibling pass) will be discarded.
_analyzePassalways callsthis._cloneProgram(program)before the caller knows whetheranalyze()will end up zeroingpasses(line 84:if (diagnostics.some(...error)) passes.length = 0;). For shaders with validation errors, every erroring pass — and any earlier successful pass in the same source — pays the full recursive clone cost for a result that's thrown away.♻️ Defer cloning until after the error check
- analyze(source: string, options?: AnalyzerOptions): AnalysisResult { + analyze(source: string, options?: AnalyzerOptions): AnalysisResult { ... const diagnostics: Diagnostic[] = []; - const passes: AnalyzedPass[] = []; + const rawPasses: AnalyzedPass[] = []; ... const analyzed = this._analyzePass(pass, diagnostics, options?.basePathForIncludeKey); - if (analyzed) passes.push(analyzed); + if (analyzed) rawPasses.push(analyzed); ... - if (diagnostics.some((diagnostic) => diagnostic.severity === DiagnosticSeverity.Error)) passes.length = 0; + const passes = diagnostics.some((d) => d.severity === DiagnosticSeverity.Error) + ? [] + : rawPasses.map((p) => ({ ...p, program: this._cloneProgram(p.program) })); this._logDiagnostics(diagnostics); return { diagnostics, passes }; }and drop the clone call inside
_analyzePass's success return (return the originalprogram).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/ShaderAnalyzer.ts` around lines 121 - 155, Defer cloning until after analyze() determines the final pass set: update _analyzePass to return the original program on success, then clone retained passes only after the diagnostics-based passes.length reset in analyze(). Ensure discarded erroring passes and sibling passes are not cloned, while successful passes retained in the final result remain isolated through _cloneProgram.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/shader-analyzer/src/ShaderAnalyzer.ts`:
- Around line 121-155: Defer cloning until after analyze() determines the final
pass set: update _analyzePass to return the original program on success, then
clone retained passes only after the diagnostics-based passes.length reset in
analyze(). Ensure discarded erroring passes and sibling passes are not cloned,
while successful passes retained in the final result remain isolated through
_cloneProgram.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86510004-d317-4af6-83fb-483aee114b39
⛔ Files ignored due to path filters (2)
tests/src/shader-compiler/expected/define-struct-access-global.frag.glslis excluded by!**/*.glsltests/src/shader-compiler/expected/define-struct-access-global.vert.glslis excluded by!**/*.glsl
📒 Files selected for processing (26)
packages/core/src/Engine.tspackages/core/src/animation/AnimationClip.tspackages/shader-analyzer/package.jsonpackages/shader-analyzer/src/ShaderAnalyzer.tspackages/shader-analyzer/src/ShaderValidator.tspackages/shader-compiler/package.jsonpackages/shader-compiler/src/ShaderCompiler.tspackages/shader-compiler/src/codeGen/GLES300.tspackages/shader-compiler/src/codeGen/VisitorContext.tspackages/shader-parser/package.jsonpackages/shader-parser/src/GSError.tspackages/shader-parser/src/ParserUtils.tspackages/shader-parser/src/Preprocessor.tspackages/shader-parser/src/ShaderCompilerUtils.tspackages/shader-parser/src/common/BaseToken.tspackages/shader-parser/src/parser/AST.tspackages/shader-parser/src/parser/PassParser.tspackages/shader-parser/src/parser/TypeSystem.tspackages/shader-parser/src/sourceParser/ShaderSourceParser.tspackages/shader-parser/src/sourceParser/ShaderSourceParser.ytests/src/shader-analyzer/ReuseAst.test.tstests/src/shader-analyzer/ReviewRegression.test.tstests/src/shader-compiler/AnalyzerInjection.test.tstests/src/shader-compiler/MacroBranchRuntime.test.tstests/src/shader-compiler/ShaderCompiler.test.tstests/vitest.config.ts
💤 Files with no reviewable changes (3)
- packages/shader-parser/package.json
- packages/shader-analyzer/package.json
- packages/shader-compiler/package.json
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/core/src/animation/AnimationClip.ts
- packages/shader-parser/src/parser/PassParser.ts
- packages/shader-parser/src/Preprocessor.ts
- tests/vitest.config.ts
- tests/src/shader-analyzer/ReuseAst.test.ts
- packages/core/src/Engine.ts
- packages/shader-parser/src/GSError.ts
- packages/shader-compiler/src/codeGen/GLES300.ts
- packages/shader-parser/src/sourceParser/ShaderSourceParser.y
- packages/shader-parser/src/ShaderCompilerUtils.ts
- packages/shader-parser/src/ParserUtils.ts
- tests/src/shader-compiler/MacroBranchRuntime.test.ts
- packages/shader-compiler/src/ShaderCompiler.ts
- packages/shader-parser/src/parser/TypeSystem.ts
- packages/shader-compiler/src/codeGen/VisitorContext.ts
- packages/shader-parser/src/common/BaseToken.ts
- packages/shader-parser/src/sourceParser/ShaderSourceParser.ts
- packages/shader-analyzer/src/ShaderValidator.ts
- tests/src/shader-compiler/ShaderCompiler.test.ts
- packages/shader-parser/src/parser/AST.ts
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
Request changes。已按 4e33d492...6cb9760a 审完本轮 28 个增量文件,并重新沿 Shader.create/Engine → compiler/preprocessor → source/pass parser → analyzer/IO → codegen/precompile 各追一层。当前 checks 全绿;本轮已合理关闭 overload call graph、pooled AST lifetime、bare return、bool const、strict bound、cross-stage fallback、missing include 阻断、browser entry 以及 Copilot 指出的 undefined 输出等问题。但 source/diagnostic ownership、可达性和 declarator/type validation 仍存在平行 owner,另有一个会静默复用错误 include 展开结果的新阻塞问题。
阻塞问题
-
[P1]
packages/shader-parser/src/Preprocessor.ts:114-118— include cache 的 key 少了会改变展开结果的 base path。_replace只用解析后的path命中ChunkOutputCache,但 miss 时又用调用方的basePathForIncludeKey递归展开 chunk;而ShaderCompiler.ts:18-19,55-60的 cache 会跨多个Shader.create调用长期复用。于是两个不同目录的 shader 引用同一个 chunk、且该 chunk 内再#include "./local.glsl"时,第二个 shader 会直接拿到第一个 shader 的 nested include 内容(或错误),静默编译成错误源码。应让“当前被 include 文件的 canonical URL/path”成为唯一 owner:递归时以该文件 URL 作为下一层 base,使展开只依赖 canonical path,再按 canonical path 缓存;删除“同一 cache entry 仍依赖 root shader base”的双源,并补两个 root path 顺序互换的回归。 -
[P1]
packages/shader-compiler/src/ShaderCompiler.ts:21,37-53/packages/shader-analyzer/src/ShaderAnalyzer.ts:94-102,140-147— source diagnostics 仍未进入 parse result,而是新增了跨调用隐藏状态,range/source 协议也仍有两套。_parseShaderSource只返回IShaderSource,却把错误挂到 compiler 实例的_sourceErrors;因此_parseShaderPass的结果取决于“上一次 parse source”,一个 bad source 后直接编译独立 valid pass 也会被无条件拒绝。analyzer 侧又把原始 ShaderLab 的 entry range 配给重建后的passText,compiler 注入路径则不传 range、退化到 0:0;GLESVisitor仍第二次运行ShaderIOAnalyzer并保留_softMissEntry空 stage 路径。请保留ShaderSourceParser作为 source error、entry binding 与原始 source mapping 的唯一 owner,返回 typed parse envelope 并让Shader.create/_precompile/analyzer/codegen机械消费;删除_sourceErrors这条平行状态、_softMissEntry,以及 codegen 对 IO facts 的第二次扫描。 -
[P1]
packages/shader-parser/src/parser/AST.ts:92-104,1115,1818-1825/ShaderIOAnalyzer.ts:90-118— branch/reachability 修复只过滤了静态 dead IO token,没有修复控制流 owner。TreeNode.set仍会跳过 unconditional child、把任意后代第一个 non-empty branch 当成父节点 branch;ShaderValidator._walk也仍遍历并诊断不可达节点,所以 vertex 中#if 0下的dFdx仍会产生阻塞错误。反向上,gl_Position/gl_FragColor仍是 program-global range bag:未被 entry 调用的 helper 写一次gl_Position就能掩盖MissingVertexPosition,unused helper 的gl_FragColor也能伪造 MRT 冲突。请保留 lexer token 的BranchSignature + isBranchReachable为唯一 branch owner,让节点机械继承真实首终结符、validator dispatch 跳过不可达节点;IO facts 按 FunctionDefinition/stage 归属并由 entry call graph 派生,删除三个 global range bag。 -
[P1]
packages/shader-parser/src/parser/AST.ts:553-585,1671-1729— declarator 的 const/initializer/array 事实仍由三条 reduction 分别维护且继续漂移。 逗号声明的InitDeclaratorList仍用默认isConst=false创建 symbol、完全不校验 initializer,并在 array 分支直接修改共享的this.typeInfo,会让后续 declarator 继承前一个 declarator 的 array shape;globalVariableDeclaration仍只校验 assignability,不校验 const expression。因此const int A=1,B=2; const int C=B;仍会误报,const float A=1.0,B=u;/const float G=u;仍会漏报,float a,b[2],c;还会把c记成 array。应以FullySpecifiedType + 每个 declarator 自己的 initializer/arraySpecifier为唯一 owner,统一 normalize/validate 后再建VarSymbol,删除三份平行的 symbol/validation 逻辑和共享SymbolTypemutation。 -
[P1]
packages/shader-analyzer/src/ShaderValidator.ts:608-674/packages/shader-parser/src/parser/TypeSystem.ts:89-113— arithmetic 修复继续维护第二套 operator/type system。 新增的_arithmeticFamily/_areArithmeticShapesCompatible在 validator 解释 operator legality,而 AST inference 仍调用不接收 operator 的TypeSystem.arithmeticResultType(a,b);例如合法mat2x3 * mat3x2在 TypeSystem 变成TypeAny,非法同型 non-square matrix multiply 又先被推成具体原类型,直到另一个 package 的 validator 才给相反结论。请让 TypeSystem 单一返回 per-operator 的 compatibility/result(含 invalid reason),parser inference 与 validator 共用;删除 validator 里的 family/shape 平行规则,而不是继续靠两边同步 case。
非阻塞项
- [P2]
packages/shader-analyzer/src/ShaderAnalyzer.ts:25-30的公开 TSDoc 仍承诺 AST “仅在下一次 analyze 前有效”,但本轮已深拷贝整个 program;当前文档会让调用者为不存在的生命周期限制额外复制或避免缓存。请把公开契约改成与实际 ownership 一致。CodeRabbit 已单独指出 error path 的无效深拷贝,这里不重复其性能项。 - [P2]
tests/src/shader-compiler/DiagnosticDriverConsistency.test.ts:873-907仍给 compiler 注入 analyzer;error case 在 WebGL oracle 前返回,因此无法证明“无 analyzer 的 codegen 与真实 driver 一致”。本轮已修正 MacroBranchRuntime 的 DSL/raw-pass 混用,剩余 oracle 应拆成 analyzer diagnostic、无 analyzer raw codegen、driver 三条独立断言。 - [P2] PR 元数据与注释规范仍未收口。 该 PR 新增两个公开 package、Analyzer API 和 Engine 配置,title 仍应从
refactor(shader)改为feat(shader);本轮新增ShaderValidator.ts:431-432等单行//仍以句号结尾,新增公开TypeSystem.matrixDimensions也没有按同族 public API 补完整多行 TSDoc、@param与@returns。
- reject custom types missing from reachable macro branches - cover variable, parameter, return, and struct-member declarations
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
Request changes。已按 6cb9760a...3aa34798 审完本轮 1 个 commit / 2 个文件,并沿 Lexer BranchSignature → TypeSpecifier/StructSymbol → SymbolTable coverage → analyzer gate → codegen/driver 各追一层。当前 checks 全绿;本轮没有关闭上一轮开放项,新增的 struct type 校验还复制了一个会错误放行宏变体的 self-guard shortcut。上一轮已关闭的 overload call graph、pooled AST lifetime、bare return、bool const、strict bound、cross-stage fallback、missing include 阻断、browser entry 和 Copilot 的 undefined 输出继续保持关闭,不重复提出。
阻塞问题
-
[P1]
packages/shader-parser/src/parser/AST.ts:442-448— 任意内层 self-guard 会抹掉外层 feature branch,缺失的 struct 仍被当成必然可见。 例如struct Data位于#ifdef FEATURE内的标准#ifndef DATA_INCLUDED / #define DATA_INCLUDEDguard 中,而Data value;在外部;FEATURE关闭时预处理结果没有Data,但 candidate signature 只要含一个selfGuardingconstraint,:447就绕过整个canBranchesCoverCallsite结果,analyzer 放行并把错误推迟到真实 driver。相同整分支 shortcut 还存在于 function/variable consumer(:967、:2029),这是同根因的平行 coverage owner。请保留BaseToken.canBranchesCoverCallsite作为唯一 branch-coverage owner:只机械消解已证明成立的 self-guard constraint,同时继续检查 signature 中的FEATURE等其余约束;删除三个 consumer 的some(isSelfGuardingBranch)fallback,并补“outer feature off/on + inner canonical guard”的 struct(以及共享 owner 的 function/variable)回归。 -
[P1]
packages/shader-parser/src/Preprocessor.ts:114-118— include cache 的 key 少了会改变展开结果的 base path。 cache 仅以解析后的path命中,miss 时却继续用 root shader 的basePathForIncludeKey展开 chunk;同一 compiler cache 跨多次创建复用后,两个目录引用同一 chunk、chunk 再相对 include 时,后一次会静默复用前一个目录的内容。请让当前 include 文件的 canonical URL/path 成为唯一 owner,递归以它作为下一层 base,再按 canonical path 缓存;删除 cache entry 对 root base 的隐式依赖,并补两个 root path 顺序互换的回归。 -
[P1]
packages/shader-compiler/src/ShaderCompiler.ts:21,37-53/packages/shader-analyzer/src/ShaderAnalyzer.ts:94-102,140-147— source diagnostics、entry binding 与 source mapping 仍由隐藏状态和平行协议维护。_parseShaderSource只返回IShaderSource,却把错误存入实例级_sourceErrors,使独立 valid pass 也会受上一次 bad source 污染;standalone analyzer 又把原始 ShaderLab range 配给重建后的passText,注入路径则没有 range,codegen 仍二次运行ShaderIOAnalyzer并保留_softMissEntry空 stage。请保留ShaderSourceParser为唯一 owner,返回携带 errors、binding source/range 与一次性 IO facts 的 typed envelope;让 compiler/analyzer/codegen 机械消费,删除_sourceErrors、_softMissEntry和 codegen 的第二次 IO 扫描。 -
[P1]
packages/shader-parser/src/parser/AST.ts:92-104/packages/shader-analyzer/src/ShaderValidator.ts:100-182/ShaderIOAnalyzer.ts:90-118— branch/reachability 与 IO facts 仍有平行 owner。TreeNode.set会跳过 unconditional child、把后代首个 non-empty branch 赋给父节点;validator 仍遍历不可达节点,IO 又使用 program-globalgl_Position/gl_FragColorrange bag。因此#if 0内 derivative 仍能阻断 vertex,unused helper 的gl_Position能掩盖MissingVertexPosition,unusedgl_FragColor能伪造 MRT 冲突。请保留 lexer 的BranchSignature + isBranchReachable为唯一 owner,让节点继承真实首终结符、validator 跳过不可达节点;IO facts 按 FunctionDefinition/stage 归属并由 entry call graph 派生,删除三个 global range bag。 -
[P1]
packages/shader-parser/src/parser/AST.ts:588-620,1710-1778— declarator 的 const/initializer/array 事实仍由多条 reduction 分别维护且漂移。 逗号声明仍丢isConst、不校验 initializer,并直接修改共享typeInfo.arraySpecifier,使后续 declarator 继承前一个 array shape;global path 仍漏NonConstInitializer。现状仍会让const int A=1,B=2; const int C=B;误报,让const float A=1.0,B=u;/const float G=u;漏报,并把float a,b[2],c;的c记成 array。请以FullySpecifiedType + 每个 declarator 自己的 initializer/arraySpecifier为唯一 owner,统一 normalize/validate 后建VarSymbol,删除三份平行逻辑和共享SymbolTypemutation。 -
[P1]
packages/shader-analyzer/src/ShaderValidator.ts:608-674/packages/shader-parser/src/parser/TypeSystem.ts:93-113— operator legality/result 仍维护两套 type system。 validator 的_arithmeticFamily/_areArithmeticShapesCompatible接收 operator 并判断 legality,AST inference 却调用不接收 operator 的arithmeticResultType(a,b);合法mat2x3 * mat3x2被降为TypeAny,非法同型 non-square matrix multiply 又先被推成具体原类型,直到另一个 package 才给相反结论。请让TypeSystem单一返回 per-operator compatibility/result(含 invalid reason),parser inference 与 validator 共用,并删除 validator 的平行 family/shape 规则。
非阻塞项
- [P2]
packages/shader-parser/src/parser/AST.ts:425-455,1723,1789-1812— 同一个 type-reference validation 被 5 个父级 reduction 手工触发,并已产生重复诊断。TypeSpecifier本身已经拥有 type token 与 branch,但 SingleDeclaration、FunctionHeader、ParameterDeclarator、StructDeclaration、VariableDeclaration 各自调用 helper;globalData a,b,c;又为后续 identifier 复用同一个FullySpecifiedType构造 syntheticVariableDeclaration,每次都会对同一 type token 再报一次错误。请让TypeSpecifier.semanticAnalyze在该 type occurrence 上校验一次,删除 5 个父级调用点;下游 declaration 只消费已解析结果。 - [P2]
packages/shader-analyzer/src/ShaderAnalyzer.ts:25-30的公开 TSDoc 仍称 AST 只在下一次 analyze 前有效,但返回前已深拷贝 program;请让契约与实际 ownership 一致。 - [P2]
tests/src/shader-compiler/DiagnosticDriverConsistency.test.ts:873-907仍给 compiler 注入 analyzer,error case 在 WebGL oracle 前返回,不能证明无 analyzer codegen 与真实 driver 一致;请拆成 analyzer diagnostic、无 analyzer raw codegen、driver 三条独立断言。 - [P2] PR 元数据与注释规范仍未收口。 该 PR 新增两个公开 package、Analyzer API 与 Engine 配置,title 仍应从
refactor(shader)改为feat(shader);新增单行//仍有句尾句号,TypeSystem.matrixDimensions也缺同族 public API 的完整多行 TSDoc、@param与@returns。
- add backend-neutral IR and core info consumed by GLES backends - move diagnostics, reachability, IO checks, and macro proofs into analyzer ownership - preserve runtime codegen with branch, include, type, and shader-library regressions covered
- retain upstream animation, clone, particle, and package changes - keep verbose parser ownership and remove the obsolete compiler verbose subpackage
- reject missing shader entries before backend generation and reuse neutral entry facts - compact the default parser artifact without compressing or mangling runtime control flow - align analyzer/codegen/driver consistency tests with structural compiler failures
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
Request changes(P1)。已基于 3aa34798137e2b882a5f08b9b940578a4af682fa...8fd6abc06fd3c7e2c39dc48a15ce1b11b24b7682 的 11 个增量 commit 审查实际 diff,并沿 ShaderLab source → preprocess/neutral IR → analyzer/core info → codegen/precompile 各追一层;当前 GitHub checks 全绿。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 8fd6abc06fd3c7e2c39dc48a15ce1b11b24b7682。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
- include cache 已改为以 canonical include path 展开和缓存,且有双 root 顺序回归(
Preprocessor.ts:139-143、ReviewRegression.test.ts)。 - canonical self-guard 不再覆盖外层 feature 约束;struct/function/variable 的 branch coverage 统一由 branch engine 消费,并补了 outer feature 回归(
BranchAwareLookup.test.ts:432-550)。 - 跨调用
_sourceErrors、codegen 二次 IO 扫描与 missing-entry 空 stage 路径已删除;ShaderCoreInfo产出 backend facts,ShaderAnalysisInfo产出 analyzer-only call graph/IO facts,8fd6abc在 backend 前拒绝缺失 entry。 - 节点可达性、entry call graph 和 IO write facts 已统一为
isBranchReachable+ShaderAnalysisInfo,死分支/未调用 helper 不再伪造 IO 事实。 - overload call graph、AST lifetime、bare return、bool const、strict comparison bound、cross-stage fallback、browser entry 和 GLES300 的
undefined输出均已关闭。 - operator legality/result 已收敛到
TypeSystem.arithmeticOperation,parser inference、普通算术和 compound assignment 共用;逗号 declarator 的 const 传播与 array shape 泄漏也已有回归保护。 - driver oracle 现在独立调用 compiler,不再向 compiler 注入 analyzer;source mapping 的 include diagnostics 也已按 source map 回写。
问题
-
[P1]
packages/shader-parser/src/parser/AST.ts:316-371,580-660,1697-1737/packages/shader-analyzer/src/ShaderValidator.ts:333-351— 全局const初始化绕过了已经抽出的 declarator 契约,非法源码不会得到NonConstInitializer。VariableDeclaration已建立含isConst/initializer 的VariableDeclaratorInfo,但只设置isStatic;ShaderValidator._checkVariableDeclarator也只检查void和 assignability。相反,局部SingleDeclaration和InitDeclaratorList仍各自在 parser 中调用ParserUtils.isConstExpr。所以float runtimeValue; const float bad = runtimeValue;在全局既不会被 analyzer 报错,又会被 compiler 作为静态声明输出,最终交给 driver 拒绝。请保留VariableDeclaratorInfo为每个 declarator 的唯一事实、ShaderValidator为唯一诊断 owner:把 const-expression 检查迁到 validator,删除两个 parser-local_validateInitializer分支,并补 global scalar/array、local/逗号 declarator 的同一矩阵回归;不要为了旧局部测试保留平行 parser validation。 -
[P1]
packages/shader-compiler/src/ShaderCompiler.ts:28-35,112-145— source parser 的结构性错误仍被 logging-only 路径吞掉,precompile 可把已丢弃的 RenderState 当成成功产物发布。ShaderSourceParser.parseWithErrors已是 source structure 和 errors 的权威 owner,但_parseShaderSource只逐条Logger.error后丢弃errors,_precompile随即继续序列化shaderSource。例如InvalidRenderStateProperty在 source parser 中明确不写入constantMap/variableMap,这条路径会得到默认 RenderState 的 precompiled shader,而不是失败;MissingEntry恰好被下游 entry lookup 拦住,不能代表全部 source error 已封口。请让_parseShaderSource传递同一个 typed parse result,_precompile在 source error 时直接失败,analyzer 仅机械消费同一 envelope 做诊断;删除 compiler 的 logging-only source-error side path,并补 invalid RenderState 和 duplicate entry 的 precompile rejection 测试,不能把失效 fixture 反过来变成 runtime compatibility fallback。 -
[P2] 公共契约和 PR 元数据仍未收口。 PR title 仍是
refactor(shader): add neutral IR and standalone diagnostics,但实际新增并发布 parser/analyzer public package、standalone diagnostics 与 CLI,应改为feat(shader): ...。同时AnalyzerOptions/AnalysisResult等新增 public surface 仍使用单行 TSDoc(如ShaderAnalyzer.ts:20-34),偏离仓库的多行 public TSDoc + 参数/返回说明规范;请随公开契约一次性修正,而不要继续把 API 说明分散到实现注释。
架构、熵增与测试治理
本轮 neutral IR 划分本身是净减熵:backend 只消费 ShaderCoreInfo,analyzer-only 的 call graph/reachability/IO facts 留在 ShaderAnalysisInfo,替代了旧的 parser/codegen 双重 IO 判断。仍有两处未完成收口:declarator 的事实已缩为一份,但 const 校验仍由两个 parser reduction 和零个 global validator 分担;source parse errors 已缩为 parseWithErrors 一份,却在 compiler 被再降级为日志副作用。应分别保留 VariableDeclaratorInfo → ShaderValidator 与 ShaderSourceParser.parseWithErrors 两条权威链路,删除局部 parser validation 和 compiler logging-only 分支。现有测试覆盖局部/逗号 const、source-map 及 missing entry,但缺全局 const 和 RenderState/duplicate-entry precompile 失败,无法守住替换后的公开契约。
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/shader-parser/src/common/PreprocessorCondition.ts (1)
120-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
| 0silently rewrites out-of-range and fractional literals.
scanNumberaccepts any finite number and then truncates it with| 0. Two cases change meaning without a diagnostic:
#if X == 4294967296becomesX == 0.#if X == 1.5becomesX == 1.The documented contract at Line 31 states that the function throws when an expression cannot be represented by this reasoning model. Truncation contradicts that contract, and the resulting value then drives branch coverage and runtime instruction encoding. Reject the literal instead of reshaping it.
🐛 Proposed fix
const parsed = Number(value); - if (!Number.isFinite(parsed)) throwMalformedPreprocessorCondition(source); + // `#if` arithmetic is signed-integer only, so reject anything this layer would have to reshape. + if (!Number.isInteger(parsed) || parsed < -2147483648 || parsed > 2147483647) { + throwMalformedPreprocessorCondition(source); + } context.index += value.length; - return parsed | 0; + return parsed; }If the preprocessor must tolerate such literals instead of rejecting them, keep the truncation and record the wrap-around behavior in the function comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-parser/src/common/PreprocessorCondition.ts` around lines 120 - 130, Update scanNumber so literals that cannot be represented exactly by its integer reasoning model are rejected rather than coerced: after parsing, validate that the value is an integer within the supported signed 32-bit range, and call throwMalformedPreprocessorCondition for fractional or out-of-range values. Remove the | 0 conversion while preserving normal integer parsing and index advancement.packages/shader-parser/src/sourceParser/SourceLexer.ts (1)
154-159: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop recovery at
}and EOF.Line 158 always advances after the scan, even when no semicolon exists. If an invalid render-state property is followed by
}, recovery consumes the structural braces, advances past EOF, and the next property parse dereferences an undefined token. Return a recovery status at}or EOF, and end the enclosing property or state parse without consuming the delimiter. This preserves the typed diagnostic instead of throwing aTypeError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-parser/src/sourceParser/SourceLexer.ts` around lines 154 - 159, Update SourceLexer.scanToCharacter to stop recovery at either `}` or EOF and return a status indicating whether the target character was found, without advancing past those delimiters. Adjust the enclosing render-state/property parsing flow to honor this status, end parsing safely, and leave `}` or EOF unconsumed so subsequent token access cannot dereference undefined values.
🧹 Nitpick comments (22)
rollup.config.js (1)
27-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the verbose shader-parser push when the package is missing.
pkgs.findreturnsundefinedwhen@galacean/engine-shader-parseris not inpkgs. Spreadingundefinedproduces{ verboseMode: true }, soconfig()then readspkgJson.nameonundefinedand the whole build fails with an opaqueTypeError.The same file already guards optional packages:
if (shaderPkg)at Line 251 andif (analyzerPkg)at Line 256. Apply the same guard here.♻️ Proposed refactor
const shaderParserPkg = pkgs.find((item) => item.pkgJson.name === "`@galacean/engine-shader-parser`"); -pkgs.push({ ...shaderParserPkg, verboseMode: true }); +if (shaderParserPkg) { + pkgs.push({ ...shaderParserPkg, verboseMode: true }); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rollup.config.js` around lines 27 - 29, Guard the `pkgs.push` call for `shaderParserPkg` with a presence check, matching the existing `shaderPkg` and `analyzerPkg` guards, so the package is only pushed with `verboseMode: true` when `@galacean/engine-shader-parser` is found.packages/shader-analyzer/src/ShaderAnalysisInfo.ts (1)
75-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a re-iterable
Iterablefromfunctions().
functions()capturesthis._functionsByName.values()once, before returning. AMapvalues iterator is single-use. The returned object therefore yields the functions on the firstfor...ofand yields nothing on every later iteration of the same value.The only current caller,
ShaderValidator._reportMutualRecursion, iterates once, so behavior is correct today. The declared return typeIterable<ASTNode.FunctionDefinition>promises repeatable iteration, so a second consumer would silently observe an empty sequence.Make the method a generator so each iteration starts a fresh map iterator.
♻️ Proposed refactor
/** * Returns every parsed function declaration. * `@returns` Function identities retained by the neutral IR. */ - functions(): Iterable<ASTNode.FunctionDefinition> { - const groups = this._functionsByName.values(); - return { - *[Symbol.iterator]() { - for (const functions of groups) yield* functions; - } - }; - } + *functions(): Generator<ASTNode.FunctionDefinition> { + for (const functions of this._functionsByName.values()) yield* functions; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/ShaderAnalysisInfo.ts` around lines 75 - 82, Update ShaderAnalysisInfo.functions() to be a generator that obtains a fresh this._functionsByName.values() iterator on each invocation/iteration, rather than capturing one iterator in the returned object. Preserve the existing behavior of yielding every function definition across all name groups while ensuring the returned Iterable can be iterated repeatedly.packages/shader-parser/src/parser/SemanticAnalyzer.ts (1)
124-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a local redefinition conflict type for
reportRedefinition.
SymbolTableStack.insertalready declaresExclude<DeclarationCoexistence, "exclusive"> | "none", soSemanticAnalyzer.reportRedefinitionshould use a local type alias instead of exposing theDeclarationCoexistenceunion in its signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-parser/src/parser/SemanticAnalyzer.ts` around lines 124 - 128, Define a local type alias for the redefinition conflict values near reportRedefinition, matching Exclude<DeclarationCoexistence, "exclusive"> | "none", and update SemanticAnalyzer.reportRedefinition to accept that alias instead of exposing the DeclarationCoexistence expression directly. Keep the behavior and accepted values unchanged.tests/src/shader-analyzer/ReviewRegression.test.ts (1)
394-395: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude the diagnostics in the failure message.
Line 395 asserts an empty array without a message. If a diagnostic appears, the failure output shows only a length mismatch. Lines 49 and 66 in this file already pass
JSON.stringify(diagnostics)as the assertion message. Apply the same pattern here so a regression names the offending diagnostic.♻️ Proposed change
const result = new ShaderAnalyzer().analyze(source); - expect(result.diagnostics).to.be.empty; + expect(result.diagnostics, JSON.stringify(result.diagnostics)).to.be.empty;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-analyzer/ReviewRegression.test.ts` around lines 394 - 395, Update the diagnostics assertion in ReviewRegression.test.ts to pass JSON.stringify(result.diagnostics) as its failure message, matching the existing assertions near lines 49 and 66 so any unexpected diagnostic is shown.packages/shader-analyzer/src/ShaderAnalyzer.ts (2)
61-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the
skipSemanticValidationpredicate into a named local.The tenth argument is a multi-line
diagnostics.some(...)expression nested inside the call. The reader must decode it before the call itself becomes readable._analyzePassalso takes ten positional parameters, so the argument list is easy to misorder in later edits.Bind the predicate to a named constant, and consider grouping the invariant parameters into a single context object.
♻️ Proposed refactor of the call site
const statements = shaderSource.pendingContents.concat(subShader.pendingContents, pass.pendingContents); + const hasPreprocessorErrorInStatements = diagnostics.some( + (diagnostic) => + diagnostic.code === DiagnosticType.PreprocessorError && + statements.some( + (statement) => + diagnostic.range.start.offset >= statement.range.start.index && + diagnostic.range.start.offset <= statement.range.end.index + ) + ); this._analyzePass( pass, statements, source, diagnostics, includeMap, chunkOutputCache, options?.basePathForIncludeKey, options?.file, - diagnostics.some( - (diagnostic) => - diagnostic.code === DiagnosticType.PreprocessorError && - statements.some( - (statement) => - diagnostic.range.start.offset >= statement.range.start.index && - diagnostic.range.start.offset <= statement.range.end.index - ) - ) + hasPreprocessorErrorInStatements );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/ShaderAnalyzer.ts` around lines 61 - 79, Extract the final diagnostics.some predicate passed to _analyzePass into a clearly named local constant, such as skipSemanticValidation, before the call, then pass that constant as the tenth argument. Preserve the predicate’s existing diagnostic and statement range conditions; leave broader parameter grouping unchanged unless needed for this refactor.
165-166: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDo not overwrite
diagnostic.filewith an undefined segment file.Line 166 assigns
startSegment.fileunconditionally. If a segment that originates from the pass itself carries nofile, this erases the file thatgseErrorToDiagnosticalready copied fromGSError.file. Line 146 and line 87 later restore a value with??=, so the result is usually the same, but the intermediate state loses parser-provided attribution.Assign only when the segment supplies a file.
♻️ Proposed change
diagnostic.relatedSource = startSegment.source; - diagnostic.file = startSegment.file; + if (startSegment.file !== undefined) diagnostic.file = startSegment.file;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/ShaderAnalyzer.ts` around lines 165 - 166, The assignment of diagnostic.file at line 166 unconditionally overwrites the file value that was previously set by gseErrorToDiagnostic from GSError.file. When startSegment.file is undefined, this erases the parser-provided attribution. Update the assignment to only set diagnostic.file when startSegment.file is defined, preserving the original value from gseErrorToDiagnostic in cases where the segment carries no file.packages/shader-analyzer/src/PreprocessorExpressionValidator.ts (2)
225-227: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThrow an
Errorsubclass instead of a bare object.
_failthrows an object literal. Thecatchinparsethen casts anything it catches toParseFailure. If an unexpectedTypeErroris raised inside_parseConditionalortokenize,parsereturns it as aParseFailure, and line 57 dereferencesfailure.token, which isundefinedon that value. A dedicated error class letsparsere-throw values it does not own.♻️ Proposed change
+class ParseFailureError extends Error { + constructor(readonly failure: ParseFailure) { + super(failure.message); + } +} + class ExpressionParser {parse(): ParseFailure | undefined { try { this._parseConditional(); const token = this._current(); if (token.kind !== "end") { const certain = token.kind !== "identifier" || token.text === "defined"; this._fail(`Unexpected token '${token.text}' in preprocessor expression.`, token, certain); } - } catch (failure) { - return failure as ParseFailure; + } catch (error) { + if (error instanceof ParseFailureError) return error.failure; + throw error; } }private _fail(message: string, token: Token, certain: boolean): never { - throw { message, token, certain } satisfies ParseFailure; + throw new ParseFailureError({ message, token, certain }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/PreprocessorExpressionValidator.ts` around lines 225 - 227, Update _fail to throw a dedicated Error subclass carrying the ParseFailure fields, and adjust parse to handle only that subclass as a validation failure while re-throwing unexpected errors from _parseConditional or tokenize. Preserve the existing failure message, token, and certain values for owned parse failures.
278-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one
positionAthelper across the package.
positionAtis duplicated verbatim inpackages/shader-analyzer/src/ShaderAnalyzer.tsat lines 229-240. Both copies convert a character offset to a 1-based line and column and both count only\n. Move the function into a shared module and import it in both files. A single copy keeps the line and column convention identical if the newline handling changes later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/PreprocessorExpressionValidator.ts` around lines 278 - 290, Move the duplicated positionAt helper into a shared package module, then import and use that single helper from both PreprocessorExpressionValidator.ts and ShaderAnalyzer.ts. Preserve its current 1-based line/column behavior and newline handling while removing both local duplicate definitions.packages/shader-analyzer/package.json (1)
16-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
typescondition to the start of the.exports entry.With
node16ornodenextmodule resolution, TypeScript preservespackage.jsonexports order and matches the first enabled condition. A runtime condition beforetypescan select./dist/module.jsas the declaration target instead of./types/index.d.ts.♻️ Proposed reordering
"exports": { ".": { + "types": "./types/index.d.ts", "import": "./dist/module.js", - "require": "./dist/main.js", - "types": "./types/index.d.ts" + "require": "./dist/main.js" }, "./package.json": "./package.json" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/package.json` around lines 16 - 23, Reorder the conditions in the root "." export so the "types" entry precedes "import" and "require", while preserving all existing targets and the "./package.json" export unchanged.packages/shader-analyzer/src/ShaderIOValidator.ts (1)
198-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing a shared zero position constant.
_entryNotFoundbuilds a new object literal cast toShaderPositionon every missing entry. A module-level frozen fallback keeps the diagnostic location shape in one place and avoids the inline cast.♻️ Proposed refactor
export class ShaderIOValidator { private static readonly _lookup = new SymbolInfo("", null); + private static readonly _originPosition = <ShaderPosition>{ index: 0, line: 0, column: 0 };- location ?? <ShaderPosition>{ index: 0, line: 0, column: 0 }, + location ?? this._originPosition,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-analyzer/src/ShaderIOValidator.ts` around lines 198 - 211, Update ShaderIOValidator._entryNotFound to use a shared module-level frozen zero-position ShaderPosition constant instead of constructing and casting an inline fallback object. Keep the existing location value when provided and preserve the current default index, line, and column values.tests/src/shader-analyzer/BranchAwareLookup.test.ts (3)
450-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert an exact diagnostic count.
expect(errors.length).to.be.greaterThan(0)passes on any number ofUseBeforeDeclarationerrors. The shader referencesdata.value,guardedValue, andguardedHelper(), so the expected count is knowable. An unrelated regression that adds or removes one error would not fail this test.💚 Proposed change
- expect(errors.length).to.be.greaterThan(0); + expect(errors).to.have.lengthOf(3);Confirm the actual count before pinning it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-analyzer/BranchAwareLookup.test.ts` around lines 450 - 453, Update the assertion in the BranchAwareLookup test to require the exact number of UseBeforeDeclaration diagnostics produced by references to data.value, guardedValue, and guardedHelper(). First confirm the current diagnostic count, then replace the greaterThan(0) check with an exact-count assertion while preserving the existing filtering criteria.
357-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared
analyzerinstance.This test constructs
new ShaderAnalyzer()at Line 372 while every other test in the file uses the module-levelanalyzerand theerrorsOfhelper. If the fresh instance is deliberate, state why in a comment. If it is not, useerrorsOffor consistency.♻️ Proposed change
- const result = new ShaderAnalyzer().analyze(src); - expect(result.diagnostics.filter((diagnostic) => diagnostic.code === "UseBeforeDeclaration")).to.have.lengthOf(1); + expect(errorsOf(src, "UseBeforeDeclaration")).to.have.lengthOf(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-analyzer/BranchAwareLookup.test.ts` around lines 357 - 374, Update the test around the module-level analyzer to use the shared analyzer instance and the existing errorsOf helper when asserting the UseBeforeDeclaration diagnostic; only retain a fresh ShaderAnalyzer construction if it is deliberate and document the reason inline.
337-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord that this case has no real coverage gap.
MODE <= 0,MODE == 1, andMODE >= 2together cover every integer, sobranchValueis always declared. The test asserts one warning, which means the solver returns"unknown"rather than"covered". The interval solver can refute the counterexample here, butcanCandidateSetCoverCallsitedoes not prove the positive direction across three candidates, so coverage stays unresolved.The current classification is the safe one. Add a comment so a future improvement that turns this warning into
"covered"reads as an intended change and not as a regression.♻️ Proposed comment
+ // These three ranges are exhaustive over the integers, so no real gap exists. The solver only + // refutes the counterexample and does not prove positive coverage across three candidates, so + // it degrades to `unknown` and warns. Update this expectation if coverage proving improves. it("does not report an integer-only coverage gap as an error", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-analyzer/BranchAwareLookup.test.ts` around lines 337 - 355, Add an explanatory comment to the test case in “does not report an integer-only coverage gap as an error,” documenting that MODE <= 0, MODE == 1, and MODE >= 2 cover all integers, while the solver currently leaves coverage unknown and therefore intentionally emits one warning. Keep the existing assertions unchanged so future classification as covered is recognized as an intended improvement.packages/shader-parser/src/common/BaseToken.ts (1)
354-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the greedy counterexample search as incomplete.
The loop picks the first negation that stays satisfiable and never backtracks. When a different negation choice would exclude a later candidate, the function returns
falseandgetBranchCoveragedegrades to"unknown". That direction is safe, because the analyzer then warns instead of failing the build. Add one sentence to the function so a future reader does not treatfalseas proof that no counterexample exists.♻️ Proposed comment
+/** + * Search for one macro configuration that reaches the callsite and excludes every candidate. + * + * The search is greedy and never backtracks over the choice of negated condition, so `false` + * means "no counterexample was found", not "no counterexample exists". Callers therefore map + * `false` to `unknown` coverage rather than to proven coverage. + */ function hasAtomicCoverageCounterexample(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-parser/src/common/BaseToken.ts` around lines 354 - 368, Add a single sentence to the function containing the greedy candidate loop, documenting that its first-satisfiable-negation search does not backtrack and therefore a false result is not proof that no counterexample exists. Do not change the loop or related coverage behavior.packages/shader-parser/src/lalr/LALR1.ts (1)
66-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard
symbolByOffsetagainstundefinedinstead of truthiness.
Tokenderives from theKeywordenum, andKeyword.CONST = 0. A grammar item such astype_qualifier → CONST . SEMICOLONwould makeitem.symbolByOffset(1)returnKeyword.CONST, whilenextSymbolis falsy, so_extendStateItemskips adding theSEMICOLONlookahead. UsenextSymbol !== undefinedso zero-valued terminals still participate in the lookahead scan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-parser/src/lalr/LALR1.ts` around lines 66 - 71, In the for loop that calls item.symbolByOffset, change the loop condition from checking nextSymbol for truthiness to explicitly checking nextSymbol !== undefined. This ensures that zero-valued terminals like Keyword.CONST (which equals 0) are not skipped due to being falsy, allowing the lookahead scan to correctly process all symbols including those at position 0 in the grammar.tests/src/shader-compiler/StateIsolation.test.ts (2)
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the comment: a missing entry now throws.
_generatethrowsVertex entry function 'vert' not found.for this fixture._parseShaderPasscatches the error, callsLogger.error, and returnsundefined. The path is no longer a soft return, and thefinallyblock is what restoresprocessingPassText. Update the comment so it describes the throw-and-catch path this test actually exercises.♻️ Proposed comment fix
-// Missing entries take the soft-return path; compiling it must not leak visitor state. +// Missing entries make `_generate` throw; `_parseShaderPass` catches, logs, and returns +// undefined. That path must not leak visitor state. const broken = `struct Attributes { vec3 POSITION; }; void notAnEntry() {}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-compiler/StateIsolation.test.ts` around lines 24 - 25, Update the comment above the broken shader fixture to describe that compiling a shader without the required vertex entry causes _generate to throw, _parseShaderPass to catch and log the error, and the finally block to restore processingPassText without leaking visitor state.
41-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the failed compile reports the error.
The test proves the degraded compile returns
undefined, but not that the failure is reported.Logger.erroris a noop until logging is enabled, so a silent regression in the catch block would pass. Spy onLogger.errorand assert one call, asStandaloneAnalyzer.test.tsdoes at Line 63.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-compiler/StateIsolation.test.ts` around lines 41 - 49, Update the degraded-compile test around ShaderCompiler and compile so it spies on Logger.error before compiling broken, then asserts it was called exactly once while preserving the existing undefined-result and subsequent valid-compile assertions; follow the established spy pattern in StandaloneAnalyzer.test.ts.packages/shader-compiler/src/ShaderCompiler.ts (1)
87-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the pass source an explicit parameter of
generate.
generatereadsShaderCompilerUtils.processingPassText, which_parseShaderPasssets and clears in itsfinallyblock. Any caller that invokesgenerateoutside an active_parseShaderPasscall receives""as the IR source, soShaderClueIR.sourcebecomes empty and error locations lose their text. The public method also silently depends on hidden global state.Add a
sourceparameter, or keep the method internal and document the required call order.♻️ Proposed signature change
generate( program: ASTNode.GLShaderProgram, vertexEntry: string, fragmentEntry: string, - backend: ShaderLanguage + backend: ShaderLanguage, + passSource = ShaderCompilerUtils.processingPassText ?? "" ): IShaderProgramSource { - const ir = new ShaderClueIR(program, ShaderCompilerUtils.processingPassText ?? ""); + const ir = new ShaderClueIR(program, passSource); const coreInfo = ShaderCoreInfo.create(ir, vertexEntry, fragmentEntry); return this._generate(ir, coreInfo, backend); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-compiler/src/ShaderCompiler.ts` around lines 87 - 96, Update ShaderCompiler.generate to accept an explicit source parameter and pass it directly to ShaderClueIR instead of reading ShaderCompilerUtils.processingPassText. Update every generate caller, including the _parseShaderPass flow, to provide the pass source while preserving existing backend and entry-point behavior.tests/src/shader-compiler/MacroBranchRuntime.test.ts (1)
200-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo tests assert the same diagnostic set but describe different causes.
Both tests expect exactly
["UseBeforeDeclaration"]. Neither asserts a diagnostic that identifies the non-complementary gap or the repeated condition. If the analyzer is expected to report only the use-before-declaration effect, state that in the test names. If a dedicated diagnostic is planned, add the assertion once it exists.Also applies to: 217-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-compiler/MacroBranchRuntime.test.ts` around lines 200 - 215, Clarify the names of the tests around the non-complementary `#ifndef/`#elif gap and repeated condition so they explicitly describe the only expected UseBeforeDeclaration diagnostic. Keep the exact diagnostic assertions and successful code-generation checks unchanged unless a dedicated gap or repeated-condition diagnostic is implemented, in which case assert it once rather than duplicating expectations.tests/src/shader-compiler/PreprocessorConditionConformance.test.ts (1)
300-301: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertion locks in a fast-parser limitation.
Line 301 requires
parsePreprocessorConditionto throw for every expression in this table, including((A == B || A == C)), which is plain parenthesized logic. If the fast parser later supports nested parentheses, this test fails even though behavior improved. Assert the end-to-end result only, or move the "unsupported by the fast parser" expectation into a separate table that is easy to update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/shader-compiler/PreprocessorConditionConformance.test.ts` around lines 300 - 301, Update the test around parsePreprocessorCondition so it no longer requires the fast parser to throw for every expression in the conformance table, especially valid nested-parentheses logic such as ((A == B || A == C)). Assert the expected end-to-end codegen/WebGL result, and isolate any intentionally unsupported fast-parser cases in a separate maintainable table.packages/shader-compiler/src/codeGen/GLESVisitor.ts (1)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the comment that names the removed
visitShaderProgrammethod.
visitShaderProgramno longer exists;generatereplaced it. The comment on Line 108 and the comment on Line 116 both still point readers to that method. NamegenerateandShaderCoreInfoinstead.♻️ Proposed comment fix
- // MRT structs were collected in visitShaderProgram; here only mark the fragment return statements + // MRT structs were collected in `ShaderCoreInfo`; here only mark the fragment return statementsApply the same correction outside this range at Line 116:
// Both stage struct-var maps are already populated in `generate`; just // pre-walk macro refs so struct codegen sees the references.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-compiler/src/codeGen/GLESVisitor.ts` at line 108, Update the comments near the fragment return handling to reference the current generate method instead of the removed visitShaderProgram method, and mention ShaderCoreInfo where appropriate. Apply the same correction to the nearby comment at line 116, preserving the existing description of struct-map population and macro-reference pre-walking.packages/shader-parser/src/parser/PassParser.ts (1)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the summary line with the returned type.
Line 13 states the function parses a pass "into an AST". The function returns
ShaderClueIR, and the@returnstag on Line 18 already says "Neutral IR". Update the summary so the doc for this new public export is consistent.♻️ Proposed doc fix
-/** - * Parses one shader pass into an AST and parse-stage diagnostics. +/** + * Parses one shader pass into neutral IR and parse-stage diagnostics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shader-parser/src/parser/PassParser.ts` around lines 12 - 19, Update the summary line of the parser function’s doc comment to say it parses the shader pass into neutral IR rather than an AST, matching the returned ShaderClueIR type and existing `@returns` description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/shader/ShaderMacroProcessor.ts`:
- Around line 342-368: Update ShaderMacroProcessor._evalRawCondition to catch
exceptions thrown during PreprocessorExpressionEvaluator evaluation, including
failures from malformed raw expressions or expansion, and return false so the
raw condition is treated as unsatisfied. Keep successful evaluations unchanged.
In `@packages/shader-analyzer/src/cli.ts`:
- Around line 82-93: Update readIncludeMap and its visit traversal to read only
files with supported shader-chunk extensions, skipping unrelated assets before
readFileSync and includeMap insertion. Preserve the existing .git and
node_modules directory exclusions, and explicitly retain the current behavior of
ignoring symbolic-link entries unless the intended behavior requires otherwise.
- Around line 1-5: The cli.ts source file requires a shebang to run as an
executable binary, but it cannot be added to the TypeScript source code since
`#!/usr/bin/env node` is not valid syntax. Update the build process or build
configuration to prepend the shebang line to the compiled dist/cli.js output
file after TypeScript compilation completes. Ensure the shebang appears as the
first line of the final emitted JavaScript file, before any existing import
statements.
In `@packages/shader-analyzer/src/PreprocessorExpressionValidator.ts`:
- Around line 132-143: Update PreprocessorExpressionValidator.parse() to track
whether _parseConditional() encountered an expandable identifier, and mark the
trailing-token failure as uncertain when that flag is set, including leftover
'(' from function-like macro invocations. Preserve the existing certainty
behavior for other unexpected tokens, and add a test covering a function-like
macro used inside `#if`.
In `@packages/shader-analyzer/src/ShaderAnalyzer.ts`:
- Around line 50-53: Move the validatePreprocessorExpressions and
ShaderCompilerUtils.clearAllShaderCompilerObjectPool calls inside analyze’s
existing try block so exceptions from either pre-parse step are converted into
diagnostics by the established catch paths. Preserve their current order and
behavior.
In `@packages/shader-compiler/package.json`:
- Around line 34-39: The `./verbose` export is pointing to the same dist/main.js
and dist/module.js files as the non-verbose export, which means it does not
actually provide a verbose build since the Rollup configuration builds those
artifacts with `_VERBOSE: false`. Update the `./verbose` export to reference
separate verbose-specific build artifacts (such as dist/main.verbose.js and
dist/module.verbose.js), and reorder the export map properties so that `types`
appears first, before `import` and `require`. Ensure the Rollup configuration
includes a second output that builds these verbose artifacts with the verbose
diagnostics flag enabled.
In `@packages/shader-compiler/rollup.config.js`:
- Line 71: Update the node-resolve configuration in the Rollup setup to include
“module” and “main” after “debug” in mainFields, preserving debug as the
preferred entry while restoring fallback resolution for dependencies without a
debug field. Leave exportConditions unchanged.
In `@packages/shader-parser/package.json`:
- Around line 14-37: The debug export conditions reference src files
(src/runtime.ts in the root export and src/index.ts in the ./verbose export) but
the files array does not include src/**/*, preventing npm from publishing these
debug entry points for consumers. Either add src/**/* to the files array to
publish source files, or update both debug export paths to point to published
dist artifacts (following the same pattern as the import and require conditions)
to ensure the exports remain resolvable after publication.
In `@packages/shader-parser/src/ir/ShaderCoreInfo.ts`:
- Around line 83-87: Update the struct-role derivation flow around
removeRoleConflicts and deriveStructVariableRoles so struct types removed as
conflicts are also excluded from structRoles and the
vertexStructVarMap/fragmentStructVarMap results. Pass or otherwise reuse the
conflict information when deriving roles, ensuring
CodeGenVisitor.visitPostfixExpression cannot select a role for a conflicted
struct while preserving role derivation for non-conflicting structs.
In `@packages/shader-parser/src/lexer/Lexer.ts`:
- Around line 246-261: Update _tokenizeForCodegen’s MACRO_ELIF, MACRO_ELSE, and
corresponding MACRO_ENDIF cleanup to propagate the existing constant-false
condition into every later arm once the branch stack records a true `#if` or
preceding `#elif`. Keep conditionalArm advancement intact and mirror the verbose
path’s static-dead handling so definitions in unreachable codegen arms remain
excluded.
In `@packages/shader-parser/src/parser/AST.ts`:
- Around line 1817-1821: In _collectIdentifierRefs, replace the early return
inside the references loop for macro-defined names with continue so only that
reference is skipped. Preserve processing of later non-macro references within
the same MacroCallSymbol or MacroCallFunction, matching the codegen path
behavior.
In `@packages/shader-parser/src/Preprocessor.ts`:
- Around line 139-143: Add an active-include tracking set to the recursive
expansion flow around _expand, checking each path before recursion and reporting
a preprocessing diagnostic when it already exists on the current include path.
Mark the key before calling _expand, and remove it after expansion completes so
only active recursion is guarded while normal cache reuse remains unchanged.
In `@tests/src/shader-analyzer/MacroBranchMatrix.test.ts`:
- Around line 23-41: Reset the shared ShaderCompiler/parser state at the start
of compile before invoking ShaderAnalyzer.analyze or
ShaderCompiler._parseShaderPass, including mutable state such as
ShaderCompilerUtils.processingPassText and any parsed-state singleton used by
these paths. Ensure each test case starts from a clean state without changing
the returned codes or fragment behavior.
In `@tests/src/shader-analyzer/PreprocessorExpressionDiagnostics.test.ts`:
- Around line 76-77: Update the diagnostic severity filters in
PreprocessorExpressionDiagnostics.test.ts and BuiltinShaderSmoke.test.ts to
compare against DiagnosticSeverity.Error rather than the string literal "error",
ensuring the assertions inspect actual error diagnostics.
In `@tests/src/shader-compiler/PrecompileABTest.test.ts`:
- Around line 321-330: Strengthen the test case around validatePrecompiledWebGL
for overlapping Particle render-mode macros by evaluating the generated vertex
instructions and asserting the expected priority winner’s output. Also verify
that instructions from the competing render-mode branches are absent, so a
priority reversal cannot pass merely because the shader remains valid.
In `@tests/src/shader-compiler/PreprocessorConditionConformance.test.ts`:
- Around line 292-295: The test expectations for the unsigned integer comparison
cases do not match the actual behavior of ShaderMacroProcessor, which evaluates
these expressions using signed semantics. Update the expected boolean values in
the test table: change the expectation for "-1 < 1u" from true to false, and for
"0xffffffffu > 0u" from false to true, to align with the signed comparison
behavior implemented in ShaderMacroProcessor's _compareValues and
evaluateBinaryExpression methods.
---
Outside diff comments:
In `@packages/shader-parser/src/common/PreprocessorCondition.ts`:
- Around line 120-130: Update scanNumber so literals that cannot be represented
exactly by its integer reasoning model are rejected rather than coerced: after
parsing, validate that the value is an integer within the supported signed
32-bit range, and call throwMalformedPreprocessorCondition for fractional or
out-of-range values. Remove the | 0 conversion while preserving normal integer
parsing and index advancement.
In `@packages/shader-parser/src/sourceParser/SourceLexer.ts`:
- Around line 154-159: Update SourceLexer.scanToCharacter to stop recovery at
either `}` or EOF and return a status indicating whether the target character
was found, without advancing past those delimiters. Adjust the enclosing
render-state/property parsing flow to honor this status, end parsing safely, and
leave `}` or EOF unconsumed so subsequent token access cannot dereference
undefined values.
---
Nitpick comments:
In `@packages/shader-analyzer/package.json`:
- Around line 16-23: Reorder the conditions in the root "." export so the
"types" entry precedes "import" and "require", while preserving all existing
targets and the "./package.json" export unchanged.
In `@packages/shader-analyzer/src/PreprocessorExpressionValidator.ts`:
- Around line 225-227: Update _fail to throw a dedicated Error subclass carrying
the ParseFailure fields, and adjust parse to handle only that subclass as a
validation failure while re-throwing unexpected errors from _parseConditional or
tokenize. Preserve the existing failure message, token, and certain values for
owned parse failures.
- Around line 278-290: Move the duplicated positionAt helper into a shared
package module, then import and use that single helper from both
PreprocessorExpressionValidator.ts and ShaderAnalyzer.ts. Preserve its current
1-based line/column behavior and newline handling while removing both local
duplicate definitions.
In `@packages/shader-analyzer/src/ShaderAnalysisInfo.ts`:
- Around line 75-82: Update ShaderAnalysisInfo.functions() to be a generator
that obtains a fresh this._functionsByName.values() iterator on each
invocation/iteration, rather than capturing one iterator in the returned object.
Preserve the existing behavior of yielding every function definition across all
name groups while ensuring the returned Iterable can be iterated repeatedly.
In `@packages/shader-analyzer/src/ShaderAnalyzer.ts`:
- Around line 61-79: Extract the final diagnostics.some predicate passed to
_analyzePass into a clearly named local constant, such as
skipSemanticValidation, before the call, then pass that constant as the tenth
argument. Preserve the predicate’s existing diagnostic and statement range
conditions; leave broader parameter grouping unchanged unless needed for this
refactor.
- Around line 165-166: The assignment of diagnostic.file at line 166
unconditionally overwrites the file value that was previously set by
gseErrorToDiagnostic from GSError.file. When startSegment.file is undefined,
this erases the parser-provided attribution. Update the assignment to only set
diagnostic.file when startSegment.file is defined, preserving the original value
from gseErrorToDiagnostic in cases where the segment carries no file.
In `@packages/shader-analyzer/src/ShaderIOValidator.ts`:
- Around line 198-211: Update ShaderIOValidator._entryNotFound to use a shared
module-level frozen zero-position ShaderPosition constant instead of
constructing and casting an inline fallback object. Keep the existing location
value when provided and preserve the current default index, line, and column
values.
In `@packages/shader-compiler/src/codeGen/GLESVisitor.ts`:
- Line 108: Update the comments near the fragment return handling to reference
the current generate method instead of the removed visitShaderProgram method,
and mention ShaderCoreInfo where appropriate. Apply the same correction to the
nearby comment at line 116, preserving the existing description of struct-map
population and macro-reference pre-walking.
In `@packages/shader-compiler/src/ShaderCompiler.ts`:
- Around line 87-96: Update ShaderCompiler.generate to accept an explicit source
parameter and pass it directly to ShaderClueIR instead of reading
ShaderCompilerUtils.processingPassText. Update every generate caller, including
the _parseShaderPass flow, to provide the pass source while preserving existing
backend and entry-point behavior.
In `@packages/shader-parser/src/common/BaseToken.ts`:
- Around line 354-368: Add a single sentence to the function containing the
greedy candidate loop, documenting that its first-satisfiable-negation search
does not backtrack and therefore a false result is not proof that no
counterexample exists. Do not change the loop or related coverage behavior.
In `@packages/shader-parser/src/lalr/LALR1.ts`:
- Around line 66-71: In the for loop that calls item.symbolByOffset, change the
loop condition from checking nextSymbol for truthiness to explicitly checking
nextSymbol !== undefined. This ensures that zero-valued terminals like
Keyword.CONST (which equals 0) are not skipped due to being falsy, allowing the
lookahead scan to correctly process all symbols including those at position 0 in
the grammar.
In `@packages/shader-parser/src/parser/PassParser.ts`:
- Around line 12-19: Update the summary line of the parser function’s doc
comment to say it parses the shader pass into neutral IR rather than an AST,
matching the returned ShaderClueIR type and existing `@returns` description.
In `@packages/shader-parser/src/parser/SemanticAnalyzer.ts`:
- Around line 124-128: Define a local type alias for the redefinition conflict
values near reportRedefinition, matching Exclude<DeclarationCoexistence,
"exclusive"> | "none", and update SemanticAnalyzer.reportRedefinition to accept
that alias instead of exposing the DeclarationCoexistence expression directly.
Keep the behavior and accepted values unchanged.
In `@rollup.config.js`:
- Around line 27-29: Guard the `pkgs.push` call for `shaderParserPkg` with a
presence check, matching the existing `shaderPkg` and `analyzerPkg` guards, so
the package is only pushed with `verboseMode: true` when
`@galacean/engine-shader-parser` is found.
In `@tests/src/shader-analyzer/BranchAwareLookup.test.ts`:
- Around line 450-453: Update the assertion in the BranchAwareLookup test to
require the exact number of UseBeforeDeclaration diagnostics produced by
references to data.value, guardedValue, and guardedHelper(). First confirm the
current diagnostic count, then replace the greaterThan(0) check with an
exact-count assertion while preserving the existing filtering criteria.
- Around line 357-374: Update the test around the module-level analyzer to use
the shared analyzer instance and the existing errorsOf helper when asserting the
UseBeforeDeclaration diagnostic; only retain a fresh ShaderAnalyzer construction
if it is deliberate and document the reason inline.
- Around line 337-355: Add an explanatory comment to the test case in “does not
report an integer-only coverage gap as an error,” documenting that MODE <= 0,
MODE == 1, and MODE >= 2 cover all integers, while the solver currently leaves
coverage unknown and therefore intentionally emits one warning. Keep the
existing assertions unchanged so future classification as covered is recognized
as an intended improvement.
In `@tests/src/shader-analyzer/ReviewRegression.test.ts`:
- Around line 394-395: Update the diagnostics assertion in
ReviewRegression.test.ts to pass JSON.stringify(result.diagnostics) as its
failure message, matching the existing assertions near lines 49 and 66 so any
unexpected diagnostic is shown.
In `@tests/src/shader-compiler/MacroBranchRuntime.test.ts`:
- Around line 200-215: Clarify the names of the tests around the
non-complementary `#ifndef/`#elif gap and repeated condition so they explicitly
describe the only expected UseBeforeDeclaration diagnostic. Keep the exact
diagnostic assertions and successful code-generation checks unchanged unless a
dedicated gap or repeated-condition diagnostic is implemented, in which case
assert it once rather than duplicating expectations.
In `@tests/src/shader-compiler/PreprocessorConditionConformance.test.ts`:
- Around line 300-301: Update the test around parsePreprocessorCondition so it
no longer requires the fast parser to throw for every expression in the
conformance table, especially valid nested-parentheses logic such as ((A == B ||
A == C)). Assert the expected end-to-end codegen/WebGL result, and isolate any
intentionally unsupported fast-parser cases in a separate maintainable table.
In `@tests/src/shader-compiler/StateIsolation.test.ts`:
- Around line 24-25: Update the comment above the broken shader fixture to
describe that compiling a shader without the required vertex entry causes
_generate to throw, _parseShaderPass to catch and log the error, and the finally
block to restore processingPassText without leaking visitor state.
- Around line 41-49: Update the degraded-compile test around ShaderCompiler and
compile so it spies on Logger.error before compiling broken, then asserts it was
called exactly once while preserving the existing undefined-result and
subsequent valid-compile assertions; follow the established spy pattern in
StandaloneAnalyzer.test.ts.
🪄 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 Plus
Run ID: 29ee22de-acfe-4d0b-a307-84c80e858704
⛔ Files ignored due to path filters (22)
packages/shader/src/ShaderLibrary/Common/Fog.glslis excluded by!**/*.glslpackages/shader/src/ShaderLibrary/Lighting/AmbientOcclusion/BilateralBlur.glslis excluded by!**/*.glslpackages/shader/src/ShaderLibrary/Lighting/AmbientOcclusion/ScalableAmbientOcclusion.glslis excluded by!**/*.glslpackages/shader/src/ShaderLibrary/Particle/ParticleVert.glslis excluded by!**/*.glslpackages/shader/src/Shaders/Effect/Particle.shaderis excluded by!**/*.shaderpnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/src/shader-compiler/shaders/define-comment-with-dot.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/define-elif-polarity.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/define-in-comment-repro.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/define-line-continuation-member-access.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/define-line-continuation-repro.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/define-mixed-form-repro.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/define-multiline-params.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/digit-ending-id-repro.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-author-error-trailing-comma.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-author-error-unbalanced-paren.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-member-access-builtin-arg.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-token-fragment-trailing-comma.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-token-fragment-unbalanced-bracket.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-token-fragment-unbalanced-paren.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-value-refs-with-comments.shaderis excluded by!**/*.shadertests/src/shader-compiler/shaders/macro-value-refs.shaderis excluded by!**/*.shader
📒 Files selected for processing (83)
examples/package.jsonexamples/src/shader-playground.tspackages/core/src/Engine.tspackages/core/src/shader/ShaderMacroProcessor.tspackages/design/src/shader-compiler/ICondition.tspackages/design/src/shader-compiler/index.tspackages/shader-analyzer/package.jsonpackages/shader-analyzer/src/Diagnostic.tspackages/shader-analyzer/src/DiagnosticCategory.tspackages/shader-analyzer/src/DiagnosticType.tspackages/shader-analyzer/src/PreprocessorExpressionValidator.tspackages/shader-analyzer/src/ShaderAnalysisInfo.tspackages/shader-analyzer/src/ShaderAnalyzer.tspackages/shader-analyzer/src/ShaderIOValidator.tspackages/shader-analyzer/src/ShaderValidator.tspackages/shader-analyzer/src/cli.tspackages/shader-analyzer/src/convert.tspackages/shader-analyzer/src/index.tspackages/shader-compiler/package.jsonpackages/shader-compiler/rollup.config.jspackages/shader-compiler/src/ShaderBackend.tspackages/shader-compiler/src/ShaderCompiler.tspackages/shader-compiler/src/ShaderInstructionEncoder.tspackages/shader-compiler/src/codeGen/CodeGenVisitor.tspackages/shader-compiler/src/codeGen/GLES300.tspackages/shader-compiler/src/codeGen/GLESVisitor.tspackages/shader-compiler/src/codeGen/VisitorContext.tspackages/shader-parser/package.jsonpackages/shader-parser/src/GSError.tspackages/shader-parser/src/ParserUtils.tspackages/shader-parser/src/Preprocessor.tspackages/shader-parser/src/ShaderCompilerUtils.tspackages/shader-parser/src/common/BaseLexer.tspackages/shader-parser/src/common/BaseToken.tspackages/shader-parser/src/common/PreprocessorCondition.tspackages/shader-parser/src/common/ShaderPosition.tspackages/shader-parser/src/common/SymbolTable.tspackages/shader-parser/src/common/SymbolTableStack.tspackages/shader-parser/src/index.tspackages/shader-parser/src/ir/ShaderClueIR.tspackages/shader-parser/src/ir/ShaderCoreInfo.tspackages/shader-parser/src/ir/index.tspackages/shader-parser/src/lalr/CFG.tspackages/shader-parser/src/lalr/LALR1.tspackages/shader-parser/src/lalr/StateItem.tspackages/shader-parser/src/lalr/Utils.tspackages/shader-parser/src/lexer/Lexer.tspackages/shader-parser/src/parser/AST.tspackages/shader-parser/src/parser/PassParser.tspackages/shader-parser/src/parser/SemanticAnalyzer.tspackages/shader-parser/src/parser/ShaderInfo.tspackages/shader-parser/src/parser/ShaderTargetParser.tspackages/shader-parser/src/parser/TargetParser.ypackages/shader-parser/src/parser/TypeSystem.tspackages/shader-parser/src/runtime.tspackages/shader-parser/src/sourceParser/ShaderSourceParser.tspackages/shader-parser/src/sourceParser/SourceLexer.tspackages/shader-parser/src/sourceParser/index.tspackages/shader-parser/verbose/package.jsonrollup.config.jstests/package.jsontests/src/shader-analyzer/BranchAwareLookup.test.tstests/src/shader-analyzer/BranchDeclarationConflict.test.tstests/src/shader-analyzer/BranchResolutionAmbiguity.test.tstests/src/shader-analyzer/BuiltinShaderSmoke.test.tstests/src/shader-analyzer/DiagnosticCoverage.test.tstests/src/shader-analyzer/MacroBranchMatrix.test.tstests/src/shader-analyzer/PreprocessorExpressionDiagnostics.test.tstests/src/shader-analyzer/ReviewRegression.test.tstests/src/shader-analyzer/ShaderAnalyzer.test.tstests/src/shader-analyzer/ShaderIOAnalyzer.test.tstests/src/shader-analyzer/ShaderPlayground.test.tstests/src/shader-compiler/DiagnosticDriverConsistency.test.tstests/src/shader-compiler/MacroBranchRuntime.test.tstests/src/shader-compiler/Precompile.test.tstests/src/shader-compiler/PrecompileABTest.test.tstests/src/shader-compiler/PreprocessorConditionConformance.test.tstests/src/shader-compiler/ReturnStatementInvariant.test.tstests/src/shader-compiler/ShaderCompiler.test.tstests/src/shader-compiler/ShaderNeutralIR.test.tstests/src/shader-compiler/StandaloneAnalyzer.test.tstests/src/shader-compiler/StateIsolation.test.tstests/vitest.config.ts
💤 Files with no reviewable changes (1)
- packages/shader-parser/src/parser/ShaderInfo.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- examples/package.json
- packages/shader-parser/src/index.ts
- packages/shader-parser/src/sourceParser/index.ts
- tests/vitest.config.ts
- packages/shader-analyzer/src/Diagnostic.ts
- tests/src/shader-compiler/ReturnStatementInvariant.test.ts
- packages/shader-compiler/src/codeGen/GLES300.ts
- packages/shader-analyzer/src/DiagnosticCategory.ts
- tests/package.json
- packages/shader-parser/src/parser/ShaderTargetParser.ts
- tests/src/shader-analyzer/ShaderIOAnalyzer.test.ts
- packages/shader-analyzer/src/index.ts
- tests/src/shader-compiler/DiagnosticDriverConsistency.test.ts
- packages/shader-parser/src/ParserUtils.ts
- packages/shader-compiler/src/codeGen/VisitorContext.ts
- tests/src/shader-compiler/Precompile.test.ts
- tests/src/shader-analyzer/BranchResolutionAmbiguity.test.ts
- tests/src/shader-analyzer/ShaderAnalyzer.test.ts
| case "raw": | ||
| return ShaderMacroProcessor._evalRawCondition(cond.e, valueMacros, funcMacros); | ||
| } | ||
| } | ||
|
|
||
| private static _evalRawCondition( | ||
| expression: string, | ||
| valueMacros: Map<string, string>, | ||
| funcMacros: Map<string, FuncMacro> | ||
| ): boolean { | ||
| const withDefinedValues = expression.replace( | ||
| /\bdefined\s*(?:\(\s*([A-Za-z_]\w*)\s*\)|([A-Za-z_]\w*))/g, | ||
| (_match, parenthesized: string | undefined, bare: string | undefined) => { | ||
| const name = parenthesized ?? bare!; | ||
| return valueMacros.has(name) || funcMacros.has(name) ? "1" : "0"; | ||
| } | ||
| ); | ||
| const expandedNames = ShaderMacroProcessor._expandedNames; | ||
| expandedNames.clear(); | ||
| const expanded = ShaderMacroProcessor._recursiveExpandMacro( | ||
| withDefinedValues, | ||
| valueMacros, | ||
| funcMacros, | ||
| expandedNames | ||
| ); | ||
| return new PreprocessorExpressionEvaluator(expanded).evaluate() !== 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Contain evaluator exceptions inside _evalRawCondition.
PreprocessorExpressionEvaluator.evaluate() throws for invalid syntax, unterminated comments, unknown operators, and division by zero. _evalRawCondition propagates that throw through _evalCondition and out of ShaderMacroProcessor.evaluate, which runs on the runtime shader-variant path.
ShaderInstructionEncoder._parseCondition (packages/shader-compiler/src/ShaderInstructionEncoder.ts, lines 161-167) produces { t: "raw", e: expression } exactly when parsePreprocessorCondition already failed. The raw payload therefore carries the expressions most likely to be malformed. A shader containing #if 1 + or #if X / 0 now aborts runtime compilation instead of resolving the branch as unsatisfied.
Catch the failure at this boundary and return a defined result.
🛡️ Proposed fix to contain the throw
private static _evalRawCondition(
expression: string,
valueMacros: Map<string, string>,
funcMacros: Map<string, FuncMacro>
): boolean {
const withDefinedValues = expression.replace(
/\bdefined\s*(?:\(\s*([A-Za-z_]\w*)\s*\)|([A-Za-z_]\w*))/g,
(_match, parenthesized: string | undefined, bare: string | undefined) => {
const name = parenthesized ?? bare!;
return valueMacros.has(name) || funcMacros.has(name) ? "1" : "0";
}
);
const expandedNames = ShaderMacroProcessor._expandedNames;
expandedNames.clear();
const expanded = ShaderMacroProcessor._recursiveExpandMacro(
withDefinedValues,
valueMacros,
funcMacros,
expandedNames
);
- return new PreprocessorExpressionEvaluator(expanded).evaluate() !== 0;
+ try {
+ return new PreprocessorExpressionEvaluator(expanded).evaluate() !== 0;
+ } catch {
+ // A malformed or unevaluable `#if` expression resolves as unsatisfied so one bad
+ // directive cannot abort variant generation for the whole shader.
+ return false;
+ }
}📝 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.
| case "raw": | |
| return ShaderMacroProcessor._evalRawCondition(cond.e, valueMacros, funcMacros); | |
| } | |
| } | |
| private static _evalRawCondition( | |
| expression: string, | |
| valueMacros: Map<string, string>, | |
| funcMacros: Map<string, FuncMacro> | |
| ): boolean { | |
| const withDefinedValues = expression.replace( | |
| /\bdefined\s*(?:\(\s*([A-Za-z_]\w*)\s*\)|([A-Za-z_]\w*))/g, | |
| (_match, parenthesized: string | undefined, bare: string | undefined) => { | |
| const name = parenthesized ?? bare!; | |
| return valueMacros.has(name) || funcMacros.has(name) ? "1" : "0"; | |
| } | |
| ); | |
| const expandedNames = ShaderMacroProcessor._expandedNames; | |
| expandedNames.clear(); | |
| const expanded = ShaderMacroProcessor._recursiveExpandMacro( | |
| withDefinedValues, | |
| valueMacros, | |
| funcMacros, | |
| expandedNames | |
| ); | |
| return new PreprocessorExpressionEvaluator(expanded).evaluate() !== 0; | |
| } | |
| case "raw": | |
| return ShaderMacroProcessor._evalRawCondition(cond.e, valueMacros, funcMacros); | |
| } | |
| } | |
| private static _evalRawCondition( | |
| expression: string, | |
| valueMacros: Map<string, string>, | |
| funcMacros: Map<string, FuncMacro> | |
| ): boolean { | |
| const withDefinedValues = expression.replace( | |
| /\bdefined\s*(?:\(\s*([A-Za-z_]\w*)\s*\)|([A-Za-z_]\w*))/g, | |
| (_match, parenthesized: string | undefined, bare: string | undefined) => { | |
| const name = parenthesized ?? bare!; | |
| return valueMacros.has(name) || funcMacros.has(name) ? "1" : "0"; | |
| } | |
| ); | |
| const expandedNames = ShaderMacroProcessor._expandedNames; | |
| expandedNames.clear(); | |
| const expanded = ShaderMacroProcessor._recursiveExpandMacro( | |
| withDefinedValues, | |
| valueMacros, | |
| funcMacros, | |
| expandedNames | |
| ); | |
| try { | |
| return new PreprocessorExpressionEvaluator(expanded).evaluate() !== 0; | |
| } catch { | |
| // A malformed or unevaluable `#if` expression resolves as unsatisfied so one bad | |
| // directive cannot abort variant generation for the whole shader. | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/shader/ShaderMacroProcessor.ts` around lines 342 - 368,
Update ShaderMacroProcessor._evalRawCondition to catch exceptions thrown during
PreprocessorExpressionEvaluator evaluation, including failures from malformed
raw expressions or expansion, and return false so the raw condition is treated
as unsatisfied. Keep successful evaluations unchanged.
| import { readFileSync, readdirSync } from "node:fs"; | ||
| import { dirname, join, relative, resolve, sep } from "node:path"; | ||
| import type { IncludeMap } from "@galacean/engine-shader-parser/verbose"; | ||
| import { ShaderAnalyzer } from "./ShaderAnalyzer"; | ||
| import { DiagnosticSeverity, formatDiagnostic } from "./Diagnostic"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no shebang is present in the CLI source and inspect the analyzer build scripts.
fd -t f 'cli.ts' packages/shader-analyzer --exec head -n 3 {}
echo '--- package.json ---'
fd -t f 'package.json' packages/shader-analyzer -d 2 --exec jq '{name, bin, scripts, files}' {}
echo '--- existing shebang conventions in the repo ---'
rg -n --glob '!node_modules' '#!/usr/bin/env node' packages | head -20Repository: galacean/engine
Length of output: 754
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- packages/shader-analyzer files ---'
git ls-files 'packages/shader-analyzer/*' | sed -n '1,120p'
echo '--- package manifests mentioning shader-analyzer or bin entries ---'
rg -n --glob 'package.json' '"`@galacean/engine-shader-analyzer`"|galacean-shader-analyzer|./dist/cli\.js|bin"|compilerOptions|transform|rollup|esbuild|vite|tsx|node' packages -g 'package.json' | sed -n '1,200p'
echo '--- CLI related tracked files ---'
fd -t f 'cli' packages/shader-analyzer
fd -t f 'rollup.config|tsconfig|vitest|vite' packages/shader-analyzer packages -g 'rollup.config.*' -g 'tsconfig*.json' -g '*.{mjs,ts,js}' | sed -n '1,120p'Repository: galacean/engine
Length of output: 2084
🌐 Web query:
Node.js CLI executable shebang POSIX npm bin file #!/usr/bin/env node npx
💡 Result:
To create a Node.js CLI tool that runs correctly across different environments, you must combine the correct package.json configuration with a specific shebang line in your entry file [1][2][3]. The Shebang Line Every executable Node.js file must begin with the shebang line #!/usr/bin/env node [1][4][5]. This line tells POSIX-compliant operating systems (like Linux and macOS) to use the env command to locate the node executable in the user's PATH [6][7]. This is superior to hard-coding a path (e.g., #!/usr/local/bin/node) because the node installation location varies across systems [4][2][8]. How npm Handles CLI Executables When you distribute your CLI package via npm, the bin field in your package.json tells npm which files should be installed as executables [1][3]. 1. POSIX Systems: npm creates a symbolic link from your specified file to a global bin directory (or a local.bin folder for project dependencies), allowing the script to be run by the name defined in the bin object [1][3]. 2. Windows: Because Windows does not natively support shebang lines, npm automatically generates a.cmd shim file alongside your executable [9][10][3]. This shim re-invokes node against your script [3]. Critically, npm reads your file's shebang line to decide how to construct this shim [9][3]. Failing to include the shebang can cause installation issues or failure to execute on Windows [1][10]. Package.json Configuration The bin field maps your command name to the path of the entry file [1][2]: { "bin": { "my-cli-command": "./bin/cli.js" } } If you have only one executable and want it to share the package name, you can simplify it to a string: "bin": "./bin/cli.js" [1]. Important Considerations - Execution Permissions: On POSIX systems, you must ensure the file is marked as executable (e.g., chmod +x bin/cli.js), though npm handles this automatically during global installation [4][6][3]. - npx Usage: The npx tool uses the same bin configuration as npm [10]. When you run npx my-cli-command, npx downloads the package (if not present) and executes the linked binary defined in the bin field [3][11]. - Node.js Behavior: Node.js itself is designed to ignore the shebang line if the file is executed directly with the node command (e.g., node cli.js), as the # character starts a comment in many languages (though not natively in JS, the runtime handles it gracefully) [6][8]. Top Results: [1], [4], [10], [3]
Citations:
- 1: https://docs.npmjs.com/cli/v9/configuring-npm/package-json/
- 2: https://p.rst.im/q/github.com/lirantal/nodejs-cli-apps-best-practices
- 3: https://www.nazarboyko.com/articles/building-cli-tools-with-nodejs
- 4: https://nodejs.org/learn/command-line/run-nodejs-scripts-from-the-command-line
- 5: https://nodejs.org/learn/command-line/run-nodejs-scripts-from-the-command-line.html
- 6: https://stackoverflow.com/questions/33509816/what-exactly-does-usr-bin-env-node-do-at-the-beginning-of-node-files
- 7: Node.js shebang line and NODE_OPTIONS nodejs/help#109
- 8: https://eytanmanor.medium.com/how-clis-in-node-js-actually-work-c26f913a335e
- 9: https://stackoverflow.com/questions/10396305/npm-package-bin-script-for-windows
- 10: shebangs and cmd shimmer documentation npm/feedback#148
- 11: https://codably.dev/tools-tech/building-your-first-cli-tool
Add a shebang and build-time prep before publishing the CLI binary.
bin.galacean-shader-analyzer points to dist/cli.js, but cli.ts starts with an import statement and tsc does not add #!/usr/bin/env node. Running npx galacean-shader-analyzer will be attempted as a shell script without an interpreter, and the missing shebang also causes npm to skip the Windows .cmd shim.
Prepend the shebang to the emitted or bundled entry file. A #! line is not valid TypeScript input, so add it in the build step instead of in src/cli.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shader-analyzer/src/cli.ts` around lines 1 - 5, The cli.ts source
file requires a shebang to run as an executable binary, but it cannot be added
to the TypeScript source code since `#!/usr/bin/env node` is not valid syntax.
Update the build process or build configuration to prepend the shebang line to
the compiled dist/cli.js output file after TypeScript compilation completes.
Ensure the shebang appears as the first line of the final emitted JavaScript
file, before any existing import statements.
| function readIncludeMap(root: string): IncludeMap { | ||
| const includeMap: Record<string, string> = {}; | ||
| const visit = (directory: string): void => { | ||
| for (const entry of readdirSync(directory, { withFileTypes: true })) { | ||
| const path = join(directory, entry.name); | ||
| if (entry.isDirectory() && entry.name !== ".git" && entry.name !== "node_modules") visit(path); | ||
| else if (entry.isFile()) includeMap[toIncludeKey(relative(root, path))] = readFileSync(path, "utf8"); | ||
| } | ||
| }; | ||
| visit(root); | ||
| return includeMap; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Filter include files by extension before reading them.
visit reads every regular file under --include-root into memory as a UTF-8 string. It excludes only .git and node_modules. If an operator points --include-root at a project directory that also holds textures, models, or archives, the CLI decodes all of them into the include map. Include chunks are text shader fragments, so every non-shader file is wasted memory, and a large asset tree can exhaust the heap before any analysis begins.
Restrict the walk to shader chunk extensions.
♻️ Proposed fix
+const INCLUDE_EXTENSIONS = [".glsl", ".shader", ".frag", ".vert", ".chunk"];
+
function readIncludeMap(root: string): IncludeMap {
const includeMap: Record<string, string> = {};
const visit = (directory: string): void => {
for (const entry of readdirSync(directory, { withFileTypes: true })) {
const path = join(directory, entry.name);
if (entry.isDirectory() && entry.name !== ".git" && entry.name !== "node_modules") visit(path);
- else if (entry.isFile()) includeMap[toIncludeKey(relative(root, path))] = readFileSync(path, "utf8");
+ else if (entry.isFile() && INCLUDE_EXTENSIONS.some((extension) => entry.name.endsWith(extension))) {
+ includeMap[toIncludeKey(relative(root, path))] = readFileSync(path, "utf8");
+ }
}
};
visit(root);
return includeMap;
}Note also that readdirSync with withFileTypes does not follow symbolic links, so entry.isDirectory() and entry.isFile() are both false for a link. Include chunks reached through a symlink are skipped without a message. Confirm whether that matches the intended behavior.
📝 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.
| function readIncludeMap(root: string): IncludeMap { | |
| const includeMap: Record<string, string> = {}; | |
| const visit = (directory: string): void => { | |
| for (const entry of readdirSync(directory, { withFileTypes: true })) { | |
| const path = join(directory, entry.name); | |
| if (entry.isDirectory() && entry.name !== ".git" && entry.name !== "node_modules") visit(path); | |
| else if (entry.isFile()) includeMap[toIncludeKey(relative(root, path))] = readFileSync(path, "utf8"); | |
| } | |
| }; | |
| visit(root); | |
| return includeMap; | |
| } | |
| const INCLUDE_EXTENSIONS = [".glsl", ".shader", ".frag", ".vert", ".chunk"]; | |
| function readIncludeMap(root: string): IncludeMap { | |
| const includeMap: Record<string, string> = {}; | |
| const visit = (directory: string): void => { | |
| for (const entry of readdirSync(directory, { withFileTypes: true })) { | |
| const path = join(directory, entry.name); | |
| if (entry.isDirectory() && entry.name !== ".git" && entry.name !== "node_modules") visit(path); | |
| else if (entry.isFile() && INCLUDE_EXTENSIONS.some((extension) => entry.name.endsWith(extension))) { | |
| includeMap[toIncludeKey(relative(root, path))] = readFileSync(path, "utf8"); | |
| } | |
| } | |
| }; | |
| visit(root); | |
| return includeMap; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shader-analyzer/src/cli.ts` around lines 82 - 93, Update
readIncludeMap and its visit traversal to read only files with supported
shader-chunk extensions, skipping unrelated assets before readFileSync and
includeMap insertion. Preserve the existing .git and node_modules directory
exclusions, and explicitly retain the current behavior of ignoring symbolic-link
entries unless the intended behavior requires otherwise.
| ["(~MASK & 0xffu) != 0", [["MASK", "255"]], false], | ||
| ["0xffffffffu + 1u == 0u", [], true], | ||
| ["-1 < 1u", [], true], | ||
| ["0xffffffffu > 0u", [], false], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the macro processor evaluates numeric literals and comparisons.
fd -t f 'ShaderMacroProcessor.ts' packages/core/src --exec cat -nRepository: galacean/engine
Length of output: 29074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the conformance test around the referenced expectations and native-check handling.
fd -t f 'PreprocessorConditionConformance.test.ts' tests --exec sh -c '
echo "--- $1 ---"
sed -n "240,325p" "$1"
' sh {}
# Inspect all relevant files sharing the processor name for completeness.
fd -t f 'ShaderMacroProcessor.ts' . --exec sh -c 'echo "--- $1 ==="; wc -l "$1"' sh {}Repository: galacean/engine
Length of output: 3640
Fix the unsigned comparison test expectations.
ShaderMacroProcessor evaluates integer literals as signed values and uses signed comparisons in _compareValues and evaluateBinaryExpression. For -1 < 1u and 0xffffffffu > 0u, the table currently expects true and false, but native preprocessor semantics match the table labels: -1 < 1u is false and 0xffffffffu > 0u is true. Update these rows to false/true, or exclude them from native checking if signed semantics are intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/shader-compiler/PreprocessorConditionConformance.test.ts` around
lines 292 - 295, The test expectations for the unsigned integer comparison cases
do not match the actual behavior of ShaderMacroProcessor, which evaluates these
expressions using signed semantics. Update the expected boolean values in the
test table: change the expectation for "-1 < 1u" from true to false, and for
"0xffffffffu > 0u" from false to true, to align with the signed comparison
behavior implemented in ShaderMacroProcessor's _compareValues and
evaluateBinaryExpression methods.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
Request changes(P1)。已基于 8fd6abc...3aabb46 的 2 个增量 commit 审查实际 diff,并沿 parser package public boundary → compiler/analyzer consumers → npm resolver,以及 ShaderLab source parser → precompile 各追一层;当前 GitHub checks 全绿。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 3aabb46。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
- include cache 已按 canonical include path 展开、递归也以当前 include 的 canonical path 为 base,并有双 root 顺序回归。
- canonical self-guard 不再覆盖外层 feature 约束,struct/function/variable 统一由 branch engine 消费,并已覆盖 outer feature。
- analyzer-only 的 call graph、reachability 与 IO facts 已收敛到 ShaderAnalysisInfo;死分支和未调用 helper 不再伪造 stage IO。
- overload call graph、pooled AST lifetime、bare return、bool const、strict comparison bound、cross-stage fallback、browser entry 与 GLES300 的 undefined 输出均已修复。
- arithmetic legality/result 已收敛到 TypeSystem.arithmeticOperation;逗号 declarator 的 const 传播和 array shape 泄漏已有回归保护。
- driver oracle 已独立调用 compiler,include diagnostics 已通过 source map 回写。
- 本轮 EmptyStruct 已由真实的宏空 member 场景触发,替换此前不可达的 gap fixture。
问题
-
[P1] packages/shader-parser/src/parser/AST.ts:316-371,580-660,1709-1737 / packages/shader-analyzer/src/ShaderValidator.ts:333-351 — 全局 const 初始化仍绕过 declarator 契约,非法源码不会产生 NonConstInitializer。 VariableDeclaratorInfo 已携带 isConst 与 initializer,但 VariableDeclaration 只注册 symbol/isStatic;ShaderValidator 只校验 void 和 assignability。相反,局部 SingleDeclaration 与 InitDeclaratorList 仍各自在 parser 中调用 ParserUtils.isConstExpr。因此 float runtimeValue; const float bad = runtimeValue; 在全局不会得到诊断,却会被当成 static 声明继续 codegen,最后才交给 driver。请保留 VariableDeclaratorInfo 为每个 declarator 的唯一事实、ShaderValidator 为唯一诊断 owner:将 const-expression 检查迁到 validator,删除两个 parser-local initializer 校验分支,并补 global scalar/array、local/逗号 declarator 的同一矩阵回归。
-
[P1] packages/shader-compiler/src/ShaderCompiler.ts:34-40,122-168 — source parser 的结构性错误仍被 logging-only 路径吞掉,precompile 可将被丢弃的 RenderState 作为成功产物发布。 ShaderSourceParser.parseWithErrors 已是 shaderSource 与 errors 的权威 owner,但 _parseShaderSource 逐条 Logger.error 后仅返回 IShaderSource,_precompile 随即序列化它。InvalidRenderStateProperty 明确不会写入 render-state map,因而这条路径可发布默认 RenderState;仅靠下游 MissingEntry 阻断并不能封住其他 source errors。请传递同一个 typed parse envelope,_precompile 在 source errors 时直接失败,analyzer 机械消费它做诊断;删除 compiler logging-only side path,并覆盖 invalid RenderState 与 duplicate entry 的 precompile rejection。
-
[P1] packages/shader-parser/package.json:12-29 / internal/package.json:1-5 / internal/verbose/package.json:1-5 — 新的 internal boundary 仍保留根包的 legacy resolver 入口,公开契约随 resolver 分叉。 exports 已只声明 ./internal 与 ./internal/verbose,但根 package 仍把 main/module/debug/types 指向同一份 runtime artifact,而 src/runtime.ts 导出全部 parser internals。moduleResolution=node 和忽略 exports 的 legacy bundler 会继续接受裸包导入;遵循 exports 的 Node 则拒绝它,形成同一源码在工具链间一边可编译或加载、一边运行失败的第二协议。请保留 root exports 作为公开路径 owner,并保留 internal 目录 manifests 仅供其 legacy 子路径解析;删除根 main/module/debug/types fallback,使旧裸包 tests/fixtures 按新 internal 契约改写,补 pack 后 root import 拒绝而两个 internal path 可解析的边界回归。
-
[P2] 公共契约文档与 PR 元数据仍未收口。 PR title 仍为 refactor(shader): add neutral IR and standalone diagnostics,实际新增并发布 parser/analyzer 公共 package、standalone diagnostics 与 CLI,应改为 feat(shader): ...。此外新增/修改的公共 surface 仍有单行 TSDoc,例如 ShaderIncludeMap、AnalyzerOptions、AnalysisResult(ShaderAnalyzer.ts:19-35)、ShaderCompiler(ShaderCompiler.ts:17)和 version(index.ts:7-8)。请按仓库多行公共 TSDoc 规范连同 param/returns 一次性收口,不要把公开契约说明继续分散在实现注释中。
架构、熵增与测试治理
本轮 package boundary 的方向正确:parser internals 应只由 compiler/analyzer 通过 internal 子路径消费,AnalyzerOptions 已把外露的 include map 收敛为 ShaderIncludeMap。可是 exports 的单一公开 owner 又被根 main/module/debug/types fallback 复制为第二条 legacy 协议;删除根 fallback 后的数据流应为消费者 → exports → internal artifact,classic resolver 仅经 internal 目录 manifest 机械解析。另一条 source→precompile 链路仍把 parseWithErrors 的唯一 errors 事实降级为日志副作用,而 declarator 的唯一事实又被三处 parser 校验和 validator 分割;应分别收敛为 parse envelope → compiler/analyzer 与 VariableDeclaratorInfo → ShaderValidator。新 EmptyStruct 测试正确删除了不可达 gap 的思路;但仍缺 global const、source error rejection 和 packed package resolution 的新契约回归。不得为了任何旧裸包测试保留 root compatibility fallback。
- Centralize const diagnostics and reject structural source parse errors before precompile. - Strip analyzer-only paths from runtime artifacts and enforce parser package boundaries. - Cover macro, include, source mapping, package, and artifact regressions.
- Execute the active npm CLI through Node so pack verification works across platforms.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:无新增 P0/P1/P2。已基于 3aabb46591b2d94f3f4fab64ad5261e8683faf94...5c593e0b97a63b66f4aa32dd075358f2f3544008 的 2 个增量 commit 完成审查,并沿 ShaderLab source parser → compiler/precompile/Shader.create、declarator facts → validator,以及 parser package exports → runtime/analyzer consumers 各追一层;GitHub checks 全绿。实际 review 动作为 COMMENTED,目标 HEAD 为 5c593e0b97a63b66f4aa32dd075358f2f3544008。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
- const initializer 已由
VariableDeclaratorInfo → ShaderValidator单链路校验,parser-local 的重复校验已删除;补充了 global、local-array 和 comma declarator 回归(a12d346)。 - ShaderLab 结构性错误现在由 parse result 驱动 compiler/precompile 直接失败,invalid RenderState 与 duplicate entry 不会再发布局部 precompile 产物(
a12d346)。 - parser root 的 legacy resolver fallback 已移除,公开入口收敛为
./internal与./internal/verbose;package verifier 覆盖打包内容、root 拒绝和子路径解析,Windows 也已接入(a12d346、5c593e0)。 - canonical include cache、include cycle、canonical self-guard coverage、reachable IO/call graph、overload identity、AST lifetime、bare return、operator type owner 与 runtime artifact gate 均已按此前结论闭环,并保留对应回归。
- PR 标题已调整为
feat(shader),新增 public TSDoc 与 analyzer/runtime 边界说明也已收口。
架构、熵增与测试治理
本轮将 source-structure errors 的权威 owner 固定为 ShaderSourceParser.parseWithErrors:上游产生同一 parse result,下游 compiler 以 _requireValidShaderSource 机械阻断 Shader.create/_precompile,不再把 errors 降级为日志或以 partial render state 继续序列化。declarator 的 const/initializer 事实保持在 VariableDeclaratorInfo,validator 是唯一诊断 owner;parser runtime 仅保留 codegen 所需事实,verbose parser 供 analyzer 消费。package root exports 是唯一公开协议,internal manifests 仅服务其对应兼容解析。
这些改动删除了 source-error side path、parser-local const 校验、root resolver fallback 和 runtime analyzer surface,没有新增镜像状态或 compatibility branch。新增回归覆盖 source failure、macro/cycle、artifact/package boundary 和 Windows verifier;旧测试已改为消费 parse envelope 或新 internal 契约,未为旧测试保留生产 fallback。
- Remove _VERBOSE/jscc dual builds. - Add explicit runtime and analyzer package entries. - Keep proofs and diagnostics analyzer-only without runtime regressions. - Add package, artifact, CLI, macro, and regression gates.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:无新增 P0/P1;本轮仅发现 1 个 P2 注释规范问题。已基于 5c593e0b97a63b66f4aa32dd075358f2f3544008...f1504b0f90e5e9f9b8cead93b15df0b03da70bf1 的 1 个增量 commit 完成审查,并沿 parser 的 runtime/analyzer 入口 → compiler/analyzer 消费端 → 打包产物与 package resolver 各追一层;GitHub build、lint、E2E 和 coverage checks 均为成功。实际 review 动作为 COMMENTED,目标 HEAD 为 f1504b0f90e5e9f9b8cead93b15df0b03da70bf1。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
- const initializer 已固定为
VariableDeclaratorInfo → ShaderValidator的单一事实/诊断链,global、local array 和 comma declarator 回归已覆盖。 - ShaderLab 结构性 source errors 已由
ShaderSourceParser.parseWithErrors直接阻断 compiler/precompile,invalid RenderState 和 duplicate entry 不再发布局部产物。 - parser 根包 legacy resolver fallback 已移除,公开入口收敛为 internal 子路径;此前的 packed resolver、Windows 和 artifact 边界回归继续有效。
- canonical include cache、include-cycle/self-guard、branch reachability、IO/call graph、overload identity、AST lifetime、bare return、operator type owner 及 runtime artifact gate 均已按此前结论闭环。
- PR 标题和新增 public TSDoc 已按上一轮要求收口;本轮将旧
internal/verbose路径机械替换为internal/analyzer,未恢复旧兼容入口。
问题
- [P2]
tests/src/shader-compiler/ShaderCompiler.test.ts:361-362— 本提交改写的两行//注释仍以句号结尾,偏离仓库“单行注释不加句号”的规范。 请把这两行合并或去掉末尾句号;不影响行为,但应随本次注释改动一并收口。
架构、熵增与测试治理
本轮将 runtime 链路保持为 Lexer → ShaderTargetParser.create() → ShaderCompiler,而 analyzer 链路由 AnalyzerLexer → branchAnalysis/analyzerSemanticDiagnostics → parseShaderPass → ShaderAnalyzer/Validator 独占 branch proof 与诊断事实;compiler 的 cold build 仅对 runtime internal 子路径绑定 workspace source,发布时仍由 exports 指向各自 artifact。exports、internal manifests 与 package verifier 共同构成唯一公开协议,旧 verbose manifest/artifact 和 root fallback 已删除。
相较上一版,条件编译开关和 verbose 名称被两个显式、单向的 runtime/analyzer 入口替换,没有新增镜像状态、同步层或 production compatibility branch;迁移后的测试/fixture 直接消费 internal/analyzer,package verifier 同时检查旧 artifact 不再打包、新 export target 可打包与 runtime 不泄漏 analyzer source。除上述新注释外,未发现需要删除的同根因冗余或失效旧测试。
- Load analyzer CLI includes lazily and verify the built executable contract. - Correct the EmptyStruct playground preset and cover every labeled diagnostic sample. - Clean the remaining review nits without restoring legacy verbose paths.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:Request changes(P1)。已基于 f1504b0f90e5e9f9b8cead93b15df0b03da70bf1...55df995efb2d5241572d2de81260da50babbb6b9 的 1 个增量 commit、8 个文件完成审查,并沿 CLI 选项 → include-map/preprocessor → analyzer 诊断输出,以及 build script → Rollup CLI 产物 → npm bin 消费路径各追一层。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 55df995efb2d5241572d2de81260da50babbb6b9。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
VariableDeclaratorInfo → ShaderValidator、ShaderLab parse envelope、branch/reachability/IO、operator type owner、include cache/cycle 和 AST lifetime 等此前阻塞项均已按单一事实链路闭环,并保留回归覆盖。- parser 公开边界已收敛为 runtime/analyzer internal 子路径:legacy root resolver fallback、
verboseexport/artifact 与对应旧测试路径均已删除,runtime artifact gate 继续有效。 - 上轮的两行单行注释已合并为无句尾句号的当前行为说明;EmptyStruct playground fixture 也改为真实的宏空 member 场景,未恢复 legacy 路径。
- 本轮按需 include map 取代了 CLI 启动时递归读取整个 include root 的实现;Preprocessor 仍是 canonical include key、cycle detection 和 source-map 归属的唯一 owner。
问题
- [P1]
scripts/verify-shader-analyzer-cli.mjs:47-50/package.json:15— 新增的 rawdist/cli.js直启断言在 POSIX 上稳定阻断了整个 build。 Rollup 仅在rollup.config.js:177-185为该产物写入 shebang;本轮 verification 却在未经过打包安装的工作区直接spawnSync(cliPath, ["--help"])。目标 HEAD 的 CI 已实证 Ubuntu 与 macOS 都在:49得到status === null(null !== 0),导致两个 build 均失败;coverage 和全部 E2E 也因其先执行npm run build随之失败,Windows 只是跳过这个分支才通过。应保留packages/shader-analyzer/package.json的bin及 npm/pnpm 安装器为可执行命令、权限和 shim 的权威 owner,删除这个 raw-dist executable 协议:要验证发布 CLI,请在临时消费者中npm pack后安装并调用生成的node_modules/.bin/galacean-shader-analyzer;若只验证 Rollup 产物,则继续用process.execPath执行。不要在 build 后手工 chmoddist/cli.js来迎合该测试,那会额外维护一条与 packagebin平行的发布协议。
架构、熵增与测试治理
本轮生产数据流是 CLI 的 --include-root → lazy IncludeMap cache → Preprocessor._resolveIncludePath 的 canonical key → parseShaderPass/ShaderAnalyzer;lazy map 只缓存已请求的 source 或缺失结果,替代旧的全目录扫描,没有新增 mirror state、compat fallback 或第二套 include 转换。包含路径规范化、递归 base 和 cycle 检测仍由 Preprocessor 单一拥有;CLI 只提供文件系统 lookup,因此该方向是净减熵。
但新 build verification 在下游把 raw Rollup 文件的 POSIX mode 误当成另一份 CLI 可执行契约,与上游 package.json#bin + package-manager shim 的唯一发布 owner 平行,且已造成跨平台 CI 失败。删除 raw-dist 直启检查、改为 pack/install 消费者测试后,链路应为 rollup JS payload → packed package bin metadata → installer shim → user command,而不是增加手工 chmod 或 production compatibility branch。playground fixture/覆盖和 TypeSystem 的纯冗余删除没有引入额外状态;现有测试也无需为旧 eager include loader 保留任何生产 fallback。
- Set the generated analyzer CLI executable bit in the Rollup output hook. - Verify the artifact mode before direct POSIX execution.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:Request changes(P1)。已基于 55df995efb2d5241572d2de81260da50babbb6b9...2ecc31b293d4e2170d39c669530065d28fe1dace 的 1 个增量 commit、2 个文件完成审查,并沿 Rollup CLI 产物 → package.json#bin → npm POSIX bin-link 消费路径各追一层;GitHub 的 build、lint、E2E 与 coverage checks 均为成功。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 2ecc31b293d4e2170d39c669530065d28fe1dace。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
VariableDeclaratorInfo → ShaderValidator、ShaderLab parse envelope、branch/reachability/IO、operator type owner、include cache/cycle 与 AST lifetime 等此前阻塞项继续由各自的单一事实链路承载,并保留回归覆盖。- parser 的公开入口已收敛为 runtime/analyzer internal 子路径;legacy root resolver、
verboseartifact 与旧测试路径均已删除,runtime artifact gate 保持有效。 - 上轮新增的单行注释已收口为无句尾句号,EmptyStruct fixture 已替换为真实宏空 member 场景;lazy include map 继续由 Preprocessor 持有 canonical key、递归 base 与 cycle detection。
问题
- [P1]
rollup.config.js:190-193/scripts/verify-shader-analyzer-cli.mjs:47-50— 用 Rollupchmod和 raw-dist 直启测试修复 CI,仍把同一可执行权限维护成平行发布协议。packages/shader-analyzer/package.json#bin才是用户调用 CLI 的公开入口;npm 的 POSIXbin-links实现会在创建 symlink 后调用fixBin(absFrom),由安装器对 bin target 执行chmod。因此原始dist/cli.js的 mode 并不属于 Rollup payload 的公开契约,而本提交把它固定为0755并把这个内部 mode 反向设为 build gate。它既绕开了实际 pack/install 链路,也把权限 policy 从 package manager 复制到 bundler,未来两者的 umask、shim 或发布策略变化会再次漂移。请保留package.json#bin与安装器为唯一 owner,删除executable-analyzer-cli的writeBundle以及statSync/raw direct-spawn 断言;发布验证应在临时消费者中npm pack后安装并调用node_modules/.bin/galacean-shader-analyzer。现有process.execPath路径已足以验证未打包的 Rollup JS payload,无需为旧 raw-artifact 测试保留生产 chmod。
架构、熵增与测试治理
本轮上游 Rollup 只应负责生成带 shebang 的 JS payload,下游公开路径应为 packed package → package.json#bin → npm/pnpm installer symlink or shim → CLI command。当前改动额外引入 Rollup chmod → raw dist direct spawn:同一 executable fact 从一个权威 owner 变为 bundler 与安装器两套 policy,并将测试从用户可见命令转向内部文件。删除该插件与 raw-mode 测试后,权限与 shim 仍由安装器机械派生,概念/owner 数量回到一份;无需任何 compatibility branch。
其余本轮改动没有新增 parser/analyzer state、转换或 legacy path;不过测试应按替换后的公开 bin 契约重写为 pack/install consumer test,并删除 raw dist/cli.js executable fixture/assertion,不能让生产模式继续迁就该失效测试。
- Remove temporary parser package and analyzer CLI verification scripts. - Restore the root build to module and type compilation only.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:Request changes(P1)。已基于 2ecc31b293d4e2170d39c669530065d28fe1dace...234ee8cf8ebdafc2726b068a6913c2b3a5650ae4 的 1 个增量 commit、3 个文件完成审查,并沿 Rollup CLI payload → package.json#bin → pack/install 的 .bin consumer 与 parser manifest/exports → packed runtime/analyzer artifact → consumer resolver 各追一层;GitHub 的 build、lint、E2E 与 coverage checks 均成功。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 234ee8cf8ebdafc2726b068a6913c2b3a5650ae4。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
VariableDeclaratorInfo → ShaderValidator、ShaderLab parse envelope、branch/reachability/IO、operator type owner、include cache/cycle 和 AST lifetime 等先前阻塞项仍由各自单一事实链路承载,并保留回归覆盖。- parser 的公开入口继续收敛为 runtime/analyzer internal 子路径;legacy root resolver、
verboseartifact 与旧测试路径已删除,未恢复 compatibility fallback。 - 前轮的单行注释、EmptyStruct fixture 和 lazy include-map/canonical base 处理均保持收口;Preprocessor 仍拥有 canonical include key、递归 base 与 cycle detection。
问题
-
[P1]
package.json:15,26-29/ 删除的scripts/verify-shader-parser-package.mjs、scripts/verify-shader-analyzer-cli.mjs/rollup.config.js:177-195— 本提交删除了两条发布契约验证,却没有按实际包消费者路径替换;手工 chmod 的平行 owner 也仍保留。 现在 CI 只在 build、coverage 和 E2E 中调用npm run build,而它已只做b:module && b:types;不会执行npm pack、解析 packed parser 的 internal exports,或调用已安装的node_modules/.bin/galacean-shader-analyzer。前者原本覆盖了 parser 的files、root-export reject、internal/runtime-analyzer artifact boundary;后者虽错误地把 rawdist/cli.js的 mode/direct spawn 当公开协议,但仍是唯一会执行这 104 行 CLI 的测试(当前 Codecov 也显示cli.ts为 0%)。所以一次files、exports target、npm bin-link 或 CLI include-root 回归可在所有现有 check 成功时直接发布坏包。请保留
package.json#exports/#bin与 npm/pnpm installer 作为唯一发布、shim 和 executable-mode owner:删除 Rollup 的executable-analyzer-cli/writeBundle chmod,不要恢复 rawdist/cli.js的stat或直启断言。可保持根build只负责编译,但应在 build 之后独立运行一个 package-consumer contract test:将 parser/analyzernpm pack后安装到临时消费者,断言 parser root 仍拒绝、两个 internal subpath 解析到各自 artifact,runtime 不泄漏 analyzer surface,并通过安装生成的.bin/galacean-shader-analyzer验证--help与--include-root。这样权限与 shim 由安装器机械派生,发布契约只有一条真实消费链,已删除的 raw-artifact fixture 不必复活。
架构、熵增与测试治理
上游 Rollup 应只产出带 shebang 的 JS payload;下游唯一公开数据流应是 packed files → package.json exports/bin → npm/pnpm install/link → parser import 或 CLI command。上轮的 raw-dist direct-spawn test 本轮已删除,但 rollup.config.js 仍以 chmod 复制 installer 的 executable policy,而本提交又将 package-boundary/CLI contract test 从 1 条降为 0 条:生产 owner 仍为两份,公开路径测试则归零。按上面的删除与 pack/install consumer test 收口后,installer 是权限/shim 的唯一 owner,manifest 是公开路径的唯一 owner,测试只消费该路径,不需要 wrapper、镜像状态、compatibility branch 或 raw-artifact mode gate。
本轮未新增 parser/analyzer 事实、状态机、转换或 legacy path;但删掉验证脚本时混合删除了失效的 raw-mode 断言和仍有效的 published-package contract。应删除前者、按新公开契约重写后者,而不是让生产发布接口失去覆盖。
- Convert public one-based diagnostic positions before source excerpt formatting. - Cover token spans and rendered Playground marker alignment.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:Request changes(P1)。已基于 234ee8cf8ebdafc2726b068a6913c2b3a5650ae4...bbcc15154ca8948d9c22782da7311967164e2f89 的 1 个增量 commit、3 个文件完成审查,并沿 Diagnostic.range(公开一基坐标)→ formatDiagnosticSource(内部零基 marker)→ Playground 渲染 与 Rollup CLI payload → package.json#bin → pack/install 的 .bin consumer 各追一层;最新 GitHub build、lint、E2E 与 coverage checks 均成功。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 bbcc15154ca8948d9c22782da7311967164e2f89。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
VariableDeclaratorInfo → ShaderValidator、ShaderLab parse envelope、branch/reachability/IO、operator type owner、include cache/cycle 与 AST lifetime 等此前阻塞项继续由各自单一事实链路承载,并保留回归覆盖。- parser 公开入口仍收敛为 runtime/analyzer internal 子路径;legacy root resolver、
verboseartifact 与旧测试路径均已删除,未恢复 compatibility fallback。 - raw
dist/cli.jsdirect-spawn/mode 测试已删除;本轮进一步把公开Diagnostic.range的一基 line/column 机械转换为 source formatter 的零基坐标,token-span 和 Playground marker 回归均覆盖该契约。
问题
-
[P1]
package.json:15,26-29/ 已删除的scripts/verify-shader-parser-package.mjs、scripts/verify-shader-analyzer-cli.mjs/rollup.config.js:177-195— 发布契约验证仍为零,且 Rollup 仍手工复制安装器的可执行权限 owner。 根build只执行b:module && b:types,而 CI 的 build、coverage、E2E 都只调用它;当前没有步骤会npm pack、解析 packed parser 的 internal exports,或调用已安装的node_modules/.bin/galacean-shader-analyzer。因此files、exports target、npm bin-link、CLI--include-root任一回归都可在所有现有 checks 成功时发布坏包;同时writeBundle的chmod(0755)又与 npm/pnpm 在根据package.json#bin链接命令时派生的权限 policy 成为两份 owner。请保留
package.json#exports/#bin与 npm/pnpm installer 作为唯一发布、shim 和 executable-mode owner:删除executable-analyzer-cli的writeBundle chmod,不要恢复 rawdist/cli.js的 stat/direct-spawn 断言。在 build 后新增独立 package-consumer contract test,将 parser/analyzernpm pack后安装进临时消费者,断言 parser root 仍拒绝、两个 internal subpath 分别解析为 runtime/analyzer artifact、runtime 不泄漏 analyzer surface,并通过安装生成的.bin/galacean-shader-analyzer覆盖--help与--include-root。这样权限和 shim 由安装器机械派生,测试只消费唯一公开链路,不需 wrapper、镜像状态、compatibility branch 或 raw-artifact mode gate。
架构、熵增与测试治理
本轮坐标流由 positionAt/gseErrorToDiagnostic 产出一基公开位置,formatDiagnostic 是唯一将其转换为 formatter 私有零基协议的 adapter;下游 Playground 只渲染 formatter 输出。该改动没有新增状态、平行 range 表示或 compatibility path,新增测试直接验证 token 与用户可见 marker,属于净收口。
但发布流仍为 Rollup JS payload → packed files → package.json exports/bin → npm/pnpm install/link → parser import 或 CLI command。当前把 package-boundary/CLI contract tests 删除到零,同时保留 Rollup chmod → raw artifact 的平行权限路径:公开 manifest 与 installer 应分别是路径和权限的唯一 owner,Rollup 只负责带 shebang 的 payload。按上述删除和 pack/install consumer test 收口后,不会增加第三份真相,也不会为了旧测试保留任何 production compatibility branch。
- Package the standalone analyzer README with API and offline installation guidance. - Expand CLI help with supported inputs, options, examples, and exit codes.
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
阻塞级别:Request changes(P1)。已基于 bbcc15154ca8948d9c22782da7311967164e2f89...8d0f71e53ee324de80d4a6ade2dfd5fb1666ce46 的 1 个增量 commit、3 个文件完成审查,并沿 README/CLI help → packed files → package.json#bin/exports → npm/pnpm install 的消费者命令 以及 CLI --include-root → lazy IncludeMap → Preprocessor canonical include key → ShaderAnalyzer diagnostics 各追一层;GitHub build、lint、E2E 与 coverage checks 均成功。实际 review 动作为 REQUEST_CHANGES,目标 HEAD 为 8d0f71e53ee324de80d4a6ade2dfd5fb1666ce46。自动 CR 不替代人工 Reviewer 的合入门禁(APPROVE)。
已关闭问题清单
VariableDeclaratorInfo → ShaderValidator、ShaderLab parse envelope、branch/reachability/IO、operator type owner、include cache/cycle 与 AST lifetime 等此前阻塞项继续由各自单一事实链路承载,并保留回归覆盖。- parser 公开入口仍收敛为 runtime/analyzer internal 子路径;legacy root resolver、
verboseartifact 与旧测试路径均已删除,未恢复 compatibility fallback。 - raw
dist/cli.jsdirect-spawn/mode 测试已删除;公开Diagnostic.range的一基 line/column 仅由 formatter adapter 转为私有零基 marker,token-span 和 Playground marker 回归均覆盖该契约。
问题
-
[P1]
package.json:15,26-29/ 已删除的scripts/verify-shader-parser-package.mjs、scripts/verify-shader-analyzer-cli.mjs/rollup.config.js:177-195— README/--help现在把离线安装、.binCLI、--include-root和 exit code 明确成发布契约,但 CI 仍没有沿这个唯一消费者路径验证,Rollup 仍手工复制安装器的可执行权限 owner。 根build仍只执行b:module && b:types,当前 build、coverage、E2E 都不会pack、在临时消费者安装产物、解析 packed parser 的 internal exports 或调用node_modules/.bin/galacean-shader-analyzer。因此 README 宣传的 tarball 内容、files、exports target、npm bin-link、CLI help/include-root 任一回归都能在全部 checks 成功时发布坏包;同时writeBundle的chmod(0755)与 npm/pnpm 根据package.json#bin创建 shim、派生执行权限的 policy 仍是两份 owner。请保留
package.json#exports/#bin与 npm/pnpm installer 分别作为唯一公开路径、shim 和 executable-mode owner:删除executable-analyzer-cli的writeBundle chmod,不要恢复 rawdist/cli.js的stat/direct-spawn 断言。build 后新增独立的 package-consumer contract test:将 parser/analyzer 及其 runtime dependency tarballs 安装进临时消费者,断言 parser root 仍拒绝、两个 internal subpath 分别解析为 runtime/analyzer artifact、runtime 不泄漏 analyzer surface,并通过安装生成的.bin/galacean-shader-analyzer覆盖 README 所列--help、--include-root、stdin JSON 和 exit-code 路径。这样 README、manifest 与 CLI 共同被同一真实消费链验证,权限/shim 由安装器机械派生,不需 wrapper、镜像状态、compatibility branch 或 raw-artifact mode gate。
架构、熵增与测试治理
本轮的上游事实是 README/CLI 的公开命令与 exit-code 文案;下游应只有 packed files → package.json exports/bin → npm/pnpm install/link → parser import 或 CLI command 一条发布协议。--include-root 仍只将文件系统查找交给 lazy IncludeMap,Preprocessor 保留 canonical path、递归 base、cache 和 cycle detection 的唯一 owner,Analyzer 只消费其结果产生 diagnostics;help/README 没有增加状态、转换或 runtime compatibility path,方向正确。
但本轮把用户可见契约从隐含行为扩展为文档化承诺,而 package-boundary/CLI contract test 仍为零、Rollup chmod → raw artifact 的平行权限路径仍存在:公开路径和权限 owner 依旧分别多出未验证/重复的一份。按上述删除和 pack/install consumer test 收口后,manifest/installer 是唯一 owner,README 只是其受验证的说明,失效的 raw-artifact fixture 不应复活,也不应为旧测试保留任何生产 fallback。
设计依据:RFC RFC: Shader Static Analyzer #3017。核心参考 Naga 的
Frontend → IR → Validator/Info → Backend分层,让解析、生成和诊断独立消费中立事实;本 PR 只建立未来后端边界,不实现 WGSL。shader-parser负责 ShaderLab/source-pass 解析并拥有ShaderClueIR、ShaderCoreInfo等中立结构;shader-compiler只消费这些事实并通过ShaderBackend生成 GLES;shader-analyzer作为同级消费者产出结构化 diagnostics,并提供浏览器 API 与 headless CLI。parser 仅暴露
@galacean/engine-shader-parser/internal运行时入口和/internal/analyzer分析入口,根包拒绝解析;已删除_VERBOSE、jscc 双构建和 verbose 产物。冷构建通过显式 workspace runtime source 解析完成,不向 npm 包发布源码条件。source parser 统一拥有 source error、entry binding、原始 range/source mapping 与 include canonical path;compiler 消费 typed parse envelope,结构性错误会在 precompile 序列化前失败,不再发布被静默丢弃 RenderState 的部分产物。
VariableDeclaratorInfo是 const、initializer 与 array shape 的唯一事实;ShaderValidator统一产生NonConstInitializer等诊断,局部、全局、数组和逗号 declarator 使用同一规则。宏分支分析采用三态契约:可证明安全时不报,确定错误时报 error 并保留 witness,无法证明时只报 warning;analyzer 不枚举全部宏组合,也不改变或阻断 runtime codegen。复杂或未知关系不被伪装成确定错误。
include 支持 canonical URL、嵌套相对路径、循环检测、双向 root 顺序回归与精确文件定位。默认运行时产物不包含 analyzer proof solver、authoring diagnostic 文案、分析 Lexer 或报告方法。
基线固定为
dev/2.0@bd34daa45612af8b402cd3be916ff181f21ae742。本地完整 Chromium Vitest 为 141/141 个测试文件、445/445 个 suite、2149/2149 个用例;14 个包类型构建通过,22/22 个内置 Shader 在 bootstrap 与最终模块构建中均成功预编译。本次 head 的 GitHub CI 10/10 通过,覆盖三平台 build、四组 e2e、lint 与 codecov。当前 shipping Shader source 相对上一 PR head 无新增改动。对
dev/2.0执行 22 个内置 Shader × 25 组宏配置、1300 个 stage 对比:49 个差异全部来自已知的非法 Fog/SSAO/Particle 重叠宏 fallback,unexpected mismatch 为 0;所有受支持配置与基线一致。真实 WebGL 预编译 A/B 为 57/57。相同 consumer entry 下,当前运行时 bundle 为 345601 B / 63149 B gzip;
dev/2.0基线为 383670 B / 65870 B gzip,分别减少 9.92% / 4.13%。产物与 sourcemap 门禁确认 runtime 未包含BranchAnalysis、AnalyzerLexer、AnalyzerSemanticDiagnostics、PassParser或 analyzer-only diagnostic 文案。同机交替 A/B 基准每轮包含 200 次 warmup、15 个交替 batch、每 batch 200 次。完整 pipeline 三轮结果为 +1.266% / +0.0368 ms、+2.527% / +0.0727 ms、-0.502% / -0.0146 ms;没有任何一轮同时超过 +3% 和 +0.05 ms 的回归门槛。
parser、compiler、analyzer 的 npm dry-run、resolver 与 export-target 门禁通过;parser 包为 80 个条目且不发布
src。Standalone CLI 已验证 clean=0、warning=0、error=1;8 份 parser/compiler/analyzer sourcemap 均包含完整sourcesContent且不泄漏绝对路径。已知边界:branch signature 不是 proof-complete 的 ESSL 宏求解器;不能证明的复杂宏关系保持 warning,最终 GLES driver 仍是运行时可接受性的事实来源。