Make the Builder declarative rather than holding live Value objects by buke · Pull Request #731 · buke/quickjs-go

buke · 2026-04-04T10:58:10Z

No description provided.

Add PropertyValue and PropertyLiteral APIs and keep Property/StaticProperty as compatibility bridges via context-bound specs. Materialize static and instance property specs in build/constructor paths with fail-closed validation and explicit cleanup. Expand class ValueSpec tests for success, invalid instance/static specs, non-legacy static specs, and post-build nil-spec mutation. Validated with go test ./...

Add StaticPropertyValue and StaticPropertyLiteral to ClassBuilder for spec-based static property definitions. Keep StaticProperty as a compatibility bridge via context-bound ValueSpec and extend class property tests for new APIs.

Mark legacy Export/Property APIs as context-bound convenience APIs and recommend declarative ValueSpec usage. Add matching declarative ModuleBuilder and ClassBuilder examples to README.md and README_zh-cn.md.

Validate ModuleBuilder export specs during Build validation so nil specs fail early instead of during import. Fix README.md and README_zh-cn.md declarative module section indentation to render headings and code blocks correctly.

…rors Store ModuleBuilder/ClassBuilder snapshots at Build time so post-build mutations do not affect runtime initialization. Add detailed materialization error context for module/class spec failures and mark legacy pointer-based convenience APIs as deprecated. Expand module/class branch coverage for snapshot mutation and materialization edge cases; verified go test, race, and 100% coverage.

codecov · 2026-04-04T11:00:09Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (b3c91bc) to head (62d7a46).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #731    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           16        17     +1     
  Lines         3086      3238   +152     
==========================================
+ Hits          3086      3238   +152

