From b09fd5019ba5472f28d0e50ec3cd0d514a141da2 Mon Sep 17 00:00:00 2001 From: "chenmo.gl" Date: Fri, 24 Apr 2026 18:39:17 +0800 Subject: [PATCH] refactor(shader-lab): collapse MacroDefineInfo classifier into single referenceName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: `MacroDefineInfo` carried `valueType: MacroValueType`, `value: string`, and `functionCallName: string` — three fields that together encoded one fact ("the external identifier this macro references, if any"). The `Other` branch of `MacroValueType` plus the `Logger.warn` in `getReferenceSymbolNames` encoded a second fact ("the regex doesn't understand this value"). After the AST-first rewrite, the second fact is misleading: AST-form macros with `Other`-shaped values (e.g. `v.v_uv`, `mat3(...)`) are handled by the visitor pipeline, not a failure to recognize. Collapse the classifier into one field: interface MacroDefineInfo { isFunction: boolean; name: string; params: string[]; valueAst?: ASTNode.AssignmentExpression; referenceName: string; // new } - `valueType` / `value` / `functionCallName` removed - `enum MacroValueType` removed (no external consumers — verified via grep) - `_parseMacroDefines` simplified: regex classification collapsed into a single `_extractReferenceName` helper - `getReferenceSymbolNames` simplified: a linear "push each entry's non-empty, non-parameter referenceName" loop; no branches, no warnings - `_isExist` dedup uses `referenceName` instead of `value`/`functionCallName` Also: `MacroDefine.semanticAnalyze` now upgrades the existing preprocessor entry in place (attaching `valueAst`) rather than pushing a duplicate. The duplicate caused the spurious warnings this commit originally targeted, then `MacroValueType` going away removed the warning itself entirely. Regression guard: `macro-member-access-builtin-arg` test spies on `Logger.warn` and asserts zero "unrecognized value" warnings — ensures the removed warning can't sneak back via similar double-bookkeeping. Net: -34 source lines, -40 with tests. No behavior change beyond the warning no longer firing; 1301/1301 suite passes. --- packages/shader-lab/src/Preprocessor.ts | 119 ++++++++---------------- packages/shader-lab/src/parser/AST.ts | 43 +++++---- tests/src/shader-lab/ShaderLab.test.ts | 54 +++++++---- 3 files changed, 98 insertions(+), 118 deletions(-) diff --git a/packages/shader-lab/src/Preprocessor.ts b/packages/shader-lab/src/Preprocessor.ts index da6bec01d..4a9aaa31a 100644 --- a/packages/shader-lab/src/Preprocessor.ts +++ b/packages/shader-lab/src/Preprocessor.ts @@ -4,47 +4,32 @@ import { ShaderLib } from "@galacean/engine"; import type { ASTNode } from "./parser/AST"; /** - * Classification of a `#define` value. Used by the opaque-token (legacy) path; - * expression-style macros go straight to AST and don't need this tag. + * Record for a single `#define` directive. * - * @internal - */ -export enum MacroValueType { - Number, // 1, 1.1 - Symbol, // variable name - FunctionCall, // function call, e.g. clamp(a, 0.0, 1.0) - Other // shaderLab does not check this -} - -/** - * Record for a single `#define` directive. Two shapes coexist: + * Two shapes coexist and can be combined on the same entry: * - * - **Expression macros** — have `valueAst` pointing at an `AssignmentExpression` - * subtree. These participate in type inference, varying flattening, and reference - * tracking through the normal visitor pattern, just like inline expressions. - * - **Opaque macros** — the value is a qualifier/type/partial-syntax form that can't - * be parsed as a GLSL expression. `valueAst` is undefined and the lexer kept the - * whole directive in a legacy `MACRO_DEFINE_EXPRESSION` token. + * - **Expression macros** — `valueAst` holds an `AssignmentExpression` subtree + * produced by the `macro_define` CFG rule. These participate in type + * inference, varying flattening, and visitor-based codegen. + * - **Legacy opaque macros** — the directive is emitted verbatim as a + * `MACRO_DEFINE_EXPRESSION` token and expanded by the GLSL driver. `valueAst` + * is undefined. * - * Both shapes share `isFunction`, `name`, `params`, and `value` for display and - * reference tracking. `functionCallName` is set only for opaque macros whose value - * is a function-call form, enabling `getReferenceSymbolNames` to record the callee. + * `referenceName` captures the top-level external identifier the value refers + * to, if any — the callee of a function-call value (`#define F foo(a,b)` + * → `foo`) or the single-identifier value (`#define F foo` → `foo`). Empty + * for numeric literals, qualifier fragments, or structurally complex values. + * Populated by the regex pass; drives symbol-table lookup for macro call + * sites, independent of whether the value also has an AST form. */ export interface MacroDefineInfo { isFunction: boolean; name: string; - /** Classification of the raw value text. Populated for opaque (legacy) macros - * only — AST-form macros leave this as `Other` since classification doesn't - * drive their semantics. */ - valueType: MacroValueType; - /** Raw value text for opaque macros; empty string for AST-form macros. */ - value: string; + params: string[]; /** AST for expression-style macros. Absent for opaque macros. */ valueAst?: ASTNode.AssignmentExpression; - params: string[]; - /** Function callee name for opaque macros classified as `FunctionCall`; empty - * string otherwise. */ - functionCallName: string; + /** Top-level external identifier the value references, or empty string. */ + referenceName: string; } export interface MacroDefineList { @@ -56,7 +41,7 @@ export class Preprocessor { private static readonly _macroRegex = /^\s*#define\s+(\w+)[ ]*(\(([^)]*)\))?[ ]+(\(?\w+\)?.*?)(?:\/\/.*|\/\*.*?\*\/)?\s*$/gm; private static readonly _symbolReg = /^[a-zA-Z_][a-zA-Z0-9_]*$/; - private static readonly _funcCallReg = /^([a-zA-Z_][a-zA-Z0-9_]*)\s*\((.*)\)$/; + private static readonly _funcCallReg = /^([a-zA-Z_][a-zA-Z0-9_]*)\s*\(.*\)$/; private static readonly _macroDefineIncludeMap = new Map(); /** @@ -79,47 +64,40 @@ export class Preprocessor { } /** - * For a given macro name, collect the external symbols its body references so - * callers of the macro can drive symbol-table lookup. Only the opaque-macro path - * needs this preprocessor-driven collection; expression macros carry their refs - * in the AST and `MacroDefine.semanticAnalyze` collects them directly. + * For each registered definition of `macroName`, push its `referenceName` + * (when non-empty and not a macro parameter) so call sites can drive + * symbol-table lookup. */ static getReferenceSymbolNames(macroDefineList: MacroDefineList, macroName: string, out: string[]): void { out.length = 0; const infos = macroDefineList[macroName]; if (!infos) return; - for (let i = 0; i < infos.length; i++) { + for (let i = 0, n = infos.length; i < n; i++) { const info = infos[i]; - // Expression macros handle their own reference tracking via the AST subtree - // (see `MacroDefine.semanticAnalyze`). Nothing to collect here for them. - if (info.valueAst) continue; - const valueType = info.valueType; - if (valueType === MacroValueType.FunctionCall || valueType === MacroValueType.Symbol) { - const referencedName = valueType === MacroValueType.FunctionCall ? info.functionCallName : info.value; - if (info.params.indexOf(referencedName) !== -1) continue; - if (out.indexOf(referencedName) === -1) out.push(referencedName); - } else if (valueType === MacroValueType.Other) { - // #if _VERBOSE - Logger.warn( - `Warning: Macro "${info.name}" has an unrecognized value "${info.value}". ShaderLab does not validate this type.` - ); - // #endif - } + const ref = info.referenceName; + if (!ref) continue; + if (info.params.indexOf(ref) !== -1) continue; + if (out.indexOf(ref) === -1) out.push(ref); } } - private static _isNumber(str: string): boolean { - return !isNaN(Number(str)); + /** + * Extract the top-level identifier a value refers to, or empty string if + * the value is a numeric literal, qualifier fragment, or structurally + * complex form we don't try to introspect. + */ + private static _extractReferenceName(value: string): string { + if (this._symbolReg.test(value)) return value; + const callMatch = this._funcCallReg.exec(value); + return callMatch ? callMatch[1] : ""; } private static _isExist(list: MacroDefineInfo[], item: MacroDefineInfo): boolean { return list.some( (e) => - e.valueType === item.valueType && - e.value === item.value && e.isFunction === item.isFunction && - e.functionCallName === item.functionCallName && + e.referenceName === item.referenceName && e.params.length === item.params.length && e.params.every((p, i) => p === item.params[i]) ); @@ -140,30 +118,9 @@ export class Preprocessor { .filter(Boolean) : []; const value = valueRaw ? valueRaw.trim() : ""; + const referenceName = value ? this._extractReferenceName(value) : ""; - let valueType = MacroValueType.Other; - let functionCallName = ""; - - if (this._isNumber(value)) { - valueType = MacroValueType.Number; - } else if (this._symbolReg.test(value)) { - valueType = MacroValueType.Symbol; - } else { - const callMatch = this._funcCallReg.exec(value); - if (callMatch) { - valueType = MacroValueType.FunctionCall; - functionCallName = callMatch[1]; - } - } - - const info: MacroDefineInfo = { - isFunction, - name, - value, - valueType, - params, - functionCallName - }; + const info: MacroDefineInfo = { isFunction, name, params, referenceName }; const arr = outMacroList[name]; if (arr) { diff --git a/packages/shader-lab/src/parser/AST.ts b/packages/shader-lab/src/parser/AST.ts index ac0685466..9fe770be9 100644 --- a/packages/shader-lab/src/parser/AST.ts +++ b/packages/shader-lab/src/parser/AST.ts @@ -4,7 +4,7 @@ import { ETokenType, GalaceanDataType, ShaderRange, TokenType, TypeAny } from ". import { BaseToken } from "../common/BaseToken"; import { Keyword } from "../common/enums/Keyword"; import { ParserUtils } from "../ParserUtils"; -import { MacroDefineInfo, MacroValueType, Preprocessor } from "../Preprocessor"; +import { MacroDefineInfo, Preprocessor } from "../Preprocessor"; import { ShaderLabUtils } from "../ShaderLabUtils"; import { BuiltinFunction, BuiltinVariable, NonGenericGalaceanType } from "./builtin"; import { NoneTerminal } from "./GrammarSymbol"; @@ -1735,24 +1735,33 @@ export namespace ASTNode { this.valueExpression = children[valueIdx] as AssignmentExpression; } - // Register this macro in the preprocessor's define list so downstream passes - // (e.g. `MacroCallSymbol.semanticAnalyze` resolving references) see it. The - // AST path stores `valueAst` and leaves the legacy opaque fields empty. - const info: MacroDefineInfo = { - isFunction: this.isFunction, - name: this.macroName, - value: "", - valueType: MacroValueType.Other, - valueAst: this.valueExpression, - params, - functionCallName: "" - }; - + // `Preprocessor._parseMacroDefines` already registered a regex-derived + // entry for this directive (so the lexer can classify the name as + // `MACRO_CALL`). Upgrade that entry in place by attaching the AST subtree, + // rather than pushing a duplicate record — duplicates leak into + // `getReferenceSymbolNames` and cause phantom issues like spurious + // warnings or double-counted references. const list = sa.macroDefineList; - if (list[this.macroName]) { - list[this.macroName].push(info); + const entries = list[this.macroName]; + const sameArity = (info: MacroDefineInfo) => + info.isFunction === this.isFunction && + info.params.length === params.length && + info.params.every((p, i) => p === params[i]); + const upgradable = entries?.find((info) => !info.valueAst && sameArity(info)); + if (upgradable) { + upgradable.valueAst = this.valueExpression; } else { - list[this.macroName] = [info]; + // No matching preprocessor entry (e.g. the lexer was fed the directive + // directly without a `Preprocessor.parse` pass). Push a fresh entry. + const info: MacroDefineInfo = { + isFunction: this.isFunction, + name: this.macroName, + params, + valueAst: this.valueExpression, + referenceName: "" + }; + if (entries) entries.push(info); + else list[this.macroName] = [info]; } } diff --git a/tests/src/shader-lab/ShaderLab.test.ts b/tests/src/shader-lab/ShaderLab.test.ts index 1fd53c69f..ef276769e 100644 --- a/tests/src/shader-lab/ShaderLab.test.ts +++ b/tests/src/shader-lab/ShaderLab.test.ts @@ -14,7 +14,7 @@ import { glslValidate } from "./ShaderValidate"; import { Logger, WebGLEngine } from "@galacean/engine"; import { server } from "@vitest/browser/context"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; const { readFile } = server.commands; Logger.enable(); registerIncludes(); @@ -300,28 +300,42 @@ describe("ShaderLab", async () => { it("macro-member-access-builtin-arg (Cocos FSInput pattern: member access macro as builtin fn arg)", async () => { const shaderSource = await readFile("./shaders/macro-member-access-builtin-arg.shader"); - glslValidate(engine, shaderSource, shaderLabRelease); - // Also verify verbose mode (semantic analysis) succeeds — this was the original bug: - // member access macros like #define FSInput_worldNormal v.v_normal.xyz resolved to - // struct type "Varyings" instead of TypeAny, causing builtin overload matching to fail. - const shader = shaderLabVerbose._parseShaderSource(shaderSource); - const passSource = shader.subShaders[0].passes[0]; - const { vertex, fragment } = shaderLabVerbose._parseShaderPass( - passSource.contents, - passSource.vertexEntry, - passSource.fragmentEntry, - 0, - "" - )!; + // Regression guard: before the preprocessor/AST deduplication fix, each + // AST-form member-access macro (e.g. `#define FSInput_worldNormal v.v_normal.xyz`) + // fired a spurious "has an unrecognized value" warning on every access. + const warnSpy = vi.spyOn(Logger, "warn"); + try { + glslValidate(engine, shaderSource, shaderLabRelease); - expect(vertex).to.be.a("string").and.not.empty; - expect(fragment).to.be.a("string").and.not.empty; + // Also verify verbose mode (semantic analysis) succeeds — this was the original bug: + // member access macros resolved to struct type "Varyings" instead of TypeAny, + // causing builtin overload matching to fail. + const shader = shaderLabVerbose._parseShaderSource(shaderSource); + const passSource = shader.subShaders[0].passes[0]; + const { vertex, fragment } = shaderLabVerbose._parseShaderPass( + passSource.contents, + passSource.vertexEntry, + passSource.fragmentEntry, + 0, + "" + )!; - // Verify key builtins are present in output (macros expanded correctly) - expect(fragment).to.contain("normalize"); - expect(fragment).to.contain("dot"); - expect(fragment).to.contain("texture2D"); + expect(vertex).to.be.a("string").and.not.empty; + expect(fragment).to.be.a("string").and.not.empty; + + // Verify key builtins are present in output (macros expanded correctly) + expect(fragment).to.contain("normalize"); + expect(fragment).to.contain("dot"); + expect(fragment).to.contain("texture2D"); + + const unrecognizedCalls = warnSpy.mock.calls.filter((args) => + args.some((a) => typeof a === "string" && a.includes("unrecognized value")) + ); + expect(unrecognizedCalls).to.have.lengthOf(0); + } finally { + warnSpy.mockRestore(); + } }); it("global-varying-var (Cocos VSOutput pattern: global Varyings var with #define macros)", async () => {