Skip to content

fix: array.pop() returns 0 instead of proper none for empty arrays #41

@mjm918

Description

@mjm918

Summary

The current option representation in naml uses 0 for none and the value directly for some(value). This makes it impossible to distinguish between:

  • A valid value of 0 (e.g., some(0))
  • The absence of a value (none)

Current Behavior

array.pop()

var arr: [int] = [0, 1, 2];
var val1: option<int> = arr.pop();  // Returns 2 ✓
var val2: option<int> = arr.pop();  // Returns 1 ✓
var val3: option<int> = arr.pop();  // Returns 0 ✓
var val4: option<int> = arr.pop();  // Returns 0 ✗ (should be none!)

// Cannot distinguish val3 from val4!
println(val3 ?? -1);  // Prints 0
println(val4 ?? -1);  // Prints 0 (should print -1)

as? fallible cast

var valid: option<int> = "0" as? int;      // Returns 0 (some(0))
var invalid: option<int> = "hello" as? int; // Returns 0 (none)

// Cannot distinguish valid from invalid!
println(valid ?? -1);    // Prints 0
println(invalid ?? -1);  // Prints 0 (should print -1)

Expected Behavior

// pop()
var arr: [int] = [0];
var val1: option<int> = arr.pop();  // Returns some(0)
var val2: option<int> = arr.pop();  // Returns none
println(val1 ?? -1);  // Should print 0
println(val2 ?? -1);  // Should print -1

// as?
var valid: option<int> = "0" as? int;       // Returns some(0)
var invalid: option<int> = "hello" as? int; // Returns none
println(valid ?? -1);    // Should print 0
println(invalid ?? -1);  // Should print -1

Root Cause

Options are represented as simple i64 values where 0 means none. This conflicts with 0 being a valid value.

Example from namlc/src/runtime/array.rs:

pub unsafe extern "C" fn naml_array_pop(arr: *mut NamlArray) -> i64 {
    if (*arr).len == 0 {
        return 0;  // Bug: 0 is indistinguishable from popping the value 0
    }
    // ...
}

Proposed Solution

Use a consistent tagged option representation across the codebase:

  • Allocate 16-byte struct: { tag: i32, value: i64 }
  • tag = 0 means none
  • tag = 1 means some(value)

This requires updating:

  1. Runtime functions that return option<T>
  2. The ?? operator to check tag field instead of null check
  3. The some() expression (already uses this format)
  4. Methods like .is_some(), .is_none(), .unwrap()
  5. The as? fallible cast operator

Affected Functions

  • naml_array_pop() - returns 0 for empty array
  • naml_array_get() - may have similar issue for out-of-bounds
  • naml_map_get() - may have similar issue for missing keys
  • as? fallible cast - returns 0 for parse failure
  • Any other function returning option<T>

Impact

Until this is fixed, option<T> cannot reliably hold the value 0 (or 0.0 for floats, false for bools, empty string for strings, etc.).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions