fix(gleam): extract parameters for external functions#1127
Conversation
Mirror regular `function` behavior so `external_function` definitions expose their parameter list as children in both native and WASM engines. Previously external functions silently dropped parameter children even though the grammar exposes them via the same `parameters` field. Closes #1110
|
Claude finished @carlos-alm's task —— View job Pull Request Review - COMPLETED ⭐⭐⭐⭐⭐Conducting comprehensive review of PR #1127 - fix(gleam): extract parameters for external functions Review Checklist
Root Cause AnalysisProblem Being Solved: Gleam external function definitions were missing parameter extraction, creating engine parity gap and inconsistency with regular function behavior. External functions had hardcoded Why This Approach: Tree-sitter Risk Assessment: ✅ MINIMAL RISK - Purely additive change that reuses battle-tested extraction logic. No modification of existing behavior. Backlog Compliance
Test Integrity Analysis ✅ EXCELLENT
Code Quality Assessment ✅ HIGH QUALITYImplementation Details:
Potential Issues Checked:
Best Practices:
Critical ConcernsNONE IDENTIFIED - This is a clean, well-implemented fix that addresses a legitimate parity gap. Final Recommendation
Specific Strengths:
|
Greptile SummaryThis PR extends parameter extraction to external Gleam functions in both the Rust and TypeScript engines by reusing the existing
Confidence Score: 5/5Safe to merge — the change is additive, touches only the external-function handler in both engines, and reuses the already-tested Both engines receive a two-line addition that calls an existing, well-tested helper and wires its output into No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Tree-sitter parses Gleam source] --> B{Node kind?}
B -- function --> C[handle_function / handleFunction]
B -- external_function --> D[handle_external_function / handleExternalFunction]
C --> E[extract_params / extractParams]
D --> E
E --> F{parameters field\nor function_parameters child?}
F -- None --> G[return empty Vec / array]
F -- found --> H[iterate parameter children]
H --> I{param kind: function_parameter\nor parameter?}
I -- yes --> J{has name field\nor identifier child?}
J -- yes --> K[push child def: kind=parameter]
J -- no --> L[skip — type-only param]
I -- no, identifier --> K
I -- other --> L
G --> M[children: None / undefined]
K --> N{params non-empty?}
N -- yes --> O[children: Some / defined]
N -- no --> M
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('omits children for external functions with type-only parameters', () => { | ||
| const symbols = parseGleam(`pub external fn random() -> Int = "rand" "uniform"`); | ||
| const randomFn = symbols.definitions.find((d) => d.name === 'random'); | ||
| expect(randomFn).toBeDefined(); | ||
| expect(randomFn?.children).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
The test is named 'omits children for external functions with type-only parameters' and its Rust counterpart has the comment // External function with type-only parameters (no names), yet both use random() — a function with zero parameters, not one with anonymous type-only params (e.g. random(Int, String)). These are structurally different tree nodes: an empty parameter list has no children at all, whereas random(Int, String) produces parameter nodes that simply lack a name field. The PR description explicitly calls out "Type-only external params (no name field) are correctly omitted", but that code path is never exercised by either test suite.
There was a problem hiding this comment.
Fixed in 7dde2c3 — both the JS and Rust tests now use random(Int, String) (type-only parameters with no name field) instead of random() (zero parameters), so the assertion that children is undefined actually exercises the documented edge case: parameter nodes that exist in the tree but get filtered out by extractParams/extract_params because they lack a name field.
Codegraph Impact Analysis4 functions changed → 3 callers affected across 2 files
|
…1127) Replace random() (zero parameters) with random(Int, String) (type-only parameters) so the test actually covers the documented edge case: parameter nodes that exist in the tree but lack a name field. The old case asserted the same observable outcome (no children) but exercised the empty-parameter-list path, not the type-only path. Applies to both Rust and JS engines.
1 participant
Summary
functionbehavior inhandle_external_function(Rust) andhandleExternalFunction(JS) so external function definitions expose their parameter list as children in both engines.external_functionexposes aparametersfield (aliased fromexternal_function_parameterstofunction_parameters) with the same shape as regularfunction, soextract_params/extractParamscan be reused as-is. Type-only external params (nonamefield) are correctly omitted.Test plan
cargo test --lib extractors::gleam— 12/12 pass (2 new tests cover named and nameless external params).npx vitest run tests/parsers/gleam.test.ts— 6/6 pass (2 new tests cover named and nameless external params).npx vitest run tests/parsers/— 476/476 pass afternpm run build.cargo test --lib— 299/299 pass.--engine wasm):codegraph children parselistsinputandbaseasparameterchildren.Closes #1110