Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces TypeScript generic type-parameter emission for Python generic constructs (TypeVar, ParamSpec, TypeVarTuple, Unpack). It extends the IR to track type-parameter metadata, updates annotation parsing to recognize generic constructs, and modifies code generation to emit TypeScript generic declarations while safely falling back to Changes
Sequence Diagram(s)sequenceDiagram
participant Extractor as IR Extractor<br/>(Python)
participant Parser as Annotation Parser<br/>(TypeScript)
participant Generator as Code Generator<br/>(TypeScript)
participant Mapper as TypeMapper<br/>(TypeScript)
participant Output as Generated<br/>Output
Extractor->>Extractor: Extract type_params from<br/>functions/classes/aliases
Extractor->>Extractor: Discover TypeVar,<br/>ParamSpec, TypeVarTuple
Extractor->>Output: Emit IR 0.2.0 with<br/>type_params metadata
Parser->>Parser: Build knownTypeParameters<br/>lookup from options
Parser->>Parser: Parse annotation,<br/>consult generic registry
Parser->>Parser: Convert generic references<br/>to PythonType nodes
Parser->>Generator: Return annotated types
Generator->>Generator: Compute per-function<br/>genericContext
Generator->>Generator: Select which generics<br/>are safe to emit
Generator->>Mapper: Request TypeScript types<br/>for sanitized Python types
Mapper->>Mapper: Map paramspec →<br/>TSCustomType
Mapper->>Mapper: Map generic constructs<br/>via mapKnownGenericType
Mapper->>Mapper: Return mapped TS type
Mapper->>Generator: TS type representation
Generator->>Generator: Emit export function<br/><T>(...) signature
Generator->>Generator: Render overloads &<br/>declarations
Generator->>Output: Generate .ts & .d.ts<br/>with generic parameters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Please run a full review on this PR. |
|
✅ Actions performedFull review triggered. |
coderabbitai
Bot
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 180263c403
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| returns = hints.get("return", sig.return_annotation) | ||
| is_async = inspect.iscoroutinefunction(obj) or inspect.isasyncgenfunction(obj) | ||
| is_generator = inspect.isgeneratorfunction(obj) | ||
| type_params = _collect_type_params_from_annotations(*annotations_for_params, returns) if include_type_params else [] |
There was a problem hiding this comment.
On Python 3.12, generic functions declared with PEP 695 syntax (for example def identity[T](x: T) -> T) expose their type parameters on __type_params__, not in the annotations list. _extract_function currently derives type_params only from parameter/return annotations, so these functions end up with type_params=[]; downstream parsing then treats T as an undeclared custom type and generates wrappers like identity(x: T): Promise<T>, which do not type-check.
Useful? React with 👍 / 👎.
| type_params = _collect_type_params_from_annotations(value) | ||
| definition = _stringify_annotation(value) or annotation_str or str(value) |
There was a problem hiding this comment.
For Python 3.12 type aliases, value is a typing.TypeAliasType and str(value) is just the alias name. In _extract_type_aliases, that means type Pair[T] = tuple[T, T] is serialized as definition='Pair' with no type_params, so the generator later emits export type Pair = Pair, which TypeScript rejects as a circular alias. This needs to read the alias payload/type params from the TypeAliasType wrapper instead of stringifying the wrapper itself.
Useful? React with 👍 / 👎.
| if (param.kind === 'paramspec' && callableParamSpecs.has(param.name)) { | ||
| emitted.push({ name: param.name, declaration: `${param.name} extends unknown[]` }); | ||
| emittedNames.add(param.name); |
There was a problem hiding this comment.
This only emits a ParamSpec if it was seen inside a raw Callable[P, ...] node. When a public API uses a generic alias such as Transform[P, T] = Callable[P, T], P appears only as a generic type argument, so it is dropped during sanitization and the generated wrapper ends up using Transform<unknown, T>. That violates the alias’s P extends unknown[] constraint and makes the generated .ts/.d.ts fail to type-check.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/type-mapping-matrix.md`:
- Line 378: Update the footer date string "Last updated: January 2026." to the
current modification date (e.g., "Last updated: March 2026") in the
docs/type-mapping-matrix.md content so the document reflects the actual last
update; locate and replace the exact string "Last updated: January 2026." with
the correct month and year.
In `@test/generator.test.ts`:
- Around line 900-903: The `not.toContain('Ts')` assertion is too broad and can
false-positive when "Ts" appears inside other identifiers; update that assertion
in test/generator.test.ts to a more specific check targeting the actual type
token (e.g., assert absence of the token or generic usage like "Ts<" or use a
word-boundary regex such as /\bTs\b/ via expect(...).not.toMatch) so you only
fail when the standalone `Ts` type appears rather than any substring of other
identifiers.
In `@test/integration.test.ts`:
- Around line 163-175: The runtime-stub.d.ts used in the test is intentionally
minimal and currently omits RuntimeBridge.dispose(); update the string written
in the writeFile call (the template that defines RuntimeBridge and
getRuntimeBridge) to include a short comment above the interface explaining that
this stub only contains the methods used by generated wrappers and that the real
RuntimeExecution includes additional methods (e.g., dispose) which the stub
intentionally omits; mention that if future generated code calls other bridge
methods the stub must be expanded.
In `@tywrap_ir/tywrap_ir/ir.py`:
- Line 199: The expression computing paramspec_match uses two separate endswith
calls; replace the boolean OR pattern with a single endswith call that accepts a
tuple so it reads like text.endswith((".args", ".kwargs")) while preserving the
existing ternary logic that returns text.split(".", 1)[0] when matched and None
otherwise (i.e., update the paramspec_match assignment to use the tuple-based
endswith).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36ebfc4e-5209-4c36-acf6-a0215bcfcd74
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
docs/type-mapping-matrix.mdsrc/core/annotation-parser.tssrc/core/generator.tssrc/core/mapper.tssrc/index.tssrc/types/index.tssrc/tywrap.tssrc/utils/ir-cache.tstest/generator.test.tstest/integration.test.tstest/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstywrap_ir/tywrap_ir/__main__.pytywrap_ir/tywrap_ir/ir.pytywrap_ir/tywrap_ir/optimized_ir.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
test/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
NODE_OPTIONS=--expose-gcenvironment variable for GC-sensitive tests
Files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode; avoid
any— useunknownand type guards instead
Files:
src/utils/ir-cache.tssrc/index.tssrc/tywrap.tssrc/core/mapper.tssrc/core/generator.tssrc/core/annotation-parser.tssrc/types/index.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and avoid
any— useunknownand type guards instead
Files:
src/utils/ir-cache.tssrc/index.tssrc/tywrap.tssrc/core/mapper.tssrc/core/generator.tssrc/core/annotation-parser.tssrc/types/index.ts
tywrap_ir/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must follow PEP 8 and use type hints
Follow PEP 8 and use type hints in Python code
Files:
tywrap_ir/tywrap_ir/__main__.pytywrap_ir/tywrap_ir/optimized_ir.pytywrap_ir/tywrap_ir/ir.py
🧠 Learnings (17)
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to test/runtime_*.test.ts : Include tests for new runtime behavior following the `test/runtime_*.test.ts` patterns
Applied to files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to test/runtime_*.test.ts : Include tests for new runtime behavior following patterns in `test/runtime_*.test.ts`
Applied to files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/ir_cache_filename.test.tssrc/utils/ir-cache.tstest/ir_extraction_advanced.test.tstest/generator.test.tssrc/index.tssrc/tywrap.tstest/integration.test.tssrc/core/mapper.tssrc/core/generator.tssrc/core/annotation-parser.tssrc/types/index.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/ir_cache_filename.test.tssrc/utils/ir-cache.tstest/ir_extraction_advanced.test.tstest/generator.test.tssrc/index.tssrc/tywrap.tstest/integration.test.tssrc/core/mapper.tssrc/core/generator.tssrc/core/annotation-parser.tssrc/types/index.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/ir_cache_filename.test.tssrc/utils/ir-cache.tstest/ir_extraction_advanced.test.tstest/generator.test.tssrc/index.tssrc/tywrap.tstest/integration.test.tssrc/core/mapper.tssrc/core/generator.tssrc/core/annotation-parser.tssrc/types/index.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to tywrap_ir/**/*.py : Follow PEP 8 and use type hints in Python code
Applied to files:
src/tywrap.tstywrap_ir/tywrap_ir/ir.py
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to tywrap_ir/**/*.py : Python code must follow PEP 8 and use type hints
Applied to files:
src/tywrap.tstywrap_ir/tywrap_ir/ir.py
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
src/tywrap.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to src/**/*.ts : Use TypeScript strict mode and avoid `any` — use `unknown` and type guards instead
Applied to files:
docs/type-mapping-matrix.md
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to src/**/*.{ts,tsx} : TypeScript strict mode; avoid `any` — use `unknown` and type guards instead
Applied to files:
docs/type-mapping-matrix.md
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to src/runtime/**/*.ts : Runtime bridges code should be located in `src/runtime/` with corresponding tests in `test/runtime_*.test.ts`
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Place runtime bridge implementations in `src/runtime/` with corresponding tests in `test/runtime_*.test.ts`
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/integration.test.ts
🪛 Ruff (0.15.6)
tywrap_ir/tywrap_ir/ir.py
[warning] 13-13: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 13-13: typing.List is deprecated, use list instead
(UP035)
[warning] 17-17: Do not catch blind exception: Exception
(BLE001)
[warning] 121-121: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 130-130: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 166-166: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 178-178: Dynamically typed expressions (typing.Any) are disallowed in annotation
(ANN401)
[warning] 186-186: Do not catch blind exception: Exception
(BLE001)
[warning] 193-193: Do not catch blind exception: Exception
(BLE001)
[warning] 199-199: Call endswith once with a tuple
Merge into a single endswith call
(PIE810)
[warning] 208-208: Dynamically typed expressions (typing.Any) are disallowed in *annotations
(ANN401)
[warning] 216-216: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 219-219: Do not catch blind exception: Exception
(BLE001)
[warning] 224-224: Do not catch blind exception: Exception
(BLE001)
[warning] 236-236: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 243-243: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[error] 254-255: try-except-pass detected, consider logging the exception
(S110)
[warning] 254-254: Do not catch blind exception: Exception
(BLE001)
[warning] 259-259: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 259-259: Unused function argument: module_name
(ARG001)
[warning] 259-259: Boolean-typed positional argument in function definition
(FBT001)
[error] 270-271: try-except-continue detected, consider logging the exception
(S112)
[warning] 270-270: Do not catch blind exception: Exception
(BLE001)
[warning] 288-288: Do not catch blind exception: Exception
(BLE001)
[warning] 303-303: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 303-303: Unused function argument: module_name
(ARG001)
[warning] 303-303: Boolean-typed positional argument in function definition
(FBT001)
[error] 316-317: try-except-continue detected, consider logging the exception
(S112)
[warning] 316-316: Do not catch blind exception: Exception
(BLE001)
[warning] 340-340: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[warning] 347-347: Do not catch blind exception: Exception
(BLE001)
[warning] 352-352: Do not catch blind exception: Exception
(BLE001)
[warning] 355-355: Do not catch blind exception: Exception
(BLE001)
[error] 532-533: try-except-pass detected, consider logging the exception
(S110)
[warning] 532-532: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (32)
test/ir_cache_filename.test.ts (1)
24-44: LGTM!The new test case correctly validates that the cache filename changes when only
irVersiondiffers. The use of a sharedkeyBaseobject keeps the test focused and readable.src/utils/ir-cache.ts (1)
3-22: LGTM!Adding
irVersionto the cache key interface ensures cache invalidation when the IR schema changes. This prevents stale cached JSON from being used after schema updates.tywrap_ir/tywrap_ir/__main__.py (1)
13-13: LGTM!Default IR version bumped to
0.2.0to align with the schema changes for generic type parameters.test/ir_extraction_advanced.test.ts (1)
178-250: LGTM!Comprehensive test coverage for the new
type_paramsIR field. The test verifies:
- Function-level generic parameters (
identitywithT)- Class-level generic parameters (
Container,KeyValueStore)- Type alias generic parameters with different kinds (
typevar,paramspec,typevartuple)The
summarizeParamshelper keeps assertions focused on the essential fields.docs/type-mapping-matrix.md (1)
120-151: LGTM!Clear documentation of the new generic type emission behavior, including:
- Declaration-level generic parameters for
TypeVar- Conservative fallback for bounds/constraints
ParamSpechandling withP extends unknown[]- Safe fallbacks for
TypeVarTuple/Unpacktounknowntest/generator.test.ts (1)
632-906: LGTM!Thorough test coverage for generic emission:
- Function overloads with generic parameters
- Generic class declarations with
static create<T>andstatic fromHandle<T>- Type alias emission including
ParamSpecmodeling asP extends unknown[]- Verification that unsafe placeholders are not emitted in output
src/index.ts (1)
86-133: LGTM!The public type exports are expanded to include the new generic-related IR types. This allows consumers to work with the full type schema when needed.
test/integration.test.ts (1)
83-238: LGTM!Excellent end-to-end integration test that:
- Generates TypeScript from a Python module with full generic coverage (
TypeVar,ParamSpec,TypeVarTuple)- Verifies generated
.tsand.d.tscontent includes expected generic signatures- Asserts absence of unsafe placeholder patterns (
~T,~P,Unpack[)- Validates that the output compiles with
tsc --strictThe use of a consumer file that actually instantiates and uses the generic types ensures the emitted TypeScript is practically usable, not just syntactically valid.
tywrap_ir/tywrap_ir/optimized_ir.py (1)
161-161: LGTM! IR version bump is consistent.The default
ir_versionis correctly updated to"0.2.0"in both the class method and module-level function signatures, aligning with the version bump inir.pyand the CLI.Also applies to: 409-409
src/tywrap.ts (3)
25-26: LGTM! IR version constant centralized.Good practice to define
TYWRAP_IR_VERSIONas a constant and use it consistently in CLI invocations and cache key computation.
428-499: Well-structured generic parameter extraction.The
mapTypeParameters,mapParam, andmapFunchelpers cleanly propagate type parameters from IR to the TypeScript model. The pattern of passingtypeParameterscontext through the parsing pipeline ensures annotations are resolved correctly against their declaring scope.
541-559: Type alias extraction correctly implemented.The
mapTypeAliasfunction properly extracts generic parameters and parses the definition with the correct type parameter context, andtypeAliasesis correctly populated from the IR.src/core/mapper.ts (3)
71-80: LGTM! Exhaustive handling of new Python type kinds.The switch statement now handles all new type variants (
paramspec,paramspec_args,paramspec_kwargs,typevartuple,unpack) with appropriate delegations to dedicated mapping methods.
346-378: Conservative fallback strategy correctly implemented.The new mapping helpers follow the documented approach:
mapParamSpecTypereturns a custom type placeholdermapParamSpecArgsTypefalls back tounknown[]mapParamSpecKwargsTypefalls back toRecord<string, unknown>mapTypeVarTupleTypeandmapUnpackTypefall back tounknownThis ensures generated TypeScript remains valid even when Python typing constructs cannot be fully represented.
291-327: Callable mapping correctly handles ParamSpec.The refactored
mapCallableTypeproperly:
- Uses
mapParamSpecTypewhen aparameterSpecis present- Preserves the ellipsis special case for
Callable[..., R]- Falls back to explicit parameter mapping otherwise
tywrap_ir/tywrap_ir/ir.py (4)
21-28: LGTM! IRTypeParam dataclass well-designed.The
IRTypeParamdataclass captures all necessary metadata for Python generic parameters: name, kind (typevar/paramspec/typevartuple), optional bound, optional constraints list, and optional variance. This enables the TypeScript side to make informed emission decisions.
121-174: Type parameter serialization correctly implemented.The helper functions properly:
- Detect TypeVar/ParamSpec/TypeVarTuple via module and class name inspection
- Extract bounds, constraints, and variance from TypeVar instances
- Strip the
~prefix from names for normalization
339-386: Function extraction correctly collects type parameters.The
include_type_paramsflag allows method extraction within classes to skip redundant type parameter collection (since class methods inherit the class's type parameters). The use ofget_type_hints(..., include_extras=True)preservesAnnotatedmetadata.
580-580: IR version bump to 0.2.0 is consistent.The default
ir_versionis correctly updated in bothextract_module_irandemit_ir_json, matching the changes inoptimized_ir.pyand the TypeScript side.Also applies to: 624-624
src/core/generator.ts (5)
19-30: LGTM! Generic rendering context well-designed.
GenericRenderContextcleanly tracks:
currentModulefor same-module reference normalizationdeclarationandtypeArgumentsstrings for emissionemittedNamesandemittedParamSpecssets for safe type variable resolutionThis enables the generator to emit only declared generics and fall back to
unknownfor undeclared ones.
134-173: Generic emission rules correctly implemented.
buildGenericRenderContextfollows the documented emission rules:
- Simple unconstrained TypeVars (no bound, no constraints, invariant) → emit as-is
- ParamSpecs used in callable positions → emit as
P extends unknown[]- Bounded/constrained TypeVars, TypeVarTuple → not emitted (will fall back to
unknown)
211-281: Safe type sanitization ensures valid TypeScript output.
sanitizeTypecorrectly:
- Passes through primitives, literals, and custom types
- Recursively sanitizes nested types (collections, unions, generics, callables)
- Replaces unemitted
typevar/paramspecwithAny(→unknownin TS)- Falls back
paramspec_argstolist[Any],paramspec_kwargstodict[str, Any]- Falls back
typevartupleandunpacktoAnyThis guarantees generated TypeScript never references undeclared type parameters.
1085-1095: Type alias generation correctly implemented.
generateTypeAliasproperly:
- Builds a generic context from the alias's type parameters
- Preserves the original casing via
preserveCase: true- Emits
export type Alias<T> = ...with sanitized body
1173-1185:wrapmethod signature updated for declaration output.The method now accepts and returns the
declarationstring, enabling proper.d.tsfile generation separate from runtime code.src/core/annotation-parser.ts (5)
1-5: LGTM! Parser options extended for type parameter context.The new
typeParametersoption allows the parser to resolve type variable references against their declaring scope, enabling correct emission oftypevar,paramspec, andtypevartuplenodes.
45-65: Type parameter resolution correctly implemented.
mapKnownTypeParameter:
- Normalizes names by stripping
~prefix- Returns the appropriate
PythonTypevariant based on parameter kind- Preserves bound/constraints/variance for TypeVars
213-243: Balanced bracket parsing ensures correct generic extraction.
splitGenericInvocationcorrectly:
- Validates the string ends with
]- Uses depth tracking to find the matching
[- Rejects malformed cases (unbalanced brackets, extra content after
])
315-357: Callable parsing handles all parameter forms.The refactored callable parsing correctly handles:
- Ellipsis (
...) → single{ kind: 'custom', name: '...' }parameter- ParamSpec reference → sets
parameterSpecfield- Explicit parameter list → parses individual types
381-403: Unpack and generic invocation support added.
Unpack[...]correctly emits{ kind: 'unpack', type: ... }- Generic invocations like
Foo[T, U]are parsed with qualified module extractionsrc/types/index.ts (3)
39-53: LGTM! Generic parameter types well-defined.
PythonTypeAliasandPythonGenericParameterproperly model:
- Type alias with optional generic parameters
- Generic parameter kind (typevar/paramspec/typevartuple)
- Optional bound, constraints, and variance for TypeVars
101-105: New PythonType variants complete the type union.The five new variants (
ParamSpecType,ParamSpecArgsType,ParamSpecKwargsType,TypeVarTupleType,UnpackType) are:
- Correctly added to the
PythonTypeunion- Consistently structured with
kinddiscriminators- Properly handled in mapper.ts and generator.ts switch statements
Also applies to: 169-192
133-133: Extended type interfaces support qualified generics and ParamSpec callables.
GenericType.moduleenables same-module reference normalizationCallableType.parameterSpecenablesCallable[P, R]representationAlso applies to: 146-146
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/annotation-parser.ts`:
- Around line 257-265: The code incorrectly treats any X.args / X.kwargs as
ParamSpec packs because of the `known === null` branch; narrow the fast path by
removing the `|| known === null` condition so only when
`mapKnownTypeParameter(baseName)` returns a known paramspec (`known?.kind ===
'paramspec'`) you return the `paramspec_args` / `paramspec_kwargs` variants;
update the logic around `paramspecArgsMatch` handling (and its returned
`PythonType` shapes) to avoid rewriting module-qualified names like
`pkg.Request.args` into ParamSpec packs.
In `@src/core/mapper.ts`:
- Around line 186-191: mapGenericType currently emits raw generic names (e.g.,
Sequence, Awaitable) and loses prior typing-to-TS conversions; update
mapGenericType to call normalizeCustomType or reuse mapCustomType on the
generic's base identifier so known typing conversions are preserved.
Specifically, inside mapGenericType (and when computing normalized from { name:
type.name, module: type.module }), ensure you use the normalized result's name
(and any mapped module/alias) for the returned TSGenericType.name rather than
the raw type.name, and keep mapping typeArgs via this.mapPythonType as before so
typing.Sequence[T] and typing.Awaitable[T] become the previously-mapped
TypeScript identifiers instead of undeclared bare names.
In `@test/ir_extraction_advanced.test.ts`:
- Around line 179-216: The test uses TypeVarTuple/Unpack which require Python
3.11+, so add a pre-check before calling createTempModule in the 'extracts
ordered type parameters for functions, classes, and type aliases' test: run a
short Python command (e.g., via child_process.execSync) or inspect
sys.version_info to detect if the interpreter is < 3.11 and if so call the test
skip/early return; update the test surrounding extractIR, createTempModule, or
the it(...) block to bail out when Python < 3.11 so the test only runs when
TypeVarTuple/Unpack are available. Ensure you reference the same test name and
keep createTempModule and extractIR calls unchanged when the guard passes.
In `@tywrap_ir/tywrap_ir/ir.py`:
- Around line 121-127: The function _type_param_kind currently only recognizes
TypeVar/ParamSpec/TypeVarTuple when their class __module__ is "typing"; update
its module check to also accept "typing_extensions" (or otherwise treat the name
match independent of module) so that backported generic markers from
typing_extensions are recognized; modify the conditional in _type_param_kind to
consider module in {"typing", "typing_extensions"} (or simply match the __name__
set {"TypeVar", "ParamSpec", "TypeVarTuple"} regardless of module) so type
parameters are detected and emitted correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03d4cd4d-f225-4cac-9962-0810bb035292
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
docs/type-mapping-matrix.mdsrc/core/annotation-parser.tssrc/core/generator.tssrc/core/mapper.tssrc/index.tssrc/types/index.tssrc/tywrap.tssrc/utils/ir-cache.tstest/generator.test.tstest/integration.test.tstest/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstywrap_ir/tywrap_ir/__main__.pytywrap_ir/tywrap_ir/ir.pytywrap_ir/tywrap_ir/optimized_ir.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode; avoid
any— useunknownand type guards instead
Files:
src/utils/ir-cache.tssrc/index.tssrc/tywrap.tssrc/core/generator.tssrc/core/mapper.tssrc/core/annotation-parser.tssrc/types/index.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and avoid
any— useunknownand type guards instead
Files:
src/utils/ir-cache.tssrc/index.tssrc/tywrap.tssrc/core/generator.tssrc/core/mapper.tssrc/core/annotation-parser.tssrc/types/index.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
NODE_OPTIONS=--expose-gcenvironment variable for GC-sensitive tests
Files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
tywrap_ir/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must follow PEP 8 and use type hints
Follow PEP 8 and use type hints in Python code
Files:
tywrap_ir/tywrap_ir/__main__.pytywrap_ir/tywrap_ir/optimized_ir.pytywrap_ir/tywrap_ir/ir.py
🧠 Learnings (18)
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
src/utils/ir-cache.tstest/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tssrc/index.tstest/generator.test.tstest/integration.test.tssrc/tywrap.tssrc/core/generator.tssrc/core/mapper.tssrc/core/annotation-parser.tssrc/types/index.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
src/utils/ir-cache.tstest/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tssrc/index.tstest/generator.test.tstest/integration.test.tssrc/tywrap.tssrc/core/generator.tssrc/core/mapper.tssrc/core/annotation-parser.tssrc/types/index.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
src/utils/ir-cache.tstest/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tssrc/index.tstest/generator.test.tstest/integration.test.tssrc/tywrap.tssrc/core/generator.tssrc/core/mapper.tssrc/core/annotation-parser.tssrc/types/index.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to test/runtime_*.test.ts : Include tests for new runtime behavior following the `test/runtime_*.test.ts` patterns
Applied to files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to test/runtime_*.test.ts : Include tests for new runtime behavior following patterns in `test/runtime_*.test.ts`
Applied to files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/ir_cache_filename.test.tstest/ir_extraction_advanced.test.tstest/generator.test.tstest/integration.test.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to tywrap_ir/**/*.py : Python code must follow PEP 8 and use type hints
Applied to files:
tywrap_ir/tywrap_ir/__main__.pysrc/tywrap.tstywrap_ir/tywrap_ir/ir.py
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to tywrap_ir/**/*.py : Follow PEP 8 and use type hints in Python code
Applied to files:
tywrap_ir/tywrap_ir/__main__.pydocs/type-mapping-matrix.mdsrc/tywrap.tstywrap_ir/tywrap_ir/ir.py
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to src/**/*.ts : Use TypeScript strict mode and avoid `any` — use `unknown` and type guards instead
Applied to files:
docs/type-mapping-matrix.mdtest/generator.test.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to src/**/*.{ts,tsx} : TypeScript strict mode; avoid `any` — use `unknown` and type guards instead
Applied to files:
docs/type-mapping-matrix.mdtest/generator.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/generator.test.tstest/integration.test.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Place runtime bridge implementations in `src/runtime/` with corresponding tests in `test/runtime_*.test.ts`
Applied to files:
test/integration.test.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to src/runtime/**/*.ts : Runtime bridges code should be located in `src/runtime/` with corresponding tests in `test/runtime_*.test.ts`
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-20T16:01:39.136Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
src/tywrap.ts
🪛 Ruff (0.15.6)
tywrap_ir/tywrap_ir/ir.py
[warning] 13-13: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 13-13: typing.List is deprecated, use list instead
(UP035)
[warning] 17-17: Do not catch blind exception: Exception
(BLE001)
[warning] 121-121: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 130-130: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 166-166: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 178-178: Dynamically typed expressions (typing.Any) are disallowed in annotation
(ANN401)
[warning] 186-186: Do not catch blind exception: Exception
(BLE001)
[warning] 193-193: Do not catch blind exception: Exception
(BLE001)
[warning] 199-199: Call endswith once with a tuple
Merge into a single endswith call
(PIE810)
[warning] 208-208: Dynamically typed expressions (typing.Any) are disallowed in *annotations
(ANN401)
[warning] 216-216: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 219-219: Do not catch blind exception: Exception
(BLE001)
[warning] 224-224: Do not catch blind exception: Exception
(BLE001)
[warning] 236-236: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 243-243: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[error] 254-255: try-except-pass detected, consider logging the exception
(S110)
[warning] 254-254: Do not catch blind exception: Exception
(BLE001)
[warning] 259-259: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 259-259: Unused function argument: module_name
(ARG001)
[warning] 259-259: Boolean-typed positional argument in function definition
(FBT001)
[error] 270-271: try-except-continue detected, consider logging the exception
(S112)
[warning] 270-270: Do not catch blind exception: Exception
(BLE001)
[warning] 288-288: Do not catch blind exception: Exception
(BLE001)
[warning] 303-303: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 303-303: Unused function argument: module_name
(ARG001)
[warning] 303-303: Boolean-typed positional argument in function definition
(FBT001)
[error] 316-317: try-except-continue detected, consider logging the exception
(S112)
[warning] 316-316: Do not catch blind exception: Exception
(BLE001)
[warning] 340-340: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[warning] 347-347: Do not catch blind exception: Exception
(BLE001)
[warning] 352-352: Do not catch blind exception: Exception
(BLE001)
[warning] 355-355: Do not catch blind exception: Exception
(BLE001)
[error] 532-533: try-except-pass detected, consider logging the exception
(S110)
[warning] 532-532: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
test/integration.test.ts (1)
92-96: Verify the fixture’s Python-version floor.This inline module now requires the newer stdlib
typingexports:ParamSpecfrom Python 3.10+, andTypeVarTuple/Unpackfrom Python 3.11+. If the integration suite still runs on older interpreters, the test will fail beforegenerate()is exercised.♻️ Portable fixture import
-from typing import Callable, Generic, ParamSpec, TypeVar, TypeVarTuple, Unpack +from typing import Callable, Generic, TypeVar +try: + from typing import ParamSpec, TypeVarTuple, Unpack +except ImportError: + from typing_extensions import ParamSpec, TypeVarTuple, Unpack
bbopen
force-pushed
the
codex/safe-typescript-generic-emission
branch
from
c77fff9 to
2538f51
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/type-mapping-matrix.md`:
- Around line 122-126: The table entry for TypeVar is misleading: update the
rows for `TypeVar('T')`, `TypeVar('T', bound=Base)`, and `TypeVar('T', str,
int)` to explicitly state that only invariant TypeVars are emitted as
declaration-level generics and that covariant/contravariant TypeVars (variance)
as well as TypeVars with bounds or constraints currently fall back to `unknown`;
revise the `TypeVar('T')` row to note "invariant only" and add a brief note to
the other two rows that they cover both bounds/constraints and variance
fallbacks so the table no longer over-promises.
In `@src/core/generator.ts`:
- Around line 146-170: The emitted generic identifiers are inserted raw causing
invalid TS for names like "default" — update the code paths that build the
emitted array and emittedNames/emittedParamSpecs to use
this.escapeIdentifier(param.name) instead of param.name: when pushing into
emitted use declaration: `${this.escapeIdentifier(param.name)}${...}` (or
`${this.escapeIdentifier(param.name)} extends unknown[]` for paramspecs) and set
the pushed object's name to the escaped identifier; also add the escaped
identifier to emittedNames and emittedParamSpecs so later sanitizeType checks
match; apply the same replacement wherever emitted/declaration/typeArguments are
composed (including other spots that build emitted.map(param => param.name) and
emitted.map(param => param.declaration)) and ensure all uses of param.name in
these generators are escaped via this.escapeIdentifier.
- Around line 175-185: mergeGenericRenderContexts currently unions emittedNames
across scopes which loses shadowing info and allows inner type parameters to be
resolved to outer names; update mergeGenericRenderContexts to preserve scope
precedence instead of a flat Set (e.g., make emittedNames a stack/array of Sets
or a structure that records origin order so lookups consult inner first), keep
declaration and typeArguments as-is, and adapt sanitizeType (the
ctx.emittedNames.has(...) check) to consult the new precedence-aware
emittedNames structure so inner/declared names shadow outer ones; refer to
mergeGenericRenderContexts, emittedNames, and sanitizeType when making the
change.
In `@test/integration.test.ts`:
- Around line 225-277: The tsconfig written in the test includes the generated
implementation .ts files which causes TypeScript to bind imports to the .ts
sources instead of the .d.ts declarations; update the tsconfig JSON written in
the test (the object passed to writeFile for tsconfig.json under compileDir) to
remove '../generated/generic_module.generated.ts' (and similarly remove
'../generated/protocol_module.generated.ts' in the other tsconfig write) from
the "include" array so the test compiles only against the .d.ts files and
validates the declaration surface.
In `@test/type_mapping_enhanced.test.ts`:
- Around line 232-370: Add a direct conversion in mapPythonType for generic
types named "Coroutine" coming from module "typing": when encountering a
PythonType with kind 'generic', name 'Coroutine', and module 'typing', map it to
a TS generic Promise using the third type argument as the promise value type
(i.e., Promise<T>), making sure to map that T via the existing mapper logic
(call mapper.mapPythonType on typeArgs[2]) and handle the case where fewer than
3 typeArgs exist gracefully (fallback to Promise<any> or similar). Update the
branch that currently preserves third-party Coroutine (module !== 'typing') to
remain unchanged so only the 'typing' module gets the Promise conversion, and
add/adjust tests to cover typing.Coroutine[A,B,T] -> Promise<T>.
In `@tywrap_ir/tywrap_ir/ir.py`:
- Around line 99-104: The current logic strips module qualification from class
annotations by taking class_path.split(".")[-1]; instead, preserve the full
qualified path so type identity is not lost: when handling the string form of an
annotation (variables str_repr, class_path, annotation), return the whole
class_path (e.g., "datetime.datetime") instead of only the last segment; keep
the existing guard that checks str_repr.startswith("<class '") and
str_repr.endswith("'>") and only change the return behavior around class_path
(remove the dot-splitting).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6571322-8e25-4c3c-900e-afa84d5a5315
📒 Files selected for processing (13)
docs/type-mapping-matrix.mdsrc/core/annotation-parser.tssrc/core/generator.tssrc/core/mapper.tssrc/runtime/node.tssrc/tywrap.tssrc/utils/runtime.tstest/annotation-parser.test.tstest/generator.test.tstest/integration.test.tstest/ir_extraction_advanced.test.tstest/type_mapping_enhanced.test.tstywrap_ir/tywrap_ir/ir.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
test/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
NODE_OPTIONS=--expose-gcenvironment variable for GC-sensitive tests
Files:
test/annotation-parser.test.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tstest/generator.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode; avoid
any— useunknownand type guards instead
Files:
src/runtime/node.tssrc/utils/runtime.tssrc/core/annotation-parser.tssrc/core/generator.tssrc/tywrap.tssrc/core/mapper.ts
src/runtime/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Runtime bridges code should be located in
src/runtime/with corresponding tests intest/runtime_*.test.ts
Files:
src/runtime/node.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode and avoid
any— useunknownand type guards instead
Files:
src/runtime/node.tssrc/utils/runtime.tssrc/core/annotation-parser.tssrc/core/generator.tssrc/tywrap.tssrc/core/mapper.ts
tywrap_ir/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must follow PEP 8 and use type hints
Follow PEP 8 and use type hints in Python code
Files:
tywrap_ir/tywrap_ir/ir.py
🧠 Learnings (20)
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to test/runtime_*.test.ts : Include tests for new runtime behavior following the `test/runtime_*.test.ts` patterns
Applied to files:
test/annotation-parser.test.tssrc/utils/runtime.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tstest/generator.test.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to test/runtime_*.test.ts : Include tests for new runtime behavior following patterns in `test/runtime_*.test.ts`
Applied to files:
test/annotation-parser.test.tssrc/utils/runtime.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tstest/generator.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/annotation-parser.test.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tstest/generator.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/annotation-parser.test.tssrc/runtime/node.tssrc/utils/runtime.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tssrc/core/annotation-parser.tssrc/core/generator.tssrc/tywrap.tssrc/core/mapper.tstest/generator.test.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/annotation-parser.test.tssrc/runtime/node.tssrc/utils/runtime.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tssrc/core/annotation-parser.tssrc/core/generator.tssrc/tywrap.tssrc/core/mapper.tstest/generator.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/annotation-parser.test.tssrc/runtime/node.tssrc/utils/runtime.tstest/type_mapping_enhanced.test.tstest/ir_extraction_advanced.test.tstest/integration.test.tssrc/core/annotation-parser.tssrc/core/generator.tssrc/tywrap.tssrc/core/mapper.tstest/generator.test.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/node.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
src/runtime/node.tstest/integration.test.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
src/runtime/node.tstest/integration.test.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Place runtime bridge implementations in `src/runtime/` with corresponding tests in `test/runtime_*.test.ts`
Applied to files:
src/runtime/node.tstest/integration.test.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to src/runtime/**/*.ts : Runtime bridges code should be located in `src/runtime/` with corresponding tests in `test/runtime_*.test.ts`
Applied to files:
src/runtime/node.tstest/integration.test.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/node.ts
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to tywrap_ir/**/*.py : Follow PEP 8 and use type hints in Python code
Applied to files:
docs/type-mapping-matrix.mdsrc/tywrap.tstywrap_ir/tywrap_ir/ir.py
📚 Learning: 2026-03-17T05:27:14.946Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T05:27:14.946Z
Learning: Applies to src/**/*.ts : Use TypeScript strict mode and avoid `any` — use `unknown` and type guards instead
Applied to files:
docs/type-mapping-matrix.mdsrc/core/annotation-parser.tssrc/core/mapper.tstest/generator.test.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to src/**/*.{ts,tsx} : TypeScript strict mode; avoid `any` — use `unknown` and type guards instead
Applied to files:
docs/type-mapping-matrix.mdsrc/core/annotation-parser.tssrc/core/mapper.tstest/generator.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/integration.test.tssrc/core/annotation-parser.tstest/generator.test.ts
📚 Learning: 2026-01-20T16:01:39.136Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/integration.test.ts
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
test/integration.test.tssrc/tywrap.ts
📚 Learning: 2026-03-17T05:27:08.277Z
Learnt from: CR
Repo: bbopen/tywrap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T05:27:08.277Z
Learning: Applies to tywrap_ir/**/*.py : Python code must follow PEP 8 and use type hints
Applied to files:
src/tywrap.tstywrap_ir/tywrap_ir/ir.py
🪛 Ruff (0.15.6)
tywrap_ir/tywrap_ir/ir.py
[warning] 13-13: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 13-13: typing.List is deprecated, use list instead
(UP035)
[warning] 17-17: Do not catch blind exception: Exception
(BLE001)
[warning] 121-121: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 130-130: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 166-166: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 181-181: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[error] 185-186: try-except-continue detected, consider logging the exception
(S112)
[warning] 185-185: Do not catch blind exception: Exception
(BLE001)
[warning] 191-191: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[warning] 199-199: Dynamically typed expressions (typing.Any) are disallowed in annotation
(ANN401)
[warning] 207-207: Do not catch blind exception: Exception
(BLE001)
[warning] 214-214: Do not catch blind exception: Exception
(BLE001)
[warning] 229-229: Dynamically typed expressions (typing.Any) are disallowed in *annotations
(ANN401)
[warning] 250-250: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[warning] 250-250: Dynamically typed expressions (typing.Any) are disallowed in *annotations
(ANN401)
[warning] 258-258: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[warning] 259-259: Dynamically typed expressions (typing.Any) are disallowed in *annotations
(ANN401)
[warning] 274-274: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[warning] 274-274: Dynamically typed expressions (typing.Any) are disallowed in _unwrap_type_alias_value
(ANN401)
[warning] 280-280: Do not catch blind exception: Exception
(BLE001)
[warning] 284-284: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 287-287: Do not catch blind exception: Exception
(BLE001)
[warning] 292-292: Do not catch blind exception: Exception
(BLE001)
[warning] 304-304: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
[warning] 311-311: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
[error] 322-323: try-except-pass detected, consider logging the exception
(S110)
[warning] 322-322: Do not catch blind exception: Exception
(BLE001)
[warning] 327-327: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 327-327: Unused function argument: module_name
(ARG001)
[warning] 327-327: Boolean-typed positional argument in function definition
(FBT001)
[error] 338-339: try-except-continue detected, consider logging the exception
(S112)
[warning] 338-338: Do not catch blind exception: Exception
(BLE001)
[warning] 356-356: Do not catch blind exception: Exception
(BLE001)
[warning] 371-371: Dynamically typed expressions (typing.Any) are disallowed in module
(ANN401)
[warning] 371-371: Unused function argument: module_name
(ARG001)
[warning] 371-371: Boolean-typed positional argument in function definition
(FBT001)
[error] 384-385: try-except-continue detected, consider logging the exception
(S112)
[warning] 384-384: Do not catch blind exception: Exception
(BLE001)
[warning] 409-409: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
[warning] 418-418: Do not catch blind exception: Exception
(BLE001)
[warning] 423-423: Do not catch blind exception: Exception
(BLE001)
[warning] 426-426: Do not catch blind exception: Exception
(BLE001)
[error] 613-614: try-except-pass detected, consider logging the exception
(S110)
[warning] 613-613: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
src/utils/runtime.ts (1)
718-726: No issues in fallback hash block change.This edit appears to be formatting-only and preserves existing fallback behavior and fixed-length output guarantees.
src/runtime/node.ts (2)
77-77: Legacy warmup union is correctly preserved for migration-safe errors.This keeps backwards-compatible typing while still allowing
normalizeWarmupCommandsto fail fast with a clear migration message at runtime.
612-612: Warmup error message fallback remains safe and clear.The condensed conditional preserves the same defensive behavior and readable failure output.
src/tywrap.ts (1)
470-476: Add regression test for method generic shadowing.The implementation correctly handles nested generic shadowing:
annotationTypeParametersis built as[...inheritedTypeParameters, ...localTypeParameters], andparseAnnotationToPythonType()usesknownTypeParameters.get()to resolve names—a Map lookup that respects insertion order, so local parameters correctly shadow inherited ones. However, there is no explicit test for this case (e.g.,class Box[T]: def method[T](...)). Add a test to prevent regression.tywrap_ir/tywrap_ir/ir.py (1)
311-324: The suggested fix does not actually address the stated problem.Removing
inspect.isclassfrom line 312 has no effect on the extraction of simple type aliases likeUserId = str. Testing confirms that both the current and proposed implementations produce identical results—they reject simple class assignments because those assignments lack the properties that_is_type_alias_value()checks for (TypeAliasType,GenericAlias,typing.get_origin()result, or__parameters__attribute).The review comment's diagnosis is incomplete. The
inspect.isclasscheck is not the barrier preventingUserId = strfrom being extracted; the barrier is thatstrhas none of the characteristics that identify type aliases. To actually capture undecorated class assignments as type aliases would require either an explicitTypeAliasannotation (UserId: TypeAlias = str) or a substantially different detection approach in_is_type_alias_value().> Likely an incorrect or invalid review comment.
| | Python Type | TypeScript Type | Notes | | ||
| | -------------------------- | --------------- | --------------------------------------------------------------------------------------------------- | | ||
| | `TypeVar('T')` | `T` | Emitted as a declaration-level generic parameter when tywrap can declare it safely | | ||
| | `TypeVar('T', bound=Base)` | `unknown` | Bound information is currently treated conservatively instead of emitting an unsupported TS generic | | ||
| | `TypeVar('T', str, int)` | `unknown` | Constraints are currently treated conservatively instead of emitting a partial TS equivalent | |
There was a problem hiding this comment.
Document variance fallback for TypeVar.
The generator only emits declaration-level generics for invariant TypeVars. Covariant and contravariant params currently fall back too, so this table now over-promises by only calling out bounds and constraints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/type-mapping-matrix.md` around lines 122 - 126, The table entry for
TypeVar is misleading: update the rows for `TypeVar('T')`, `TypeVar('T',
bound=Base)`, and `TypeVar('T', str, int)` to explicitly state that only
invariant TypeVars are emitted as declaration-level generics and that
covariant/contravariant TypeVars (variance) as well as TypeVars with bounds or
constraints currently fall back to `unknown`; revise the `TypeVar('T')` row to
note "invariant only" and add a brief note to the other two rows that they cover
both bounds/constraints and variance fallbacks so the table no longer
over-promises.
| typeParameters.forEach(param => { | ||
| if ( | ||
| param.kind === 'typevar' && | ||
| !param.bound && | ||
| !(param.constraints && param.constraints.length > 0) && | ||
| (!param.variance || param.variance === 'invariant') | ||
| ) { | ||
| emitted.push({ name: param.name, declaration: param.name }); | ||
| emittedNames.add(param.name); | ||
| return; | ||
| } | ||
|
|
||
| if (param.kind === 'paramspec' && callableParamSpecs.has(param.name)) { | ||
| emitted.push({ name: param.name, declaration: `${param.name} extends unknown[]` }); | ||
| emittedNames.add(param.name); | ||
| emittedParamSpecs.add(param.name); | ||
| } | ||
| }); | ||
|
|
||
| return { | ||
| currentModule, | ||
| declaration: | ||
| emitted.length > 0 ? `<${emitted.map(param => param.declaration).join(', ')}>` : '', | ||
| typeArguments: emitted.length > 0 ? `<${emitted.map(param => param.name).join(', ')}>` : '', | ||
| emittedNames, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && wc -l src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '146,170p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '1,50p' src/core/generator.ts | head -30Repository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '100,180p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && rg -n "escapeIdentifier" src/core/generator.ts -A 5Repository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '270,310p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && rg -n "param\.name" src/core/generator.ts -B 2 -A 2Repository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && rg -n "default|new|extends|class|function" src/core/generator.ts | head -20Repository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '128,200p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '200,280p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && rg -n "type GenericRenderContext\|interface GenericRenderContext" -A 10 src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && rg -n "GenericRenderContext" src/core/generator.ts | head -20Repository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
cd /tmp/repo && sed -n '1,130p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 116
🏁 Script executed:
pwd && ls -laRepository: bbopen/tywrap
Length of output: 2125
🏁 Script executed:
git ls-files src/core/generator.tsRepository: bbopen/tywrap
Length of output: 78
🏁 Script executed:
cat -n src/core/generator.ts | sed -n '146,170p'Repository: bbopen/tywrap
Length of output: 1139
🏁 Script executed:
cat -n src/core/generator.ts | sed -n '1,50p'Repository: bbopen/tywrap
Length of output: 1332
🏁 Script executed:
cat -n src/core/generator.ts | sed -n '100,180p'Repository: bbopen/tywrap
Length of output: 3287
🏁 Script executed:
rg -n "escapeIdentifier" src/core/generator.ts -A 5Repository: bbopen/tywrap
Length of output: 7804
🏁 Script executed:
cat -n src/core/generator.ts | sed -n '270,310p'Repository: bbopen/tywrap
Length of output: 1578
🏁 Script executed:
rg -n "param\.name" src/core/generator.ts -B 2 -A 2 | head -50Repository: bbopen/tywrap
Length of output: 856
🏁 Script executed:
cat -n src/core/generator.ts | sed -n '1,180p'Repository: bbopen/tywrap
Length of output: 5976
Escape emitted generic identifiers before emitting them in TypeScript declarations.
param.name is copied directly into <...> without sanitization, but Python allows names like default or new that are invalid in TypeScript. For example, TypeVar("default") produces <default> which is a syntax error. Use this.escapeIdentifier(param.name) when building the declaration field (lines 153, 159) and name field in the emitted array, and update emittedNames to store the escaped name so that sanitizeType checks match the emitted identifiers.
Also applies to: 274-276, 300-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/generator.ts` around lines 146 - 170, The emitted generic
identifiers are inserted raw causing invalid TS for names like "default" —
update the code paths that build the emitted array and
emittedNames/emittedParamSpecs to use this.escapeIdentifier(param.name) instead
of param.name: when pushing into emitted use declaration:
`${this.escapeIdentifier(param.name)}${...}` (or
`${this.escapeIdentifier(param.name)} extends unknown[]` for paramspecs) and set
the pushed object's name to the escaped identifier; also add the escaped
identifier to emittedNames and emittedParamSpecs so later sanitizeType checks
match; apply the same replacement wherever emitted/declaration/typeArguments are
composed (including other spots that build emitted.map(param => param.name) and
emitted.map(param => param.declaration)) and ensure all uses of param.name in
these generators are escaped via this.escapeIdentifier.
| private mergeGenericRenderContexts( | ||
| outer: GenericRenderContext, | ||
| inner: GenericRenderContext | ||
| ): GenericRenderContext { | ||
| return { | ||
| currentModule: inner.currentModule ?? outer.currentModule, | ||
| declaration: inner.declaration, | ||
| typeArguments: inner.typeArguments, | ||
| emittedNames: new Set([...outer.emittedNames, ...inner.emittedNames]), | ||
| emittedParamSpecs: new Set([...outer.emittedParamSpecs, ...inner.emittedParamSpecs]), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and check its size
find . -type f -name "generator.ts" | head -20Repository: bbopen/tywrap
Length of output: 80
🏁 Script executed:
# Get the file size to determine if we can read it entirely
wc -l src/core/generator.tsRepository: bbopen/tywrap
Length of output: 83
🏁 Script executed:
# Read the relevant sections around lines 175-185 and 274-276
sed -n '170,190p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 713
🏁 Script executed:
# Also read the 274-276 area
sed -n '270,280p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 440
🏁 Script executed:
# Search for GenericRenderContext type definition
rg "type GenericRenderContext|interface GenericRenderContext" -A 10Repository: bbopen/tywrap
Length of output: 592
🏁 Script executed:
# Search for other usages of mergeGenericRenderContexts to understand how it's used
rg "mergeGenericRenderContexts" -B 3 -A 3Repository: bbopen/tywrap
Length of output: 1168
🏁 Script executed:
# Look for any shadow-related logic or type guards in the file
rg "shadow" -i src/core/generator.tsRepository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Read lines 270-285 to see the context around 274-276
sed -n '270,285p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 567
🏁 Script executed:
# Search for how emittedNames is used in type resolution
rg "emittedNames\.has|emittedNames\." -B 2 -A 2Repository: bbopen/tywrap
Length of output: 955
🏁 Script executed:
# Look for sanitizeType method and understand its full scope
sed -n '250,300p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 1818
🏁 Script executed:
# Check how the generic contexts are created initially
rg "emittedNames:" -B 3 -A 1Repository: bbopen/tywrap
Length of output: 730
🏁 Script executed:
# Search for type parameter handling and scope awareness
rg "typevar|paramspec" src/core/generator.ts -B 2 -A 2 | head -80Repository: bbopen/tywrap
Length of output: 1353
🏁 Script executed:
# Check if there are any TODOs or comments about shadowing or scope
rg "TODO|FIXME|BUG|shadow" -i src/core/generator.tsRepository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Look for how declaration field is used in GenericRenderContext
rg "declaration" src/core/generator.ts -B 2 -A 2 | head -40Repository: bbopen/tywrap
Length of output: 1304
🏁 Script executed:
# Check if there's any scope tracking mechanism elsewhere
rg "scope|origin|priority" -i src/core/generator.ts | head -20Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Examine the complete flow: how contexts are created and what declaration contains
sed -n '100,170p' src/core/generator.tsRepository: bbopen/tywrap
Length of output: 2474
Make merged generic contexts shadow-aware.
The merged context unions all emitted names into a single set without tracking scope, so an inner (method-local) type parameter can silently resolve as an outer (class-level) parameter. For example, class Box[T]: def f[T: int](x: T) -> T would resolve the method's T through the class parameter instead of recognizing the shadow. The sanitizeType check at line 276 (ctx.emittedNames.has(type.name)) cannot distinguish between scopes. While declaration and typeArguments fields correctly use only the inner scope, emittedNames needs to track scope origin or precedence to prevent shadowed parameters from resolving through outer contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/generator.ts` around lines 175 - 185, mergeGenericRenderContexts
currently unions emittedNames across scopes which loses shadowing info and
allows inner type parameters to be resolved to outer names; update
mergeGenericRenderContexts to preserve scope precedence instead of a flat Set
(e.g., make emittedNames a stack/array of Sets or a structure that records
origin order so lookups consult inner first), keep declaration and typeArguments
as-is, and adapt sanitizeType (the ctx.emittedNames.has(...) check) to consult
the new precedence-aware emittedNames structure so inner/declared names shadow
outer ones; refer to mergeGenericRenderContexts, emittedNames, and sanitizeType
when making the change.
| await writeFile( | ||
| join(compileDir, 'consumer.ts'), | ||
| `import { Container, Pair, Transform, acceptTransform, forward, identity, passthrough } from '../generated/generic_module.generated.js'; | ||
|
|
||
| const pair: Pair<string> = ['a', 'b']; | ||
| const transform: Transform<[number], string> = (...args) => String(args[0]); | ||
| const container = Container.fromHandle<number>('handle'); | ||
| const accepted: Promise<Transform<[number], string>> = acceptTransform<[number], string>(transform); | ||
| const forwarded: Promise<Container<number>> = forward<number>(container); | ||
| const resolved: Promise<number> = identity<number>(1); | ||
| const cloned: Promise<Container<number>> = container.clone(); | ||
| const identified: Promise<[number, string]> = container.id<string>('value'); | ||
| const passthroughResult: Promise<void> = passthrough([1, 2], { flag: true }); | ||
|
|
||
| const checkResult = await generate(options as any, { check: true }); | ||
| expect((checkResult.outOfDate ?? []).includes(generatedTsPath as string)).toBe(true); | ||
| void pair; | ||
| void transform; | ||
| void accepted; | ||
| void forwarded; | ||
| void resolved; | ||
| void cloned; | ||
| void identified; | ||
| void passthroughResult; | ||
| `, | ||
| 'utf-8' | ||
| ); | ||
| await writeFile( | ||
| join(compileDir, 'tsconfig.json'), | ||
| JSON.stringify( | ||
| { | ||
| compilerOptions: { | ||
| target: 'ES2022', | ||
| module: 'NodeNext', | ||
| moduleResolution: 'NodeNext', | ||
| strict: true, | ||
| noEmit: true, | ||
| skipLibCheck: false, | ||
| baseUrl: '.', | ||
| paths: { | ||
| 'tywrap/runtime': ['./runtime-stub'], | ||
| }, | ||
| }, | ||
| include: [ | ||
| './consumer.ts', | ||
| './runtime-stub.d.ts', | ||
| '../generated/generic_module.generated.ts', | ||
| '../generated/generic_module.generated.d.ts', | ||
| ], | ||
| }, | ||
| null, | ||
| 2 | ||
| ), | ||
| 'utf-8' | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript with "moduleResolution": "NodeNext", if a source file imports "../generated/foo.generated.js" and both "../generated/foo.generated.ts" and "../generated/foo.generated.d.ts" exist in the program, which file is used for type checking?
💡 Result:
In TypeScript with "moduleResolution": "NodeNext", when importing "../generated/foo.generated.js" and both "../generated/foo.generated.ts" and "../generated/foo.generated.d.ts" exist in the program, TypeScript uses the foo.generated.ts file for type checking. TypeScript's NodeNext resolution first checks for the source file (foo.generated.ts) matching the import extension pattern, then declaration files (.d.ts), then JS files. Since foo.generated.ts exists and provides full type information (including implementation for emission if needed), it takes precedence over foo.generated.d.ts for type checking. The .d.ts is typically for JS modules without source, but here the source is preferred when present.
Citations:
- 1: Refactor module resolution
Extensions, fix lookup priorities microsoft/TypeScript#51471 - 2: https://coldfusion-example.blogspot.com/2026/02/typescript-module-resolution-guide.html
- 3: Module resolution: NodeNext breaks typechecking microsoft/TypeScript#60561
- 4: https://www.totaltypescript.com/relative-import-paths-need-explicit-file-extensions-in-ecmascript-imports
🏁 Script executed:
# Check the test file and surrounding context
wc -l test/integration.test.tsRepository: bbopen/tywrap
Length of output: 85
🏁 Script executed:
# Read the specific section mentioned in the review (lines 225-277)
sed -n '225,277p' test/integration.test.tsRepository: bbopen/tywrap
Length of output: 1815
🏁 Script executed:
# Check the second mentioned section (lines 341-378)
sed -n '341,378p' test/integration.test.tsRepository: bbopen/tywrap
Length of output: 977
🏁 Script executed:
# Check if generated files directory exists and what's in it
find . -path "*/generated/generic_module.generated.*" -type f 2>/dev/null | head -20Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Check the actual structure around these test areas to understand context
sed -n '215,285p' test/integration.test.ts | cat -nRepository: bbopen/tywrap
Length of output: 3094
Remove generated .ts files from test compiles to actually validate the .d.ts declarations.
Both test programs include the generated .ts implementation alongside the .d.ts declarations in the include array. With moduleResolution: "NodeNext", TypeScript binds the import of ../generated/foo.generated.js to the sibling .ts source file when present, bypassing the declarations entirely. This undermines the declaration validation these tests should provide.
Exclude the .ts files from the include array to compile only against the declarations:
- Remove
'../generated/generic_module.generated.ts'(line 225-277 test, also lines 341-378) - Remove
'../generated/protocol_module.generated.ts'(lines 341-378 test)
This ensures the consumer actually type-checks against the exported .d.ts surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration.test.ts` around lines 225 - 277, The tsconfig written in the
test includes the generated implementation .ts files which causes TypeScript to
bind imports to the .ts sources instead of the .d.ts declarations; update the
tsconfig JSON written in the test (the object passed to writeFile for
tsconfig.json under compileDir) to remove
'../generated/generic_module.generated.ts' (and similarly remove
'../generated/protocol_module.generated.ts' in the other tsconfig write) from
the "include" array so the test compiles only against the .d.ts files and
validates the declaration surface.
| async function supportsVariadicTypingFeatures(): Promise<boolean> { | ||
| const result = await processUtils.exec(PYTHON_EXECUTABLE, [ | ||
| '-c', | ||
| ` | ||
| try: | ||
| from typing import ParamSpec, TypeVarTuple, Unpack | ||
| except ImportError: | ||
| try: | ||
| from typing_extensions import ParamSpec, TypeVarTuple, Unpack | ||
| except ImportError: | ||
| raise SystemExit(1) | ||
| raise SystemExit(0) | ||
| `, | ||
| ]); | ||
| return result.code === 0; | ||
| } | ||
|
|
||
| async function supportsTypingExtensionsBackports(): Promise<boolean> { | ||
| const result = await processUtils.exec(PYTHON_EXECUTABLE, [ | ||
| '-c', | ||
| 'from typing_extensions import ParamSpec, TypeVarTuple, Unpack', | ||
| ]); | ||
| return result.code === 0; | ||
| } | ||
|
|
||
| async function supportsPep695Syntax(): Promise<boolean> { | ||
| const result = await processUtils.exec(PYTHON_EXECUTABLE, [ | ||
| '-c', | ||
| 'import sys; raise SystemExit(0 if sys.version_info >= (3, 12) else 1)', | ||
| ]); | ||
| return result.code === 0; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the Python feature probes into a shared test helper.
supportsVariadicTypingFeatures() and supportsPep695Syntax() are now duplicated here and in test/integration.test.ts. Moving them to a common helper will keep the gating logic from drifting as more version-specific cases get added.
| test('maps generic typing.Awaitable[T] to Promise<T>', () => { | ||
| const awaitableType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Awaitable', | ||
| module: 'typing', | ||
| typeArgs: [{ kind: 'primitive', name: 'str' }], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(awaitableType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Promise'); | ||
| expect(result.typeArgs).toEqual([{ kind: 'primitive', name: 'string' }]); | ||
| }); | ||
|
|
||
| test('maps generic typing.Sequence[T] to Array<T>', () => { | ||
| const sequenceType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Sequence', | ||
| module: 'typing', | ||
| typeArgs: [{ kind: 'primitive', name: 'int' }], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(sequenceType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Array'); | ||
| expect(result.typeArgs).toEqual([{ kind: 'primitive', name: 'number' }]); | ||
| }); | ||
|
|
||
| test('maps generic collections.abc.Sequence[T] to Array<T>', () => { | ||
| const sequenceType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Sequence', | ||
| module: 'collections.abc', | ||
| typeArgs: [{ kind: 'primitive', name: 'bool' }], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(sequenceType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Array'); | ||
| expect(result.typeArgs).toEqual([{ kind: 'primitive', name: 'boolean' }]); | ||
| }); | ||
|
|
||
| test('maps generic typing_extensions.Mapping[K, V] to an object index signature', () => { | ||
| const mappingType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Mapping', | ||
| module: 'typing_extensions', | ||
| typeArgs: [ | ||
| { kind: 'primitive', name: 'str' }, | ||
| { kind: 'primitive', name: 'int' }, | ||
| ], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(mappingType) as TSObjectType; | ||
|
|
||
| expect(result.kind).toBe('object'); | ||
| expect(result.properties).toEqual([]); | ||
| expect(result.indexSignature).toEqual({ | ||
| keyType: { kind: 'primitive', name: 'string' }, | ||
| valueType: { kind: 'primitive', name: 'number' }, | ||
| }); | ||
| }); | ||
|
|
||
| test('preserves third-party Sequence generics', () => { | ||
| const sequenceType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Sequence', | ||
| module: 'pkg', | ||
| typeArgs: [{ kind: 'primitive', name: 'int' }], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(sequenceType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Sequence'); | ||
| expect(result.typeArgs).toEqual([{ kind: 'primitive', name: 'number' }]); | ||
| }); | ||
|
|
||
| test('preserves third-party Awaitable generics', () => { | ||
| const awaitableType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Awaitable', | ||
| module: 'pkg', | ||
| typeArgs: [{ kind: 'primitive', name: 'str' }], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(awaitableType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Awaitable'); | ||
| expect(result.typeArgs).toEqual([{ kind: 'primitive', name: 'string' }]); | ||
| }); | ||
|
|
||
| test('preserves third-party Coroutine generics', () => { | ||
| const coroutineType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Coroutine', | ||
| module: 'vendor', | ||
| typeArgs: [ | ||
| { kind: 'primitive', name: 'int' }, | ||
| { kind: 'primitive', name: 'int' }, | ||
| { kind: 'primitive', name: 'str' }, | ||
| ], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(coroutineType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Coroutine'); | ||
| expect(result.typeArgs).toEqual([ | ||
| { kind: 'primitive', name: 'number' }, | ||
| { kind: 'primitive', name: 'number' }, | ||
| { kind: 'primitive', name: 'string' }, | ||
| ]); | ||
| }); | ||
|
|
||
| test('preserves third-party Mapping generics', () => { | ||
| const mappingType: PythonType = { | ||
| kind: 'generic', | ||
| name: 'Mapping', | ||
| module: 'vendor', | ||
| typeArgs: [ | ||
| { kind: 'primitive', name: 'str' }, | ||
| { kind: 'primitive', name: 'int' }, | ||
| ], | ||
| }; | ||
|
|
||
| const result = mapper.mapPythonType(mappingType) as TSGenericType; | ||
|
|
||
| expect(result.kind).toBe('generic'); | ||
| expect(result.name).toBe('Mapping'); | ||
| expect(result.typeArgs).toEqual([ | ||
| { kind: 'primitive', name: 'string' }, | ||
| { kind: 'primitive', name: 'number' }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Add a direct typing.Coroutine[...] conversion case.
The new assertions cover the preserve-as-is branch for third-party Coroutine, but not the new typing.Coroutine[A, B, T] -> Promise<T> mapper path. A regression there would still pass this suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/type_mapping_enhanced.test.ts` around lines 232 - 370, Add a direct
conversion in mapPythonType for generic types named "Coroutine" coming from
module "typing": when encountering a PythonType with kind 'generic', name
'Coroutine', and module 'typing', map it to a TS generic Promise using the third
type argument as the promise value type (i.e., Promise<T>), making sure to map
that T via the existing mapper logic (call mapper.mapPythonType on typeArgs[2])
and handle the case where fewer than 3 typeArgs exist gracefully (fallback to
Promise<any> or similar). Update the branch that currently preserves third-party
Coroutine (module !== 'typing') to remain unchanged so only the 'typing' module
gets the Promise conversion, and add/adjust tests to cover
typing.Coroutine[A,B,T] -> Promise<T>.
| str_repr = str(annotation) | ||
|
|
||
| # Clean up class references to show just the class name | ||
| if str_repr.startswith("<class '") and str_repr.endswith("'>"): | ||
| # Extract class name from <class 'module.ClassName'> | ||
| class_path = str_repr[8:-2] # Remove <class ' and '> | ||
| if '.' in class_path: | ||
| return class_path.split('.')[-1] # Just the class name | ||
| class_path = str_repr[8:-2] | ||
| if "." in class_path: | ||
| return class_path.split(".")[-1] | ||
| return class_path |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "ir.py" -path "*/tywrap_ir/*" 2>/dev/null | head -5Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
git ls-files | grep -E "tywrap_ir.*ir\.py$"Repository: bbopen/tywrap
Length of output: 118
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '90,115p'Repository: bbopen/tywrap
Length of output: 1143
🏁 Script executed:
rg -n "_stringify_annotation" tywrap_ir/tywrap_ir/ir.pyRepository: bbopen/tywrap
Length of output: 1141
🏁 Script executed:
rg -n "_stringify_annotation" tywrap_ir/ --type pyRepository: bbopen/tywrap
Length of output: 1626
🏁 Script executed:
rg -B5 -A10 "def _stringify_annotation" tywrap_ir/tywrap_ir/ir.pyRepository: bbopen/tywrap
Length of output: 588
🏁 Script executed:
find tywrap_ir -name "*test*.py" -o -name "test_*" | head -10Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
git ls-files | grep -i test | grep irRepository: bbopen/tywrap
Length of output: 503
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '380,400p'Repository: bbopen/tywrap
Length of output: 1076
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '215,225p'Repository: bbopen/tywrap
Length of output: 584
🏁 Script executed:
cat test/__snapshots__/ir_snapshot.test.ts.snap | head -100Repository: bbopen/tywrap
Length of output: 498
🏁 Script executed:
cat -n test/__snapshots__/ir_snapshot.test.ts.snap | sed -n '1,500p'Repository: bbopen/tywrap
Length of output: 701
🏁 Script executed:
rg -A5 -B5 "annotation" test/__snapshots__/ir_snapshot.test.ts.snap | head -100Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '1,40p'Repository: bbopen/tywrap
Length of output: 1084
🏁 Script executed:
rg -B3 -A3 "datetime" tywrap_ir/ --type pyRepository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
python3 << 'EOF'
import inspect
from datetime import datetime
# Simulate what _stringify_annotation does
annotation = datetime
str_repr = str(annotation)
print(f"str(annotation) = {str_repr}")
if str_repr.startswith("<class '") and str_repr.endswith("'>"):
class_path = str_repr[8:-2]
print(f"class_path = {class_path}")
if "." in class_path:
shortened = class_path.split(".")[-1]
print(f"shortened = {shortened}")
else:
print(f"no dots, returns = {class_path}")
# Test with a custom class
class MyClass:
pass
annotation2 = MyClass
str_repr2 = str(annotation2)
print(f"\nstr(MyClass) = {str_repr2}")
EOFRepository: bbopen/tywrap
Length of output: 197
🏁 Script executed:
rg -B10 -A10 "IRTypeAlias\|definition" tywrap_ir/tywrap_ir/ir.py | head -80Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '50,75p'Repository: bbopen/tywrap
Length of output: 706
🏁 Script executed:
python3 << 'EOF'
# Check what happens with nested modules and collisions
test_cases = [
"<class 'datetime.datetime'>",
"<class 'collections.defaultdict'>",
"<class 'MyClass'>",
"<class '__main__.MyClass'>",
]
for case in test_cases:
if case.startswith("<class '") and case.endswith("'>"):
class_path = case[8:-2]
if "." in class_path:
shortened = class_path.split(".")[-1]
print(f"{case:45} -> {shortened}")
else:
print(f"{case:45} -> {class_path}")
EOFRepository: bbopen/tywrap
Length of output: 289
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '73,90p'Repository: bbopen/tywrap
Length of output: 496
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '360,410p'Repository: bbopen/tywrap
Length of output: 2133
🏁 Script executed:
grep -r "definition" test/ --include="*.ts" | head -20Repository: bbopen/tywrap
Length of output: 613
🏁 Script executed:
python3 << 'EOF'
# Scenario: collision risk with module stripping
# If we have both a local datetime and datetime.datetime
print("Scenario 1: Type alias for datetime.datetime")
print(" definition = 'datetime' (current behavior)")
print(" Problem: can't distinguish from local datetime class")
print()
print("Scenario 2: Type alias for collections.defaultdict")
print(" definition = 'defaultdict' (current behavior)")
print(" Problem: might collide with a local defaultdict")
print()
print("The reviewer's suggestion: keep full path")
print(" definition = 'datetime.datetime'")
print(" definition = 'collections.defaultdict'")
print(" Downstream TypeScript can shorten if needed")
EOFRepository: bbopen/tywrap
Length of output: 516
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '370,400p'Repository: bbopen/tywrap
Length of output: 1630
🏁 Script executed:
cat -n test/ir_extraction_advanced.test.ts | sed -n '1,100p'Repository: bbopen/tywrap
Length of output: 3741
🏁 Script executed:
rg -B5 -A5 "expect.*definition" test/ --type tsRepository: bbopen/tywrap
Length of output: 965
🏁 Script executed:
rg -B10 -A10 "type_alias" test/ir_extraction_advanced.test.ts | head -80Repository: bbopen/tywrap
Length of output: 3244
🏁 Script executed:
grep -r "datetime\|collections\|external" test/fixtures/python/ --include="*.py"Repository: bbopen/tywrap
Length of output: 3143
🏁 Script executed:
python3 << 'EOF'
# Check what str() returns for various type objects
import typing
from datetime import datetime
from collections import defaultdict
test_values = [
datetime,
defaultdict,
str,
int,
list,
]
for val in test_values:
print(f"str({val.__name__:15}) = {str(val)}")
# Check what happens with generic aliases
print("\nGeneric aliases:")
print(f"str(typing.List[int]) = {str(typing.List[int])}")
print(f"str(tuple[int, int]) = {str(tuple[int, int])}")
EOFRepository: bbopen/tywrap
Length of output: 374
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '130,170p'Repository: bbopen/tywrap
Length of output: 1775
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '200,240p'Repository: bbopen/tywrap
Length of output: 1596
🏁 Script executed:
python3 << 'EOF'
# Test what annotation strings look like for type aliases
import typing
# Simulate what _stringify_annotation returns for various cases
test_cases = [
("bare class", "<class 'datetime.datetime'>"),
("bare class no module", "<class 'str'>"),
("generic alias", "tuple[int, int]"),
("typing generic", "typing.List[int]"),
]
for label, val in test_cases:
if val.startswith("<class '") and val.endswith("'>"):
class_path = val[8:-2]
if "." in class_path:
shortened = class_path.split(".")[-1]
print(f"{label:25} {val:40} → {shortened}")
else:
print(f"{label:25} {val:40} → {class_path}")
else:
print(f"{label:25} {val:40} → (no change)")
EOFRepository: bbopen/tywrap
Length of output: 369
🏁 Script executed:
rg -B5 -A5 "definition.*datetime\|datetime.*definition" test/ tywrap_ir/Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
python3 << 'EOF'
# Simulate the collision scenario the reviewer mentioned
import sys
from io import StringIO
# Create a scenario where we have local datetime and external datetime.datetime
code = """
from datetime import datetime
# User defines a local wrapper
class datetime:
'''A local datetime wrapper'''
pass
# Type alias using the local class
UserId = datetime
"""
# Parse and check what would be extracted
lines = code.strip().split('\n')
for i, line in enumerate(lines, 1):
print(f"{i:2}: {line}")
print("\nScenario: _stringify_annotation(datetime) where datetime is the LOCAL class")
print("Current behavior would return: 'datetime'")
print("But which datetime? The imported one or the local wrapper?")
print("Information loss creates ambiguity in the IR.")
EOFRepository: bbopen/tywrap
Length of output: 492
🏁 Script executed:
cat -n tywrap_ir/tywrap_ir/ir.py | sed -n '95,108p'Repository: bbopen/tywrap
Length of output: 638
🏁 Script executed:
python3 << 'EOF'
# Check if there are any test assertions about class name stripping
import subprocess
result = subprocess.run(
['grep', '-r', 'definition.*:.*', 'test/', '--include=*.ts'],
capture_output=True,
text=True,
cwd='/tmp'
)
# Filter for meaningful results
lines = [l for l in result.stdout.split('\n') if 'definition' in l and ('datetime' in l or 'defaultdict' in l or '.' in l)]
for line in lines[:10]:
print(line)
if not lines:
print("No test assertions found checking definition strings with module paths")
EOFRepository: bbopen/tywrap
Length of output: 127
Preserve module qualification in class annotations to maintain type identity.
Stripping the module path from <class 'datetime.datetime'> to datetime loses namespace information and creates collision risk with local classes of the same name. Keeping the full qualified path allows downstream type processing to decide when shortening is safe.
♻️ Suggested fix
if str_repr.startswith("<class '") and str_repr.endswith("'>"):
class_path = str_repr[8:-2]
- if "." in class_path:
- return class_path.split(".")[-1]
- return class_path
+ return class_path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tywrap_ir/tywrap_ir/ir.py` around lines 99 - 104, The current logic strips
module qualification from class annotations by taking class_path.split(".")[-1];
instead, preserve the full qualified path so type identity is not lost: when
handling the string form of an annotation (variables str_repr, class_path,
annotation), return the whole class_path (e.g., "datetime.datetime") instead of
only the last segment; keep the existing guard that checks
str_repr.startswith("<class '") and str_repr.endswith("'>") and only change the
return behavior around class_path (remove the dot-splitting).
|
@coderabbitai review Please do a full, extensive review of the merged state this PR would introduce against the current base branch, not only the raw diff. Focus on:
If CI gates are green and you do not find blocker issues, please mark the review as passable/approved from your side. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll do a thorough inspection of the key implementation files before writing the final review. 🏁 Script executed: Length of output: 5408 🏁 Script executed: Length of output: 3673 🏁 Script executed: Length of output: 284 🏁 Script executed: Length of output: 613 🏁 Script executed: Length of output: 956 🏁 Script executed: Length of output: 832 🏁 Script executed: Length of output: 2941 🏁 Script executed: Length of output: 166 🏁 Script executed: Length of output: 17333 🏁 Script executed: Length of output: 2288 🏁 Script executed: Length of output: 9678
Review: PR
|
| Call site | declaration value |
Notes |
|---|---|---|
generateFunctionWrapper |
${jsdoc}${declarationBody} |
Proper overload-only declaration |
| Protocol class fallback | ts (same as runtime) |
Protocol emits a export type = alias — runtime and declaration are identical, correct |
generateClassWrapper |
separate declaration |
Class with typed members |
generateTypeAlias |
ts (same) |
A type alias is already a declaration |
generateModuleDefinition |
assembled declaration string | Aggregates all three categories |
No orphaned or stale call sites.
2. sanitizeType — safe, no silent data loss
sanitizeType replaces only references to undeclared generic identifiers (i.e., typevar/paramspec/typevartuple names not present in emittedNames) with { kind: 'custom', name: 'Any' }. It also strips parameterSpec from callable when the spec wasn't emitted. This is exactly the right conservative behavior to prevent undeclared identifier leakage in generated .ts/.d.ts. Since Any is already the unknown-equivalent fallback documented in the PR, there's no unexpected information loss.
3. buildGenericRenderContext — emission rules are correct and conservative
Only emits:
typevarthat is unconstrained, unbound, and invariantparamspecthat appears in a callable position (detected viacollectCallableParamSpecs), emitted asP extends unknown[]
Bound/constrained TypeVar, TypeVarTuple, and Unpack silently fall back — matches the documented non-goals in #209.
4. Annotation-parser P.args / ~P.args / Request.args disambiguation — correct
The regex /^~?([A-Za-z_][A-Za-z0-9_]*)\.(args|kwargs)$/ is broad, but the guard condition that gates whether a match becomes paramspec_args correctly requires either a known paramspec entry in typeParameters or a leading ~. The three new test cases confirm this:
P.argswith explicittypeParameters→paramspec_args✓~P.argswithouttypeParameters→paramspec_args✓Request.args(no~, not intypeParameters) →custom✓
5. Cache invalidation — correct
TYWRAP_IR_VERSION = '0.2.0' is hardcoded in tywrap.ts, passed to the IR CLI via --ir-version 0.2.0, and included in IrCacheKeyObject.irVersion. Since computeIrCacheFilename SHA-256 digests the entire JSON-stringified key, stale 0.1.0-format caches are correctly invalidated. The new ir_cache_filename.test.ts explicitly asserts that 0.1.0 and 0.2.0 produce different filenames.
6. Declaration emission path — improved, no regression
The removed renderDts regex-based approach was fragile. The new path uses gen.declaration directly from the generator, which is assembled with full generic-context awareness. tywrap.ts line 267 (content: gen.declaration) correctly routes into the output.declaration path.
7. CI matrix — sufficient, new tests are gated correctly
| Dimension | Coverage |
|---|---|
| Node | 20, 22 |
| Python | 3.10, 3.11, 3.12 |
| OS | ubuntu (matrix), macOS + Windows (smoke) |
| Runtime | Bun (ubuntu + macOS) |
The new integration tests in test/integration.test.ts and test/ir_extraction_advanced.test.ts use supportsVariadicTypingFeatures(), supportsTypingExtensionsBackports(), and supportsPep695Syntax() guards — they skip gracefully on Python 3.10/3.11 where PEP 695 is unavailable, and run fully on 3.12. The tsc --noEmit check inside the integration test provides a meaningful compile-verified regression gate.
8. Minor observations (non-blocking)
collectCallableParamSpecsdoes not recurse intotypevar/typevartuplekinds — correct, those are not callable-bearing types.mergeGenericRenderContexts: innerdeclaration/typeArgumentsoverride outer, whileemittedNames/emittedParamSpecsare unioned — semantically correct for method-level overriding of class-level context.- Python 3.13 not in test matrix — not a blocker; PEP 695 coverage comes from 3.12.
python-suite-core/python-suite-dataworkflows do not exercise the newtype_paramsIR path — but these are library-integration suites, not tywrap-generation tests; end-to-end coverage lives in the vitest integration suite with tsc validation.
Verdict
No blocker issues. The implementation is conservative, well-reasoned, properly tested with feature-detection guards, and the CI matrix is adequate. If CI gates pass, this PR is ready to merge.
[approve]
🧠 Learnings used
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2 participants
Closes #209.
Summary
This PR implements safe TypeScript generic emission for issue #209. Before this change, tywrap did not preserve enough Python generic metadata end-to-end, so generated wrappers either dropped generics entirely or leaked raw placeholder syntax such as
~T,P.args, andUnpack[Ts]. That made generated.tsand.d.tsoutput lossy and, in some cases, invalid or misleading for downstream consumers.Root cause
The Python IR did not carry ordered generic parameter metadata for functions, classes, and type aliases, and the local cache key did not distinguish IR schema versions. On the TypeScript side, the internal model dropped exported type aliases, the annotation parser treated generic placeholders as opaque names, and declaration files were derived after generation instead of being rendered from the same generic-aware pass.
Fix
This change adds ordered
type_paramsextraction to the IR for functions, classes, and top-level assigned type aliases, and bumps the IR version to0.2.0. The cache key now includes the IR version so stale cached JSON cannot hide the new schema.On the TypeScript side, the model now carries declaration-level
typeParametersand module-leveltypeAliases, the annotation parser understands~T,~P,P.args,P.kwargs,Unpack[Ts], and module-qualified generic references, and the generator emits declarations directly throughGeneratedCode.declaration.The generator now emits simple unconstrained
TypeVarparameters as declaration-local generics, emits callableParamSpecpacks asP extends unknown[], makes generic classstatic createandfromHandlemethods generic themselves, normalizes same-module dotted generic references to local names, and falls back tounknownorunknown[]when a Python construct cannot be represented safely. Bound or constrainedTypeVars,TypeVarTuple, andUnpackremain conservative by design in this change.The PR also adds regression coverage for IR extraction order, generic alias extraction, declaration output, cache invalidation, and an integration test that generates
.tsand.d.tsfiles for a local generic Python module and typechecks them with a real TypeScript compile.Validation
./node_modules/.bin/tsc --noEmit./node_modules/.bin/vitest --run test/ir_cache_filename.test.ts test/ir_extraction_advanced.test.ts test/generator.test.ts test/integration.test.ts test/type_mapping_enhanced.test.ts test/type_mapping_edge_cases.test.tsNotes
package-lock.jsonchanged because the repository lockfile was out of sync andnpm installwas required before the validation commands could run.