Skip to content

Fix/zig string safety#721

Draft
tarun1sisodia wants to merge 3 commits intometacall:developfrom
tarun1sisodia:fix/zig-string-safety
Draft

Fix/zig string safety#721
tarun1sisodia wants to merge 3 commits intometacall:developfrom
tarun1sisodia:fix/zig-string-safety

Conversation

@tarun1sisodia
Copy link
Copy Markdown

Description

This PR fixes a critical bug in the MetaCall Zig port where string return types were being freed before they could be used by the caller. It introduces a Value(T) wrapper pattern that provides manual control over resource deinitialization (RAII-like), ensuring memory safety for multi-language calls.

Key Changes:

  • root.zig: Added a Value(T) struct with get() and deinit() methods. Updated metacall to return this wrapper for strings while maintaining direct returns for primitive types.
  • Portability: Replaced hardcoded absolute paths in build.zig and integrated.zig with repo-relative paths (../../../build and src/tests/*).
  • Zig Compatibility: Fixed a reflective type check error (.Array vs .array) to support Zig 0.16.0-dev.
  • Robustness: Added integrated stress tests (10,000 calls) and edge case validation for empty strings.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (API change for string returns)

Checklist:

  • I have performed a self-review of my own code.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective.
  • I have tested the tests pass (zig build test --summary all).
  • I have run zig fmt and followed style guidelines.

Copilot AI review requested due to automatic review settings March 25, 2026 05:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a lifetime bug in the Zig port where string return values from metacall could be freed before the caller uses them by introducing a manual-deinit wrapper for string returns, plus updates to Zig build/test portability.

Changes:

  • Introduces a Value(T) wrapper and routes string return types through it to avoid premature metacall_value_destroy.
  • Updates the integrated Zig test to use repo-relative loader paths and adds stress/edge-case coverage for string returns.
  • Adds a build.zig for the Zig port and adjusts .gitignore for Zig cache directories.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
source/ports/zig_port/src/root.zig Adds Value(T) wrapper and changes metacall return type behavior for strings.
source/ports/zig_port/src/tests/integrated.zig Uses relative test paths and adds stress/empty-string checks for string returns.
source/ports/zig_port/build.zig Adds Zig build/test steps for the port (currently mis-wired as noted in comments).
source/ports/zig_port/.gitignore Updates Zig cache ignore entry to .zig-cache.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread source/ports/zig_port/build.zig
Comment thread source/ports/zig_port/src/tests/integrated.zig Outdated
Comment thread source/ports/zig_port/src/root.zig Outdated
Comment thread source/ports/zig_port/src/root.zig
Comment thread source/ports/zig_port/build.zig Outdated
@tarun1sisodia
Copy link
Copy Markdown
Author

test

above you can see the compiled and tests passed.

and here are the paths i defined
1
both images are not using hardcoded path
2

@viferga
Copy link
Copy Markdown
Member

viferga commented Mar 27, 2026

Do you think it's ready to merge then? I would like to add CI even if it's after this PR.

@tarun1sisodia
Copy link
Copy Markdown
Author

Do you think it's ready to merge then? I would like to add CI even if it's after this PR.

Yes ofcourse of you want them merge it or add ci for zig

@viferga
Copy link
Copy Markdown
Member

viferga commented Mar 31, 2026

Do you think it's ready to merge then? I would like to add CI even if it's after this PR.

Yes ofcourse of you want them merge it or add ci for zig

I prefer if you add it.

@viferga
Copy link
Copy Markdown
Member

viferga commented Apr 17, 2026

@tarun1sisodia can you add zig to the ci? I will leave it as draft meanwhile, check out tools/metacall-environment.sh

@viferga viferga marked this pull request as draft April 17, 2026 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants