Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR expands the public API surface by exposing additional checker endpoints (types/signatures utilities) through the Go session layer and the TypeScript client libraries.
Changes:
- Added new API methods/endpoints: resolved signature, non-nullable type, type-from-type-node, widened type, parameter type, array-like checks, and signature declaration printing.
- Exposed
NodeBuilderFlagsin the TypeScript API to support node/signature printing flags. - Added Go checker wrappers for newly exposed functionality.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/checker/printer.go | Exposes SignatureToSignatureDeclaration via Checker. |
| internal/checker/exports.go | Exposes widened-type functionality via Checker. |
| internal/api/session.go | Routes and implements new API handlers for the added endpoints. |
| internal/api/proto.go | Adds method constants, unmarshalers, and request parameter types for new endpoints. |
| _packages/api/src/sync/api.ts | Adds sync client methods for new endpoints and exports NodeBuilderFlags. |
| _packages/api/src/async/api.ts | Adds async client methods for new endpoints and exports NodeBuilderFlags. |
| _packages/api/src/enums/nodeBuilderFlags.ts | Adds generated runtime NodeBuilderFlags enum values. |
| _packages/api/src/enums/nodeBuilderFlags.enum.ts | Adds generated TypeScript NodeBuilderFlags type enum. |
| // handleGetParameterType returns the type of a parameter at a given index in a signature. | ||
| func (s *Session) handleGetParameterType(ctx context.Context, params *GetParameterTypeParams) (*TypeResponse, error) { | ||
| setup, err := s.setupChecker(ctx, params.Snapshot, params.Project) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer setup.done() | ||
|
|
||
| sig, err := setup.sd.resolveSignatureHandle(params.Signature) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| t := setup.checker.GetTypeParameterAtPosition(sig, int(params.Index)) | ||
| if t == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| return setup.sd.registerType(t), nil | ||
| } |
There was a problem hiding this comment.
This handler is documented and named to return a parameter type, but it calls GetTypeParameterAtPosition, which typically refers to a generic type parameter (e.g., T) rather than a function parameter’s type. Either switch to the checker API that returns the parameter type at position (commonly GetTypeAtPosition(signature, index)), or rename the endpoint/params/method to getTypeParameterAtPosition to match the implemented behavior.
There was a problem hiding this comment.
Name is same as that in Strada. The function returns the type of parameter as an index and not a generic type parameter.
There was a problem hiding this comment.
No, Copilot is right about this. The function you wrote here returns the type of a generic type parameter. The checker function you need to use is called getTypeAtPosition and isn't exported yet.
| sig, err := setup.sd.resolveSignatureHandle(params.Signature) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
params.Index is int32 and is directly cast to int without validation. Negative values (or out-of-range values) should be rejected with a clear error to avoid undefined behavior or downstream panics inside the checker. Consider validating params.Index >= 0 (and, if feasible, bounds-checking against the signature’s parameter count) and returning an error like invalid parameter index.
| } | |
| } | |
| if params.Index < 0 { | |
| return nil, fmt.Errorf("invalid parameter index") | |
| } |
| signatureToSignatureDeclaration(signature: Signature, kind: number, enclosingDeclaration?: Node, flags?: number): Node | undefined { | ||
| const binaryData = this.client.apiRequestBinary("signatureToSignatureDeclaration", { | ||
| snapshot: this.snapshotId, | ||
| project: this.projectId, | ||
| signature: signature.id, | ||
| kind, | ||
| location: enclosingDeclaration ? getNodeId(enclosingDeclaration) : undefined, | ||
| flags, | ||
| }); |
There was a problem hiding this comment.
The TS client method is typed with kind: number and flags?: number, but this endpoint is semantically tied to specific enums (AST Kind/SyntaxKind for kind, and NodeBuilderFlags for flags). Since NodeBuilderFlags is already added to exports, update the method signature to use flags?: NodeBuilderFlags (and a stronger type for kind if available) to prevent accidental invalid values and improve IntelliSense.
| async signatureToSignatureDeclaration(signature: Signature, kind: number, enclosingDeclaration?: Node, flags?: number): Promise<Node | undefined> { | ||
| const binaryData = await this.client.apiRequestBinary("signatureToSignatureDeclaration", { | ||
| snapshot: this.snapshotId, | ||
| project: this.projectId, | ||
| signature: signature.id, | ||
| kind, | ||
| location: enclosingDeclaration ? getNodeId(enclosingDeclaration) : undefined, | ||
| flags, | ||
| }); |
There was a problem hiding this comment.
Same typing issue as the sync client: kind/flags are exposed as plain numbers. Prefer flags?: NodeBuilderFlags (and a stronger enum type for kind if available) so callers don’t accidentally pass unrelated constants.
| // GetNonNullableTypeParams returns the type with null and undefined removed. | ||
| type GetNonNullableTypeParams struct { | ||
| Snapshot Handle[project.Snapshot] `json:"snapshot"` | ||
| Project Handle[project.Project] `json:"project"` | ||
| Type Handle[checker.Type] `json:"type"` | ||
| } | ||
|
|
||
| // GetTypeFromTypeNodeParams returns the type for a type node. | ||
| type GetTypeFromTypeNodeParams struct { | ||
| Snapshot Handle[project.Snapshot] `json:"snapshot"` | ||
| Project Handle[project.Project] `json:"project"` | ||
| Location Handle[ast.Node] `json:"location"` | ||
| } | ||
|
|
||
| // GetWidenedTypeParams returns the widened type of a type. |
There was a problem hiding this comment.
These doc comments describe the result of the operation rather than what the params struct represents (e.g., “returns …”). For clarity and consistency, reword to something like: // GetNonNullableTypeParams are the parameters for the getNonNullableType method. (Similarly for GetTypeFromTypeNodeParams, GetWidenedTypeParams, and GetParameterTypeParams.)
| // GetNonNullableTypeParams returns the type with null and undefined removed. | |
| type GetNonNullableTypeParams struct { | |
| Snapshot Handle[project.Snapshot] `json:"snapshot"` | |
| Project Handle[project.Project] `json:"project"` | |
| Type Handle[checker.Type] `json:"type"` | |
| } | |
| // GetTypeFromTypeNodeParams returns the type for a type node. | |
| type GetTypeFromTypeNodeParams struct { | |
| Snapshot Handle[project.Snapshot] `json:"snapshot"` | |
| Project Handle[project.Project] `json:"project"` | |
| Location Handle[ast.Node] `json:"location"` | |
| } | |
| // GetWidenedTypeParams returns the widened type of a type. | |
| // GetNonNullableTypeParams are the parameters for the getNonNullableType method. | |
| type GetNonNullableTypeParams struct { | |
| Snapshot Handle[project.Snapshot] `json:"snapshot"` | |
| Project Handle[project.Project] `json:"project"` | |
| Type Handle[checker.Type] `json:"type"` | |
| } | |
| // GetTypeFromTypeNodeParams are the parameters for the getTypeFromTypeNode method. | |
| type GetTypeFromTypeNodeParams struct { | |
| Snapshot Handle[project.Snapshot] `json:"snapshot"` | |
| Project Handle[project.Project] `json:"project"` | |
| Location Handle[ast.Node] `json:"location"` | |
| } | |
| // GetWidenedTypeParams are the parameters for the getWidenedType method. |
No description provided.