refactor(shader): split parser/compiler/analyzer + structured diagnostics (RFC #3017)#3054
Draft
zhuxudong wants to merge 135 commits into
Draft
refactor(shader): split parser/compiler/analyzer + structured diagnostics (RFC #3017)#3054zhuxudong wants to merge 135 commits into
zhuxudong wants to merge 135 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
The check only flagged direct sampler return; a comment claimed the struct case was handled but the code never did. Now looks the struct up in the shader symbol table and walks its members, recursively descending into nested structs and using a visited set to break typedef/macro cycles. Both leaf sampler and struct-containing-sampler return types now fire.
isConstExpr only recognised literals and bare const/macro identifiers. Extended to: - built-in function calls (constructors + sin/cos/pow/... via BuiltinFunction.isExist) whose args are themselves constant - compound expressions (binary/unary/ternary) where every sub-expression is constant Short-circuits on the first non-constant operand so `u_uniform + sin(0.5)` still reports (uniform mixed in) while `sin(0.5)` and `1.0 + 2.0` don't. Only invoked from the NonConstInitializer / NonConstArraySize diagnostics in the parser semantic pass — codegen never reads this predicate so it adds zero runtime cost to shader compilation.
GLSL ES §4.1.9: array size must be a positive integer. Real WebGL drivers reject `float a[0];` with "array size must be greater than zero" but we silently accepted it and only failed later at WebGL compile time. New DiagnosticType.InvalidArraySize fires when the literal-folded size evaluates to zero or a negative number. Non-literal / undefined sizes still fall through to the const-expression check.
Direct self-call was already covered. Added an SCC post-pass over the call graph built during the walk: every `FunctionCallGeneric` edge is recorded regardless of whether it's a self-call, and after the walk we iterate over the graph looking for cycles of length ≥ 2. Each cycle is reported once, at the lexicographically-first participant's declaration. The playground sample keeps only direct recursion because our grammar doesn't accept function forward declarations — mutual recursion is grammatically unreachable in ShaderLab today. The pass still fires when a future grammar change unlocks it, or when a mutually-recursive shape sneaks through via macros.
Direct dFdx / dFdy / fwidth inside the vertex entry was already flagged
via WalkContext.currentStage. A helper called from the vertex entry stays
`currentStage === null`, so `float helper(float x) { return dFdx(x); }`
was silently accepted even though the vertex path evaluates it.
Now record every derivative call site by enclosing function name during
the walk. After the walk, transitively reach from the vertex entry via
the call graph and report each derivative site inside a reachable helper.
Helpers on the fragment-only path stay silent.
Both diagnostics fired once per reference — a shader that touched gl_FragColor / gl_FragData three times reported the same conflict three times. The condition is per-shader (mixed use of legacy and MRT outputs), not per reference. Report GlFragColorWithMrt at the first gl_FragColor reference and stop; report GlFragData once per shader via a dedup flag on ShaderValidator (covers both the bare-identifier and gl_FragData[i] paths).
- gl_FragData[i] is a valid GLSL ES 1.00 fragment output and does not require MRT - remove the analyzer's guard (ShaderValidator._checkGlFragDataReference), DiagnosticType.GlFragData, its DIAGNOSTIC_CATEGORY entry, and the covering tests / playground sample - codegen keeps its own MRT normalization; nothing else observes the removed code
… warnings - undeclared identifiers (UseBeforeDeclaration) and unknown function names (UndefinedFunction) may resolve to runtime macros or conditional #include bodies that the precompile phase cannot see — reporting them as errors caused false positives for built-in shaders (e.g. RENDERER_JOINTS_NUM, texture2DLodOffset) - overload mismatch on a KNOWN function name (NoMatchingOverload) stays an error; the arg-type contract is only meaningful once the callee is resolved - NonConstArraySize now only fires when the size identifier resolves to a known non-const var — an undeclared identifier is already covered by the UseBeforeDeclaration warning, avoiding a duplicate/misleading error
…onsistency suite - for each curated GLSL-body case: run the DSL through ShaderAnalyzer, run the same body through ShaderCompiler._parseShaderPass, then feed the emitted GLSL to a real WebGL context and check the three views agree - severity contract: error → driver must reject; warning → driver behavior is not asserted (may be rescued by a runtime macro / conditional #include); no diagnostic → driver must accept - 10 curated cases covering clean baseline, five error-severity diagnostics (AssignTypeMismatch, ConstructorArgCount, InvalidReturnType, IndexOutOfBounds, NoMatchingOverload, NonBoolCondition, MisplacedControlFlow) and the two identifier-lookup warnings (UseBeforeDeclaration, UndefinedFunction) - also enforces "codegen never gates on severity" — every case, including error-severity ones, must still produce GLSL for editor / IDE consumption
…ic cases - add 26 cases spanning Type / Symbol / ControlFlow / PipelineIO categories: InvalidSwizzle, UndeclaredStructMember, ConstDivideByZero, ShiftOutOfRange, NonIntegerIndex, NonIndexableType, ExpectedSampler, InvalidUnaryOperand, InvalidBinaryOperands, ConstructorArgType, NonConstInitializer, NonConstArraySize, InvalidArraySize, NonFloatDerivativeArg, MissingReturn, Redefinition, RecursiveFunction, NonConstructibleReturnType, DerivativeInVertexShader, InvalidEntryReturnType, StructRoleConflict, GlFragColorWithMrt, NestedIOStruct, MissingVertexPosition, NonFlatIntegerVarying, EntryNotFound - relax the codegen assertion: `_parseShaderPass` may return undefined for parse-fail / pipeline-setup failures, but only if the analyzer produced an error — silent drops are still failures - mark "either" for cases the driver may still fold (constant division / shift overflow) or optimize away (unreferenced sampler-returning function) - RenderState-category and DSL-level entry diagnostics (DuplicateEntryAssignment, MissingEntry) are covered by DiagnosticCoverage.test.ts; they never reach the codegen path this suite drives
Two documentation-level defects from the ABC review:
- ShaderAnalyzer.test.ts:41 title said "error diagnostic" but the assertion
is `severity === "warning"`; rename to "warning diagnostic"
- DiagnosticDriverConsistency.test.ts had the two warning-severity cases
(UseBeforeDeclaration, UndefinedFunction) marked `driverExpects: "either"`
with a comment explicitly admitting "the driver rejects" — the suite was
documenting its own contract hole. Flip both to `"reject"`, keep `"either"`
reserved for spec-undefined cases (const div-by-zero, shift overflow), and
update the file header to make clear that warning severity encodes *intent*
("a runtime macro may rescue at bind time"), not driver acceptance
… use - `gl_FragData` is a `vec4[]` fragment-output array; the driver only accepts `gl_FragData[i]` (an indexed subscript). Bare use as an l-value, r-value, swizzle base, or function argument is invalid GLSL — writing `gl_FragData = vec4(0.0)` or `vec4 c = gl_FragData` compiles to nothing meaningful and the driver rejects it - collect every `gl_FragData` reference at parse time into `shaderData.glFragDataReferences` (mirroring `gl_FragColorReferences`); ShaderValidator's PostfixExpression check records the base of each legal `gl_FragData[i]` shape in `_indexedGlFragDataStarts` - after the walk, `_reportBareGlFragData` subtracts the indexed set from the collected refs and reports the first residue as `BareGlFragData` (once per shader; the driver only needs one signal to reject) - DiagnosticCoverage picks up the case; existing "gl_FragData[i] is legal" smoke test continues to pass (validates the strike-through path)
…codegen console
Two shader-diagnostics UX changes bundled by scope (both align the pipeline
with the analyzer-runtime decoupling stance):
- reword UseBeforeDeclaration and UndefinedFunction warnings from raw driver
language ("undeclared identifier" / "Undefined function:") to a directive
aimed at the shader author: "ensure it is provided at runtime (macro /
#include)". Rationale is in-line: the analyzer is decoupled from the engine
runtime by design and can't see macros that the material system feeds in at
bind time, so it hands the responsibility back to the author instead of
hard-failing at precompile
- drop `console.warn` from `GLESVisitor._softMissEntry`. The analyzer's
`EntryNotFound` diagnostic already routes through the engine Logger for the
user-facing signal; codegen is not the layer that surfaces UX, so precompile
of built-in shaders (PBR, Blinn, etc.) never spams the browser console. The
dedup Set and its `reset` call go with it — dead once the warn is gone
- `#include` directives are expanded by `Preprocessor.parse` before the AST is built, so the warning suggesting "provided at runtime (macro / #include)" was misleading — the only remaining "provided later" channel is a runtime macro from the material system - reword UseBeforeDeclaration and UndefinedFunction to "ensure it is provided at runtime as a macro" and tighten the surrounding rationale comment
…mbol - `VariableDeclaration.semanticAnalyze` (global scope path used by ShaderTarget- Parser) constructed VarSymbol without forwarding the `const` qualifier from `FullySpecifiedType.isConst`, so global `const float C = 1.0;` ended up with `VarSymbol.isConst = false` - effect: any downstream check that treats `isConst` as authoritative (e.g. the new InvalidAssignmentTarget rule against writing to const-qualified variables) silently missed globals; SingleDeclaration (local scope) was already correct, so the gap only affected top-level `const` - pass `type.isConst` as the fifth VarSymbol arg, mirroring the local path
GLSL ES §5.8: "the left operand of the assignment operator must be an l-value."
The parser only checked type compatibility on assignment (AssignTypeMismatch);
shapes that could never be written to went through silently — including the
`#define A 1; A = 2;` case that motivated this addition.
- new `DiagnosticType.InvalidAssignmentTarget` in the Type category
- `ShaderValidator._checkAssignmentTarget` runs on every AssignmentExpression
and descends its LHS via `_nonAssignableReason`, mirroring the grammar's
operator-precedence chain (single-child wrappers pass through, r-value-only
shapes terminate with a specific reason)
- covered non-l-values:
* macro reference (`MacroCallSymbol` / `MacroCallFunction`)
* numeric / boolean literal
* function-call result
* `const`-qualified variable (lookup via `shaderData.symbolTable`)
* compound arithmetic / logical / relational expressions
* unary-operator result
* ternary expression result
- DiagnosticDriverConsistency picks up five real-WebGL cases (macro, literal,
function call, const, compound); DiagnosticCoverage adds the macro trigger
- follows naga's "one error kind, message describes the cause" convention
rather than splitting per LHS shape
…s and assignments
Real WebGL 1 and WebGL 2 drivers reject the following with "cannot convert from
'const int' to 'mediump float'":
float b = 1; // initializer
float b; b = 1; // assignment
float f = 1.0;
float g = f + 1; // binary op
The analyzer previously accepted all of these. The gap had two root causes:
- `TypeSystem.isAssignable` allowed the naga-style scalar promotion set (int →
uint/float, uint → float, ivecN → uvecN/vecN, uvecN → vecN). GLSL ES §4 states
the language has **no implicit conversions between types**; §5.8 (assignment)
and §5.9 (binary operators) require operand types to match; §5.4.1 lists
explicit scalar constructors as the only conversion mechanism. naga's
`implicit_conversion` violates the spec — the driver is the source of truth,
so tighten to `target === source`
- `SingleDeclaration` and `VariableDeclaration` never checked the initializer's
type against the declared type — so `float b = 1;` slipped through. Both now
emit `AssignTypeMismatch` on mismatch (array initializers skipped, they need
component-level checking)
The two pre-existing "does not flag a valid implicit conversion" tests in
ShaderAnalyzer.test.ts were written from the wrong premise — flipped to assert
the diagnostic **must** fire, with in-comment rationale citing the spec.
DiagnosticDriverConsistency picks up three real-WebGL cases:
* `float b = 1;` (initializer)
* `int a = 1.0;` (initializer, other direction)
* `float b; b = 1;` (assignment)
Built-in shader precompile stays clean — no engine shader relied on the removed
scalar promotion.
Every gap here was surfaced by porting a Khronos/glslang negative-test file (`Test/300operations.frag`, `Test/invalidSwizzle.vert`, `Test/300scope.vert`) to Galacean shaderlab DSL and running through a real WebGL driver. Each was a case the analyzer silently accepted while the driver rejected — the exact class of bug the ABC roleplay design misses (it only pressure-tests declared diagnostics, never audits the diagnostic-code list against spec / corpora). Fixes (9 gaps, 5 rule additions, 1 new diagnostic code): - **G3 + G4 · InvalidSwizzle receiver-type check** (`ShaderValidator._checkPostfix`) `s.rr` (sampler receiver) and `f().xx` (void return) now report `Field selection ... requires a structure, vector, or scalar receiver`. §5.5 — receiver must be scalar/vector/struct; the driver rejects with "field selection requires structure, vector, or interface block on left hand side." - **G2 · InvalidAssignmentTarget extended to uniform / sampler / `++`/`--`** `VarSymbol` gains `isUniform` (global var without an initializer, non-const — Galacean's implicit uniform). `_nonAssignableReason` now flags uniforms and samplers as non-l-values; `_checkPostfix` and `_checkUnaryOperand` route `++`/`--` through the same reason check. `u_i++` on a uniform is rejected by the driver as "l-value required (can't modify a uniform)". - **G1 · Modulo (`%`) requires integer operands** (`_checkModuloOperandsInteger`) §5.9 says `%` is defined only on signed/unsigned integer scalar or vector. Floats slipped past `_checkArithmeticOperands` because they're a valid *arithmetic* type; the driver rejects with "wrong operand types." - **G5 + G6 · Logical `&&`/`||`/`^^` require scalar bool operands** (`_checkScalarBoolBinaryOperands`) — §5.9 ES3 restricts these to `bool` only (not `bvecN`, unlike desktop GL). Covers `int && int` and `bool && bvec3`. - **Shift + bitwise operators require integer operands** (`_checkIntegerBinaryOperands`) — §5.9. Sibling of the modulo check. - **G7 + G8 · Arithmetic family mismatch** (`_checkArithmeticFamilyMatch`) `int + float`, `ivec3 + uvec3`, `uint + float` are all rejected because GLSL ES §4 has no implicit conversions between families. Only fires on direct-typed operands (respects Phase-2 constraint disabling compound- expression inference). - **G9 · new `LocalFunctionPrototype` diagnostic** (`_checkLocalFunctionPrototype`) §6 restricts function prototypes to global scope. The grammar accepts `int g();` inside a function body; without this check the parser cascades into a misleading `EntryNotFound`. New Symbol-category diagnostic replaces the cascade with a targeted error. Fixed a related parser gap the same round exposed: `VariableDeclaration` (global scope) never propagated `type.isConst` into `VarSymbol.isConst` — a global `const float C` was `isConst: false`. Now that InvalidAssignmentTarget also relies on `isUniform`, the constructor takes both flags derived from `FullySpecifiedType` and `children.length`. Verification: - 9 new cases in DiagnosticDriverConsistency drive each gap through the real WebGL driver — every one shows `driverExpects: "reject"` with the actual driver log cited in the case comment. - New `LocalFunctionPrototype` picked up by DiagnosticCoverage. - Built-in shader precompile still clean (no engine shader relied on the removed lenient behavior).
Verified against real WebGL: FXAA3_11.glsl uses `#define lumaN luma4B.z; ... lumaN = lumaW;`, a legal swizzle l-value that both WebGL 1 and WebGL 2 drivers accept. The analyzer was categorically rejecting every macro-as-LHS as `Cannot assign to a macro`, producing 2 false-positive errors on the shipping FinalAntiAliasing post-processing shader. A macro's l-value-ness depends on its EXPANSION, not on the fact that it's a macro. Rejecting all macro-LHS is over-eager — the runtime driver already catches genuine `#define K 3; K = 5;` after preprocess. So skip the macro branch in `_nonAssignableReason`; keep the const / uniform / sampler / literal / function-call / compound reasons intact. Also add `BuiltinShaderSmoke.test.ts` — every shipping built-in shader is now walked through `ShaderAnalyzer.analyze()` and asserted to fire no `InvalidAssignmentTarget` / `InvalidSwizzle` / `InvalidBinaryOperands` / `BareGlFragData` / `LocalFunctionPrototype` (the diagnostic codes this PR sequence introduces or tightens). Prior verification only ran `precompile` (codegen), which is why the FXAA regression shipped unnoticed. Bug traced to `dfba45b5d` (the InvalidAssignmentTarget introduction, one commit before this fix). Fix belongs here because the surrounding PR extends the same `_nonAssignableReason` code path.
…y reason, IO defensive gate - **InvalidVoidVariable** (§4.1.1) — `void x;` variable / parameter declarations now fire a specific error. Both `SingleDeclaration` (function-local) and `VariableDeclaration` (global) paths gate on `FullySpecifiedType.type === Keyword.VOID`. Driver rejects with `illegal use of type 'void'`; author gets a targeted message instead of a downstream cascade. - **`_nonAssignableReason` sampler check order** — moved sampler-type check ahead of the isUniform branch. A sampler in ES is always uniform (§4.1.7), so both branches would fire; "a sampler" is a more actionable reason than the generic uniform text. Was previously dead code because the uniform check consumed the sampler case first. - **`_deriveStructVarMap` defensive gate** — the `symbolTable.forEach` walk now gates on `sym.isGlobalVariable`. Currently unreachable because `popScope` always fires before validation, but if error recovery ever leaks a local into the top scope (e.g. from a partially parsed function), the map would misclassify it as a varying / attribute source. `VarSymbol.isGlobalVariable` is set at construction and is the correct axis.
…de `#if/#else` Root cause: the analyzer walks every arm of `#if/#ifdef/#else` regions simultaneously (there is no attempt at branch-selective analysis), so an assignment `T x = 1.0;` where one arm defines `#define T vec3` and the other `#define T float` produces a spurious `AssignTypeMismatch` in whichever arm picks the wrong side of the cross-arm view. Same class of false positive drove 40+ `MissingReturn`s, plus `NonIndexableType` / `IndexOutOfBounds` / `ConstructorArgCount` misfires on shipping shaders. `analyze()` over the built-in corpus reported ~76 error-severity diagnostics none of which reflect real bugs in any specific macro combination. Fix: preserve the diagnostic (author still gets a signal), but downgrade its severity to warning when the report site is inside a non-empty macro branch. Empty-branch (top-level) diagnostics stay errors — those are unconditional and reliable. Wiring: - `TreeNode._branch: BranchSignature` — inherited from the first child that carries a branch signature. Filled in `set()` at parse time, zero analyzer input required. Empty means unconditional. - Parse-time reports (`SemanticAnalyzer.reportError`) consult `symbolTableStack.isInMacroBranch` (the live parser state) and route to `reportWarning` when active. `SyntaxError` bypasses this — it goes through `ShaderTargetParser` directly and stays a hard error. - Post-parse reports (`ShaderValidator._push`) track `_macroBranchDepth` during `_walk`; every AST node with `_branch.length > 0` increments the depth for its subtree, and `_push` chooses `CompilationWarn` vs `CompilationError` based on the depth at report time. Effect on `BuiltinShaderSmoke`: - Before: 22 shipping shaders, 76 error-severity diagnostics, ~30 warnings - After : 22 shipping shaders, 1 error-severity diagnostic (SkyProcedural vec4 vs float @l398, real unconditional issue — not this PR's concern), ~450 warnings preserving the signal - Zero shader breaks the smoke test; the PR-introduced-codes gate stays green
…ors inside `#if/#else`" This reverts commit f5eb959.
…nt types across `#if/#else` Per shader-analyzer scoping rule: within a scope, each name has exactly one type — macros don't fork the type system. When a variable is declared with different data types across mutually-exclusive `#if/#else` branches, whichever type the analyzer picks for downstream lookups produces spurious `AssignTypeMismatch` / `NonIndexableType` cascades. Report the real root cause directly, listing each conflicting site (branch + type + location) so the fix is unambiguous. Implementation: - New `DiagnosticType.MacroBranchConflict` (Symbol category) - `ShaderValidator._reportMacroBranchConflicts` runs post-walk, groups `VarSymbol`s by name, and reports when at least one entry lives in a macro branch AND multiple distinct data types appear - Message lists every declaration site: branch flag, type, line/col - Only variables checked; function overloads share a signature-keyed namespace and are already covered by `Redefinition` Observed on shipping shaders: - `scene_ShadowMap` in `Shadow.glsl` — `sampler2DShadow` under `#ifdef GRAPHICS_API_WEBGL2`, `sampler2D` under `#else`. Deliberate — WebGL2 wants hardware PCF, WebGL1 falls back to manual depth comparison. The conflict IS real per the "one name, one type" rule; the fix would be either renaming (`scene_ShadowMap_WGL2` / `scene_ShadowMap_WGL1`) or agreeing that this API-conditional pattern is exempt from the diagnostic. Downstream classifications for the 75 other diagnostics on built-in shaders (MissingReturn, AssignTypeMismatch, NonIndexableType, IndexOutOfBounds, ConstructorArgCount) are NOT MacroBranchConflict — they're other analyzer categories that need separate treatment (either analyzer improvements to recognize legal GLSL patterns like `length(vec3) -> float`, or shader-level fixes). Tracked separately.
… divergent types across `#if/#else`" This reverts commit 3cdd448.
- SymbolInfo carries the `#ifdef` branch signature of its declaration; SymbolTable filters candidates by `isBranchVisibleFrom` vs a callsite branch - TreeNode inherits a branch from its first terminal descendant; ASTNode.get() pushes it into SymbolTableStack._currentBranch so insert() stamps declarations with the right branch - Reference lookups (VariableIdentifier, PostfixExpression.field, FunctionCallGeneric, ArraySpecifier size) opt in by passing `this._branch` — same/nested branch resolves, mutually-exclusive is invisible - Duplicate / redefinition checks (SingleDeclaration, VariableDeclaration, FunctionDefinition) stay on the legacy `!isInMacroBranch` skip — otherwise the `#ifndef X_INCLUDED / #define X_INCLUDED / #endif` include-guard pattern would falsely trigger Redefinition on every guarded chunk - Adds BranchAwareLookup.test.ts covering same-branch resolve, outer-scope visibility, and preserved errors inside macro branches
- variable_declaration reduces `fully_specified_type ID array_specifier` but SymbolType was constructed without the array specifier arg - Every `float arr[N]` at pass-body scope stored as scalar `float`, so any `arr[i]` reference misfired NonIndexableType with base type 'float' - Pull children[2] as ArraySpecifier when the production is length 3 and thread it through to the VarSymbol
- _blockGuaranteesReturn's CFG treated MacroIfStatement as opaque, so a function body ending in `#ifdef X return a; #else return b; #endif` fired MissingReturn even though every runtime arm returns - Recurse into MacroIfStatement / MacroBranch mirroring the SelectionStatement handling: both `#if` and `#else` arms must guarantee for the block to count - Bare `#endif` (no `#else`) stays conservative — runtime preprocessor may see zero arms match
- IndexOutOfBounds: skip the vector-size bounds check when the base is an array (`ivec2 arr[N]; arr[i]` indexes the outer array, not the inner ivec2). Array-size check still runs when the size is known. - ConstructorArgCount: `matN(matM)` is legal per GLSL ES §5.4.3 (matrix truncated/padded diagonally). Short-circuit the exact-component-count check for single-matrix-arg matrix constructors before it fires on the source's total component count. - AssignTypeMismatch: compound-op assign (`+=` `-=` `*=` `/=`) is `L = L op R`, not `L = R`. `vec3 *= float` is legal (scalar⊙vector broadcast); check assignability of arithmeticResultType(L, R) back to L. Operator token lives at AssignmentOperator.children[0], not AssignmentExpression.children[1] (that's the non-terminal wrapper).
- BuiltinFunction.resolveOverload locked the Size dimension from a scalar arg even when another arg was TypeAny, over-specializing calls like `max(TypeAny, 0.0)` to float. Track `sizeAmbiguous` / `scalarTypeAmbiguous` and fall through to TypeAny when the return family shares an ambiguous dimension. - User-function overload resolution: `SymbolInfo.equal` treats TypeAny args as wildcards, so reverse-insertion lookup silently picks whichever overload was inserted last (e.g. shipping `permute` ships `float`/`vec3`/`vec4` variants; a TypeAny-typed arg matched all three but committed to the last-inserted return type). When multiple overloads match with divergent return types, keep `fnSymbol` set but drop the call's type to TypeAny.
- VariableIdentifier.lookupAll returns every branch-visible decl; if their identity (base type + isArray + arraySize) diverges, commit to TypeAny + isArray=false + arraySize=undefined instead of silently picking the last-inserted candidate (that was environment-sensitive flaky behaviour) - Emits AmbiguousMacroBranchType at the reference site so callers know the checks are disabled; silent TypeAny would hide the fact - Report-once per (pass, symbol name) — a divergent symbol may be referenced dozens of times (renderer_BlendShapeWeights has ~110 index sites across the include chain); flooding the editor buries the signal - Symmetric with FunctionCallGeneric's existing overloadTypeAmbiguous guard - Shipping shaders: 9 warnings across 4 symbols, 0 new error introduced - Perf overhead measured at ~2% wall clock on parseShaderPass; codegen path is unchanged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景与目标
dev/2.0 的 shader 诊断与运行时编译耦合在一包,靠
#if _VERBOSE门控:release 产物零诊断、错误无行列定位、同一错误多次上报、外部无法消费。本 PR 把 shader 体系拆为三包并引入结构化诊断:
架构
flowchart LR SRC["ShaderLab 源码"] --> P["📦 shader-parser<br/>中性 typed-AST + 语义线索<br/>(唯一来源)"] P --> C["⚙️ shader-compiler<br/>codegen → GLSL ES"] P --> A["🔍 shader-analyzer<br/>ShaderValidator 校验 + 收集线索"] C --> OUT["IShaderProgramSource"] A --> DIAG["Diagnostic[]"] classDef base fill:#fff9c4,stroke:#f57f17 classDef comp fill:#d4edda,stroke:#28a745 classDef ana fill:#e3f2fd,stroke:#1976d2 class P base class C,OUT comp class A,DIAG anashader-compiler与shader-analyzer互不依赖,是 parser 之上两个平等消费者。类型/控制流校验抽成独立的ShaderValidatorpass(走 typed-AST),类型语义收口到TypeSystem;与解析序耦合的校验(重载 / 声明序 / const 求值)+ 管线 IO + RenderState 仍在 parser 内联打标。整体对齐 Naga「中性源 + 独立 Validator + 多 backend」的分离方向(建模型 / 校验 / 发射);模型是带注解的 typed-AST,非完全 lower 的 IR。核心机制:校验独立于 codegen
reportError+#if _VERBOSEShaderValidator走 typed-AST 现算;解析序耦合项 parser 内联打标(error-as-clue)ShaderValidator走 typed-AST 做类型/控制流校验 + 收集 parser 线索 →Diagnostic[]对外 API
analyzer.analyze(src)Diagnostic[]WebGLEngine.create({ shaderCompiler, shaderAnalyzer })formatDiagnostic(d)诊断能力
43 个
DiagnosticType,逐条对 Nagavalid/源码核过粒度(过度拆分已合并、ShaderLab/GLSL 专属项标注依据);完整目录 + 正/误示例见 RFC #3017 §3。按实体分类:优势
file:line:col [Code] msg、LLM 拿去自动修复。测试
DiagnosticType+ 位置)。IShaderProgramSource字节对等(precompile 守护)。DiagnosticType必有触发用例,缺测即 fail。npm run build(b:module+b:types,tsc 全量类型检查)后npm run test。