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 the Gleam extractor in both the Rust (
Confidence Score: 5/5Safe to merge — both engines receive an identical, minimal extension that reuses well-tested helpers, with no changes to existing code paths. The change is a two-line addition in each engine that delegates entirely to the already-tested No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tree-sitter node: external_function] --> B{has 'name' field?}
B -- No --> C[return / skip]
B -- Yes --> D[extract_params / extractParams]
D --> E{find 'parameters' field or 'function_parameters' child}
E -- Not found --> F[empty Vec / empty array]
E -- Found --> G[iterate parameter children]
G --> H{param kind: function_parameter or parameter}
H -- Yes --> I{has 'name' field or identifier child?}
I -- Yes --> J[push SubDeclaration kind='parameter']
I -- No type-only --> K[skip]
H -- identifier --> J
H -- other --> K
J --> L[opt_children / conditional children]
F --> L
K --> L
L --> M[push Definition kind='function' children: params or None/undefined]
Reviews (7): 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.
…al-function-params
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