Rewritten

[Dejan Lekic]

On Sunday, 24 May 2026 at 21:50:28 UTC, mitgedanken wrote:
> Wrong link 8)
> Anyway, check out the GitHub link.

There are many issues with the code here are the most serious 
ones:

## 1. Critical: Dangling Pointers Everywhere — `Node*` Points to 
Stack Locals, Not GC Objects

This is the **fundamental design flaw** in the entire module. In 
D, `class` is a reference type.
`Node n = new Node(...)` puts `n` (a reference) on the stack and 
the object on the GC heap.
`&n` gives you a `Node*` — a pointer to the **stack-local 
reference**, not to the GC object.

```d
Tree tree = new Tree();
TreePtr treeRef = &tree;   // pointer to stack-local `tree`
Node op = new Node(treeRef, Symbol("+"), 0);
Node one = new Node(treeRef, Symbol("1"), 0, nullableNode(&op));
//                                                ^^
//                                address of stack-local `op`
```

When `op` goes out of scope, `one._root._node` is a **dangling 
pointer**. The unittests happen
to work because everything is in the same scope, but as library 
code this is
**undefined behavior**.

**The fix:** `Node` is already a reference type. `Node` 
references are already nullable
(`Node n = null`). The entire `NullableNode` struct is 
unnecessary — you could just use
`Node` directly:

```d
// Instead of NullableNode wrapping Node*:
Node _root;   // already nullable, already a reference to the GC 
object
```

---

## 2. Critical: `opBinary("~")` with `NullableNode` Returns a 
Dangling Pointer

```d
auto opBinary(string op : "~")(NullableNode rhs) {
     Node node = new Node(
         this._tree,
         this._symbol ~ rhs.symbol.str,
         this._precedence,
         this._root,
         this._left,
         this._right
     );
     return NullableNode(&node);  // ← node is LOCAL, &node 
dangles after return
}
```

`node` is a local variable. `&node` points to the stack frame. 
After the function returns,
the `NullableNode` holds a dangling pointer — immediate 
use-after-free.

---

## 3. Critical: `NullableNode.toHash` Hashes the Stack Address, 
Not Object Identity

```d
size_t toHash() const pure nothrow @safe {
     return cast(size_t)cast(void*)this._node;
}
```

This hashes the address of the *reference variable on the stack*, 
not the identity of the
GC heap object. Two `NullableNode`s referencing the same `Node` 
object through different
local variables will have different hashes:

```d
Node n1 = new Node(...);
Node n2 = n1;               // same object
auto a = nullableNode(&n1);
auto b = nullableNode(&n2);
assert(a == b);             // FALSE — different stack addresses!
```

`opEquals` also compares with `is` (pointer equality), so this 
same bug means two
references to the same object through different variables are 
considered **not equal**.

---

## 4. Serious: `opEquals` / `toHash` Contract Violation on `Node`

```d
override bool opEquals(Object other) const {
     return (n.repr() == this.repr());   // repr() includes 
children, parent
}

override size_t toHash() const nothrow @safe {
     // only hashes symbol + precedence
}
```

If you mutate a `Node`'s children after inserting it into an 
associative array,
`toHash` stays the same but `opEquals` changes, violating the AA 
invariant. The hash
and equality must agree on what constitutes identity.

---

## 5. Serious: `_setKind` Classifies a Parentless Single-Child 
Node as `Branch`

```d
if (!this.hasParent && this.hasLeft && this.hasRight)
     this._kind = Kind.Root;       // requires BOTH children
else if (this.hasLeft || this.hasRight)
     this._kind = Kind.Branch;     // any children + parent → 
branch
```

A node with no parent and only one child gets classified as 
`Branch`, not `Root`.
This is probably wrong — a node at the top of a tree with a 
single child should be
a `Root`.

---

## 6. Serious: `Tree.repr()` Only Follows One Path

```d
while (curr !is null && !curr.isLeaf) {
     if (curr.hasLeft())
         curr = curr.left.node;     // always prefers left
     else if (curr.hasRight())
         curr = curr.right.node;
}
```

This traverses only the leftmost path, ignoring entire right 
subtrees. A proper tree
representation needs to visit all nodes (pre-order, in-order, 
post-order, or level-order).

---

## 7. Serious: `TreeCrawler.opApply` Only Visits One Level

```d
int opApply(scope int delegate(ref NodePtr) dg) {
     if (auto node = this._state._current) {
         if (node.hasLeft)  { /* visit left child */ }
         if (node.hasRight) { /* visit right child */ }
     }
     return 0;
}
```

This only visits the immediate children of the current node — it 
doesn't recursively
traverse the tree. As a "crawler" it's misleading. It also 
mutates `_state._current`
during iteration, making the crawler stateful in a confusing way.