Files with missing lines	Coverage Δ
class.go	`100.00% <100.00%> (ø)`
class_bridge.go	`100.00% <100.00%> (ø)`
module.go	`100.00% <100.00%> (ø)`
module_bridge.go	`100.00% <100.00%> (ø)`
value.go	`100.00% <100.00%> (ø)`
value_spec.go	`100.00% <100.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3c91bc...62d7a46. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gemini-code-assist

Code Review

This pull request introduces a declarative ValueSpec API for ModuleBuilder and ClassBuilder, enabling reusable and context-independent definitions for exports and properties. Key changes include the addition of ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods, alongside internal snapshotting of builders to ensure consistency during initialization. Feedback highlights critical concerns regarding potential double-free risks during value materialization, memory leaks from unreleased C strings in the class creation logic, and the destructive impact of the current implementation on legacy context-bound APIs. Additionally, the use of fmt.Printf for error reporting should be replaced with standard error handling or logging.

class.go

class_bridge.go

class.go

module_bridge.go

class.go

buke · 2026-04-04T11:24:41Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative ValueSpec API for ModuleBuilder and ClassBuilder, enabling reusable module and class definitions by materializing JavaScript values at build-time. Key changes include the addition of ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods, along with internal snapshotting of builder states to ensure consistency. The documentation has been updated to reflect these recommended patterns. Feedback highlights a redundancy between LiteralSpec and MarshalSpec, suggesting they be consolidated to simplify the codebase.

value_spec.go

Remove LiteralSpec and keep MarshalSpec as the single materialization spec. Keep ExportLiteral/PropertyLiteral/StaticPropertyLiteral for readability while delegating to MarshalSpec internally. Update tests to use MarshalSpec directly where LiteralSpec was referenced.

buke · 2026-04-04T11:45:46Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative ValueSpec API for ModuleBuilder and ClassBuilder, enabling reusable definitions that materialize JavaScript values at build or instantiation time. Key additions include ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods, while older context-bound methods are deprecated. The implementation incorporates snapshotting to prevent post-build mutations from affecting registered entities and adds a borrowed flag to the Value struct for better reference management. Review feedback suggests simplifying redundant nil checks in the deprecated Property and StaticProperty methods.

class.go

Reorder Property/StaticProperty to compute spec first and return once. Behavior remains unchanged for nil and non-nil value paths.

buke · 2026-04-04T11:54:05Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative ValueSpec API for ModuleBuilder and ClassBuilder, enabling reusable definitions that materialize JavaScript values at build-time or instantiation-time. This replaces the reliance on long-lived, context-bound JSValue pointers with more flexible specs like MarshalSpec and FactorySpec. Key changes include the addition of ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods, alongside internal snapshotting of builders to prevent post-build mutations from affecting module or class initialization. Review feedback suggests enhancing the documentation examples with expected output and generalizing a specific error message in the ValueSpec implementation.

README.md

README_zh-cn.md

value_spec.go

Change contextValueSpec nil error text to a generic context-bound wording. Update EN/ZH declarative property examples to print the evaluated result with expected output comments for easier verification.

buke · 2026-04-04T12:03:13Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative API for defining module exports and class properties through a new ValueSpec interface, allowing for reusable and context-independent definitions. Key changes include the addition of ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods, along with snapshotting mechanisms in ModuleBuilder and ClassBuilder to ensure consistency after the build process. A critical issue was identified in the handling of legacy module exports in module_bridge.go, where the consumption of JSValue by JS_SetModuleExport could lead to a use-after-free bug; it is recommended to duplicate the value for legacy exports to maintain the validity of the original Go handle.

module_bridge.go

Clarify that legacy export wrappers are borrowed non-owning aliases whose lifetime is held by the module export slot. Add boundary coverage for read-after-GC and repeated Free calls on legacy aliases.

buke · 2026-04-04T12:14:12Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative API for defining module exports and class properties through a new ValueSpec interface, allowing for reusable definitions that are materialized at build time. Key changes include the addition of ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods, the deprecation of older context-bound APIs, and the implementation of builder snapshotting to ensure consistency during initialization. Additionally, a borrowed state was added to the Value struct to correctly manage reference counting for legacy exports. Review feedback highlights a potential double-free vulnerability in module_bridge.go when an export fails and suggests clarifying the immutability requirements for ValueSpec implementations due to shallow copying in builder snapshots.

module_bridge.go

class.go

Invalidate all materialized export handles when JS_SetModuleExport fails to avoid stale references. Add regression coverage for non-legacy FactorySpec failure path and document ValueSpec immutability expectations for shallow builder snapshots.

buke · 2026-04-04T12:31:08Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative ValueSpec API for ModuleBuilder and ClassBuilder, enabling reusable module and class definitions by materializing JavaScript values on demand. Key changes include the addition of MarshalSpec and FactorySpec, internal snapshotting of builders for consistency, and a borrowed handle state to manage reference counting. The review feedback highlights a critical stability concern: potential panics within user-provided Materialize functions are currently unhandled, which could lead to program crashes when triggered from C-exported proxy functions. Implementing panic recovery in these sections is recommended to ensure the Go-C boundary remains stable.

class_bridge.go

module_bridge.go

class.go

Add a shared safe materialization helper that converts panics into errors. Use it in module init, class constructor proxy, and class static property build paths to protect Go-C boundaries. Add regression tests for module factory panic, instance property panic, and static property panic handling.

buke · 2026-04-04T12:42:26Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative ValueSpec API for ModuleBuilder and ClassBuilder, allowing for reusable definitions by materializing JavaScript values at build or instantiation time instead of storing long-lived pointers. Key changes include the implementation of MarshalSpec and FactorySpec, builder snapshotting to prevent post-build mutation issues, and a new 'borrowed' state for Values to handle legacy context-bound exports. Review feedback identified critical memory leaks in the error handling paths of class.go, class_bridge.go, and module_bridge.go, where materialized values were not correctly registered for cleanup before potential validation failures.

class.go

class_bridge.go

module_bridge.go

Defer module builder-handle cleanup immediately after resolving the builder to cover all early-return paths. Track only same-context materialized values for defensive cleanup in class and class-constructor materialization flows. Add regression coverage to ensure module init early failures do not leak builder handles.

buke · 2026-04-04T13:00:11Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative API for defining module exports and class properties using a new ValueSpec interface, which allows for reusable definitions via MarshalSpec and FactorySpec. Key changes include the deprecation of context-bound Export and Property methods, the implementation of builder snapshotting to ensure immutability, and the addition of a borrowed flag to the Value struct for improved reference management. Review feedback highlighted a potential memory leak in module initialization due to incorrect ownership assumptions with JS_SetModuleExport and suggested aligning nil handling in ModuleBuilder.Export with ClassBuilder.Property for more consistent build-time validation.

module_bridge.go

module.go

Make deprecated ModuleBuilder.Export(name, nil) produce a nil spec so validation fails at Build-time. Add regression coverage for legacy Export nil behavior and update bridge expectations to match early validation.

buke · 2026-04-04T13:11:02Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative API for exporting values in modules and defining properties in classes using a new ValueSpec interface. This change allows for reusable definitions that are materialized at runtime or initialization rather than being bound to a specific context at definition time. Key additions include ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral, while older context-bound methods have been deprecated. The implementation includes a snapshotting mechanism during the build process to ensure that post-build mutations to builders do not affect registered modules or classes. Review feedback highlights that the snapshots perform shallow copies of ValueSpec interfaces, suggesting that the immutability of these implementations must be strictly maintained to avoid unexpected side effects.

class.go

module.go

Add explicit notes in module/class Build docs that passed spec object state must not be modified after Build.

buke · 2026-04-04T13:23:34Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a declarative API for defining module exports and class properties via a new ValueSpec interface, enabling reusable definitions that materialize within a specific context. It adds ExportValue, ExportLiteral, PropertyValue, and PropertyLiteral methods to ModuleBuilder and ClassBuilder, while deprecating the older context-bound versions. The system now snapshots builder configurations during the Build process to prevent post-build mutations and includes a borrowed state for Value handles to correctly manage QuickJS reference counting. I have no feedback to provide.

Add a ValueSpec invalid-spec case using contextValueSpec{} to exercise the nil branch in contextValueSpec.Materialize. This restores exact coverage to 100% for value_spec.go and overall coverage profile.

buke added 6 commits April 4, 2026 17:48

Introduce ValueSpec and module export materialization

c9c8ee9

feat(class): add static ValueSpec property APIs

abffbf3

Add StaticPropertyValue and StaticPropertyLiteral to ClassBuilder for spec-based static property definitions. Keep StaticProperty as a compatibility bridge via context-bound ValueSpec and extend class property tests for new APIs.

docs(readme): sync declarative builder guidance in EN/ZH

09bd9f7

Mark legacy Export/Property APIs as context-bound convenience APIs and recommend declarative ValueSpec usage. Add matching declarative ModuleBuilder and ClassBuilder examples to README.md and README_zh-cn.md.

refactor(builder): fail fast on nil module specs

43f0a4d

Validate ModuleBuilder export specs during Build validation so nil specs fail early instead of during import. Fix README.md and README_zh-cn.md declarative module section indentation to render headings and code blocks correctly.