From bf9112d4f8d9b0a09ea613e98132a5fe94defc2d Mon Sep 17 00:00:00 2001 From: Kareem Albaba Date: Sat, 9 May 2026 06:59:40 -0400 Subject: [PATCH] fix(extractors): address PR review findings on UE macro preprocessor Resolves three blocking issues raised by automated review: 1. Prettier format: ran prettier --write on call-processor.ts, heritage-processor.ts, import-processor.ts (the three sites where the cache-miss reparse hook insertion landed unformatted). 2. Byte-length contract narrowed: language-provider.ts docblock now states the contract precisely (UTF-16 .length + newline-position preservation, not UTF-8 byte length). Notes that startIndex byte offsets only match the original file when the elided range is pure ASCII -- which is the practical UE case (reflection macros and module-export tokens are ASCII-only). 3. Tree-sitter extraction tests added: new end-to-end tests parse the preprocessed source with tree-sitter-cpp and assert the captured class/struct name is the real UClass identifier (UMyClass, FMyData), never the MODULE_API export macro. Also asserts source positions (startPosition.row) survive the transform. Plus one moderate fix: 4. _API stripping is now scoped to UE files only. The HAS_UE_HINT guard previously included [A-Z]_API tokens, which would fire on non-UE codebases that use REST_API / HTTP_API / MY_LIB_API as constants or enum values, silently erasing them. The guard now requires a strong UE marker (UCLASS|UFUNCTION|UPROPERTY|USTRUCT|UENUM|UINTERFACE|GENERATED_BODY|UE_DEPRECATED|DECLARE_*_DELEGATE) to be present before any stripping runs. Two new tests confirm REST_API and DECLARE_HANDLER style identifiers in non-UE files are left untouched. Plus one minor fix: 5. stripUeMacros signature now accepts (source, _filePath?) to match the LanguageProvider.preprocessSource hook contract exactly. The filePath argument is unused; UE detection is purely content-based. Verification: 34/34 preprocessor tests pass (was 27, +7 new for non-ASCII preservation, REST_API safety, tree-sitter extraction, struct extraction, source position preservation). Full unit suite 5349 pass, 0 regressions. Typecheck clean. Prettier --check clean on all 9 changed files. --- gitnexus/src/core/ingestion/call-processor.ts | 6 +- .../src/core/ingestion/cpp-ue-preprocessor.ts | 24 +++- .../src/core/ingestion/heritage-processor.ts | 6 +- .../src/core/ingestion/import-processor.ts | 3 +- .../src/core/ingestion/language-provider.ts | 21 +++- .../test/unit/cpp-ue-preprocessor.test.ts | 115 +++++++++++++++++- 6 files changed, 152 insertions(+), 23 deletions(-) diff --git a/gitnexus/src/core/ingestion/call-processor.ts b/gitnexus/src/core/ingestion/call-processor.ts index c1aeba95b..bf0206057 100644 --- a/gitnexus/src/core/ingestion/call-processor.ts +++ b/gitnexus/src/core/ingestion/call-processor.ts @@ -769,8 +769,7 @@ export const processCalls = async ( let tree = astCache.get(file.path); if (!tree) { - const parseContent = - provider.preprocessSource?.(file.content, file.path) ?? file.content; + const parseContent = provider.preprocessSource?.(file.content, file.path) ?? file.content; try { tree = parser.parse(parseContent, undefined, { bufferSize: getTreeSitterBufferSize(parseContent), @@ -3282,8 +3281,7 @@ export const extractFetchCallsFromFiles = async ( let tree = astCache.get(file.path); if (!tree) { - const parseContent = - provider.preprocessSource?.(file.content, file.path) ?? file.content; + const parseContent = provider.preprocessSource?.(file.content, file.path) ?? file.content; try { tree = parser.parse(parseContent, undefined, { bufferSize: getTreeSitterBufferSize(parseContent), diff --git a/gitnexus/src/core/ingestion/cpp-ue-preprocessor.ts b/gitnexus/src/core/ingestion/cpp-ue-preprocessor.ts index ba37df6aa..baa5d17ff 100644 --- a/gitnexus/src/core/ingestion/cpp-ue-preprocessor.ts +++ b/gitnexus/src/core/ingestion/cpp-ue-preprocessor.ts @@ -18,7 +18,18 @@ * * Pure function — no tree-sitter dependency, safe for worker threads. */ -const HAS_UE_HINT = /\b(?:UCLASS|UFUNCTION|UPROPERTY|USTRUCT|UENUM|UINTERFACE|GENERATED_BODY|GENERATED_[A-Z_]+_BODY|UE_DEPRECATED|DECLARE_(?:DYNAMIC_)?(?:MULTICAST_)?DELEGATE|[A-Z][A-Z0-9_]*_API)/; +/** + * Strong UE markers — reflection macros that only Unreal Engine projects use. + * Presence of one of these is sufficient evidence that the file is a UE source + * and that `MODULENAME_API` tokens in it are intended as export macros. + * + * Importantly, `_API` tokens are NOT in this guard — `REST_API`, `HTTP_API`, + * `MY_LIB_API` and similar identifiers appear in plenty of non-UE C++ codebases + * as constants/enums/parameter names. We must not erase them just because the + * file mentions an `_API` token. + */ +const HAS_UE_HINT = + /\b(?:UCLASS|UFUNCTION|UPROPERTY|USTRUCT|UENUM|UINTERFACE|GENERATED_BODY|GENERATED_[A-Z_]+_BODY|UE_DEPRECATED|DECLARE_(?:DYNAMIC_)?(?:MULTICAST_)?DELEGATE)/; const SIMPLE_MACROS_NO_ARGS: readonly string[] = [ 'GENERATED_BODY', @@ -185,10 +196,15 @@ function skipWhitespace(source: string, idx: number): number { /** * Strip Unreal Engine reflection macros from C++ source, length-preserving. * - * Returns the original string unchanged if no UE markers are detected, so - * non-UE C++ files incur only a single regex test. + * Returns the original string unchanged if no strong UE marker is detected, + * so non-UE C++ files (including ones that contain `*_API`-suffixed + * identifiers like `REST_API` or `HTTP_API`) incur only a single regex test. + * + * The `_filePath` parameter is part of the `LanguageProvider.preprocessSource` + * contract but is unused — UE detection is purely content-based. Accepted and + * ignored here so the function matches the hook signature exactly. */ -export function stripUeMacros(source: string): string { +export function stripUeMacros(source: string, _filePath?: string): string { if (!HAS_UE_HINT.test(source)) return source; const chars: string[] = source.split(''); diff --git a/gitnexus/src/core/ingestion/heritage-processor.ts b/gitnexus/src/core/ingestion/heritage-processor.ts index 2727f1116..f8628e651 100644 --- a/gitnexus/src/core/ingestion/heritage-processor.ts +++ b/gitnexus/src/core/ingestion/heritage-processor.ts @@ -222,8 +222,7 @@ export const processHeritage = async ( // Per-language source preprocessor (length-preserving, e.g. UE macro // stripping for C++). MUST mirror parsing-processor on cache miss so // re-parses see the same input as the cached AST. - const parseContent = - provider.preprocessSource?.(file.content, file.path) ?? file.content; + const parseContent = provider.preprocessSource?.(file.content, file.path) ?? file.content; try { tree = parser.parse(parseContent, undefined, { bufferSize: getTreeSitterBufferSize(parseContent), @@ -418,8 +417,7 @@ export async function extractExtractedHeritageFromFiles( let tree = astCache.get(file.path); if (!tree) { - const parseContent = - provider.preprocessSource?.(file.content, file.path) ?? file.content; + const parseContent = provider.preprocessSource?.(file.content, file.path) ?? file.content; try { tree = parser.parse(parseContent, undefined, { bufferSize: getTreeSitterBufferSize(parseContent), diff --git a/gitnexus/src/core/ingestion/import-processor.ts b/gitnexus/src/core/ingestion/import-processor.ts index b51f69428..6f0b40b6f 100644 --- a/gitnexus/src/core/ingestion/import-processor.ts +++ b/gitnexus/src/core/ingestion/import-processor.ts @@ -305,8 +305,7 @@ export const processImports = async ( let wasReparsed = false; if (!tree) { - const parseContent = - provider.preprocessSource?.(file.content, file.path) ?? file.content; + const parseContent = provider.preprocessSource?.(file.content, file.path) ?? file.content; try { tree = parser.parse(parseContent, undefined, { bufferSize: getTreeSitterBufferSize(parseContent), diff --git a/gitnexus/src/core/ingestion/language-provider.ts b/gitnexus/src/core/ingestion/language-provider.ts index 1a3a1335e..e139cf5f3 100644 --- a/gitnexus/src/core/ingestion/language-provider.ts +++ b/gitnexus/src/core/ingestion/language-provider.ts @@ -124,11 +124,22 @@ interface LanguageProviderConfig { * `UFUNCTION`, `MODULENAME_API`) in C++ headers that prevent the parser from * recognising class/function names correctly. * - * **Length-preserving requirement:** the returned string MUST have the same - * byte length as the input. Implementations should replace removed text with - * spaces, preserving newlines, so tree-sitter's reported byte offsets and - * line/column positions still match the original file. Violating this will - * silently corrupt symbol locations in the graph. + * **Length / position preservation:** the returned string MUST have the same + * JavaScript `.length` as the input AND preserve every newline (`\n`/`\r`) + * position byte-for-byte. Implementations replace elided characters with + * ASCII spaces while leaving newlines untouched. With this contract: + * + * - tree-sitter's reported `startPosition.row`/`startPosition.column` + * match the original file exactly (line/column come from newline counts) + * - `startIndex`/`endIndex` byte offsets match the original file exactly + * **when the elided range is pure ASCII** (UTF-16 `.length` equals UTF-8 + * byte length only for ASCII). + * + * Implementations targeting languages where elided ranges may contain + * non-ASCII content must therefore preserve byte length, not just `.length`, + * if downstream code uses `startIndex` to slice the original UTF-8 bytes. + * The current C++ UE-macro preprocessor relies on the practical fact that + * UE reflection macros and module-export tokens are ASCII-only. * * Must be a pure function — same input always yields the same output. Called * once per file, on every code path that re-parses (parsing-processor, import diff --git a/gitnexus/test/unit/cpp-ue-preprocessor.test.ts b/gitnexus/test/unit/cpp-ue-preprocessor.test.ts index c282320f1..087c5ba2a 100644 --- a/gitnexus/test/unit/cpp-ue-preprocessor.test.ts +++ b/gitnexus/test/unit/cpp-ue-preprocessor.test.ts @@ -1,4 +1,6 @@ import { describe, it, expect } from 'vitest'; +import Parser from 'tree-sitter'; +import CPP from 'tree-sitter-cpp'; import { stripUeMacros } from '../../src/core/ingestion/cpp-ue-preprocessor.js'; describe('stripUeMacros — detection guard', () => { @@ -57,8 +59,8 @@ describe('stripUeMacros — macro removal', () => { expect(out).toContain('class UBar {};'); }); - it('elides MODULE_API export macros (BRAWLUI_API style)', () => { - const src = `class BRAWLUI_API UMyClass : public UObject {};`; + it('elides MODULE_API export macros (BRAWLUI_API style) when paired with a UE marker', () => { + const src = `UCLASS()\nclass BRAWLUI_API UMyClass : public UObject {};`; const out = stripUeMacros(src); expect(out).not.toContain('BRAWLUI_API'); expect(out).toContain('class'); @@ -66,8 +68,8 @@ describe('stripUeMacros — macro removal', () => { expect(out).toContain('public UObject'); }); - it('elides multiple distinct *_API tokens in same file', () => { - const src = `class CORE_API A {};\nclass UMG_API B : public A {};`; + it('elides multiple distinct *_API tokens in same file when UE marker is present', () => { + const src = `UCLASS()\nclass CORE_API A {};\nUCLASS()\nclass UMG_API B : public A {};`; const out = stripUeMacros(src); expect(out).not.toContain('CORE_API'); expect(out).not.toContain('UMG_API'); @@ -116,6 +118,43 @@ describe('stripUeMacros — macro removal', () => { }); }); +describe('stripUeMacros — non-UE files left alone', () => { + it('does NOT strip standalone *_API identifiers when no UE marker is present', () => { + const src = `enum class Status { REST_API = 1, HTTP_API = 2, MY_LIB_API = 3 };\nvoid handle(REST_API status);`; + expect(stripUeMacros(src)).toBe(src); + }); + + it('does NOT strip _API tokens in a file that only mentions DECLARE_DELEGATE-like macros from non-UE codebases', () => { + const src = `// Custom delegate framework, not UE\n#define DECLARE_HANDLER(x) void x()\nDECLARE_HANDLER(MyHandler);\nint REST_API = 0;`; + expect(stripUeMacros(src)).toBe(src); + }); +}); + +describe('stripUeMacros — non-ASCII content preservation', () => { + it('leaves non-ASCII content outside elided ranges intact and at the same .length offset', () => { + const src = `// Comment with non-ASCII: café résumé naïve\nUCLASS()\nclass UMyClass : public UObject\n{\n GENERATED_BODY()\n // Trailing: 日本語 αβγ\n};`; + const out = stripUeMacros(src); + expect(out.length).toBe(src.length); + expect(out).toContain('café résumé naïve'); + expect(out).toContain('日本語 αβγ'); + expect(out).toContain('class UMyClass : public UObject'); + expect(out).not.toContain('UCLASS'); + expect(out).not.toContain('GENERATED_BODY'); + }); + + it('preserves newline positions when the file contains non-ASCII characters', () => { + const src = `// café\nUPROPERTY()\nint32 Health;\n// résumé\nUFUNCTION()\nvoid Run();`; + const out = stripUeMacros(src); + const inputNewlines: number[] = []; + const outputNewlines: number[] = []; + for (let i = 0; i < src.length; i++) { + if (src.charCodeAt(i) === 0x0a) inputNewlines.push(i); + if (out.charCodeAt(i) === 0x0a) outputNewlines.push(i); + } + expect(outputNewlines).toEqual(inputNewlines); + }); +}); + describe('stripUeMacros — false-positive guards', () => { it('does NOT strip identifiers that merely contain UCLASS as a substring', () => { const src = `void NotUCLASSAtAll(); int MyUCLASS = 0;`; @@ -163,3 +202,71 @@ describe('stripUeMacros — class-name extraction sanity', () => { expect(tail.startsWith('UMyClass')).toBe(true); }); }); + +describe('stripUeMacros — tree-sitter extraction (end-to-end)', () => { + /** + * Walk the parse tree and return the captured class name(s). Works against + * the actual tree-sitter-cpp grammar so this is a true integration check + * for the core PR claim: the indexer now sees `UMyClass`, not `BRAWLUI_API`. + */ + function extractClassNames(source: string): string[] { + const parser = new Parser(); + parser.setLanguage(CPP as unknown as Parser.Language); + const tree = parser.parse(source); + const names: string[] = []; + const stack: Parser.SyntaxNode[] = [tree.rootNode]; + while (stack.length > 0) { + const node = stack.pop()!; + if (node.type === 'class_specifier' || node.type === 'struct_specifier') { + const nameNode = node.childForFieldName('name'); + if (nameNode) names.push(nameNode.text); + } + for (let i = node.namedChildCount - 1; i >= 0; i--) { + const child = node.namedChild(i); + if (child) stack.push(child); + } + } + return names; + } + + it('tree-sitter-cpp captures UMyClass as the class name (not BRAWLUI_API)', () => { + const src = `UCLASS(BlueprintType)\nclass BRAWLUI_API UMyClass : public UObject\n{\n GENERATED_BODY()\n public:\n UFUNCTION()\n void Run();\n};`; + const out = stripUeMacros(src); + const names = extractClassNames(out); + expect(names).toContain('UMyClass'); + expect(names).not.toContain('BRAWLUI_API'); + }); + + it('tree-sitter-cpp captures struct name correctly through USTRUCT + MODULE_API', () => { + const src = `USTRUCT(BlueprintType)\nstruct ENGINE_API FMyData : public FBase\n{\n GENERATED_BODY()\n float Value;\n};`; + const out = stripUeMacros(src); + const names = extractClassNames(out); + expect(names).toContain('FMyData'); + expect(names).not.toContain('ENGINE_API'); + }); + + it('tree-sitter-cpp source positions are preserved across stripping (line numbers match)', () => { + const src = `UCLASS()\nclass BRAWLUI_API UMyClass : public UObject\n{\n GENERATED_BODY()\n public:\n void Run();\n};`; + const out = stripUeMacros(src); + const parser = new Parser(); + parser.setLanguage(CPP as unknown as Parser.Language); + const tree = parser.parse(out); + const stack: Parser.SyntaxNode[] = [tree.rootNode]; + let runLine: number | undefined; + while (stack.length > 0) { + const node = stack.pop()!; + if (node.type === 'function_declarator') { + const declarator = node.childForFieldName('declarator'); + if (declarator?.text === 'Run') { + runLine = node.startPosition.row; + break; + } + } + for (let i = node.namedChildCount - 1; i >= 0; i--) { + const child = node.namedChild(i); + if (child) stack.push(child); + } + } + expect(runLine).toBe(5); // 0-indexed: "void Run();" is on line 6 (index 5) + }); +});