Skip to content

Fix CLI scripts calling the wrong function, and the errors that hid it - #5715

Merged
StachuDotNet merged 1 commit into
darklang:mainfrom
StachuDotNet:hash-after-resolution
Aug 19, 2026
Merged

Fix CLI scripts calling the wrong function, and the errors that hid it#5715
StachuDotNet merged 1 commit into
darklang:mainfrom
StachuDotNet:hash-after-resolution

Conversation

@StachuDotNet

@StachuDotNet StachuDotNet commented Aug 19, 2026

Copy link
Copy Markdown
Member

Three fixes on one code path. The first is the bug; the other two are why nobody saw it.

1. Two declarations could share a hash, and one silently won

type TA = { a: String }
type TB = { b: Int }
let takesA (r: TA) : Int = 7
let takesB (r: TB) : Int = 7
- takesA (TA { a = "x" })  ->  rejected, with a message naming two hashes
+ takesA (TA { a = "x" })  ->  7

Worse than the error: a function whose signature says TA would accept a TB and run.

Why. A script's declarations are grafted into the package manager keyed by content hash. Pass 1 hashes them before their references resolve, and an unresolved reference serialises without its name, so these two hashed identically. The graft kept one; calls to the other reached the survivor.

Fix. Pass 1 stamps location placeholders, which cannot collide. Real content hashes come from HashStabilization.stabilize once pass 2 has resolved everything. That is the pipeline the package path already runs, SCC batching included; the script path was an ad-hoc reimplementation of it with the placeholder step missing.

Hashing after resolution, rather than keeping the placeholder as the identity, is what keeps content addressing working across the boundary: a script type shaped like a package type is still that type, and renaming still changes nothing.

2. A script's own declarations printed as hashes

- d485a575...'s 1st parameter `t` expects 1bd4987a..., but got f8fd3b75...
+ describe's 1st parameter `t` expects Celsius, but got Fahrenheit

This is why (1) read as an ordinary type mismatch rather than as the wrong function being called.

Why. Script declarations are never written to the store, and the CLI renders the error after the executor holding them is gone, so the pretty-printer's hash-to-name lookup missed every time.

Fix. LibDB.EphemeralPackages holds hash-to-location for declarations that exist in the process but not the store, and PackageManager.pt falls back to it on the three reverse lookups.

A fallback, not an override. One hash can be bound to many names and pickLocation breaks ties by shortest path, so consulting the registry first let a throwaway type MyErr = | BadFormat rename Stdlib.Int.ParseError for the rest of the process.

Two naming problems surfaced once the hashes were gone, both fixed: scripts declared their contents under a module named after the file, and CliScript (like the empty owner eval uses) is scaffolding rather than a name anything can be reached by, so both name builders now drop it alongside Tests.

3. Errors named the wrong one of several valid names

let f (e: Darklang.Stdlib.Int.ParseError) : Int = 1
- f's 1st parameter `e` expects Darklang.Stdlib.Float.ParseError
+ f's 1st parameter `e` expects Darklang.Stdlib.Int.ParseError

Why. Stdlib.Int.ParseError, Stdlib.Float.ParseError and Stdlib.Uuid.ParseError are all | BadFormat, so they are one type with three names, and choosing from the hash was a guess. Prefix-distance scoring cannot help: a caller in neither module scores zero against both and falls through to a coin toss.

Fix. The five errors that print a declared type now carry the type reference as written, next to the ValueType rather than instead of it. Each is authoritative about a different thing: the ValueType has been through the type symbol table, so it knows List<'a> came out as List<Int>, while the declaration knows the names. The renderer walks the two together, taking shape from one and names from the other, and drops the declaration for any subtree where they stop lining up.

Walking, rather than reading one name off the top, is what handles nesting:

- expects (Darklang.Stdlib.Float.ParseError * Darklang.Stdlib.Float.ParseError)
+ expects (Darklang.Stdlib.Int.ParseError * Darklang.Stdlib.Uuid.ParseError)

One hash, two positions, two names. Record fields, enum fields and record updates get the same treatment.

Names never reach the hash. This is originalName, which already rides alongside the resolution in PT and RT. Putting a name into the hash is the fix rejected in notes/hashing-unresolved-refs-collision-2026-08-19.md, and is the opposite of this.

@StachuDotNet
StachuDotNet marked this pull request as ready for review August 19, 2026 16:44
@StachuDotNet
StachuDotNet marked this pull request as draft August 19, 2026 16:47
@StachuDotNet
StachuDotNet force-pushed the hash-after-resolution branch 3 times, most recently from 93f95b5 to 86b7bc7 Compare August 19, 2026 20:02
Three fixes on one code path. The first is the bug; the other two are why nobody saw it.

---

## 1. Two declarations could share a hash, and one silently won

```dark
type TA = { a: String }
type TB = { b: Int }
let takesA (r: TA) : Int = 7
let takesB (r: TB) : Int = 7
```

```diff
- takesA (TA { a = "x" })  ->  rejected, with a message naming two hashes
+ takesA (TA { a = "x" })  ->  7
```

Worse than the error: a function whose signature says `TA` would accept a `TB` and run.

**Why.** A script's declarations are grafted into the package manager keyed by content hash. Pass 1 hashes them before their references resolve, and an unresolved reference serialises without its name, so these two hashed identically. The graft kept one; calls to the other reached the survivor.

**Fix.** Pass 1 stamps location placeholders, which cannot collide. Real content hashes come from `HashStabilization.stabilize` once pass 2 has resolved everything. That is the pipeline the package path already runs, SCC batching included; the script path was an ad-hoc reimplementation of it with the placeholder step missing.

Hashing after resolution, rather than keeping the placeholder as the identity, is what keeps content addressing working across the boundary: a script type shaped like a package type is still that type, and renaming still changes nothing.

## 2. A script's own declarations printed as hashes

```diff
- d485a575...'s 1st parameter `t` expects 1bd4987a..., but got f8fd3b75...
+ describe's 1st parameter `t` expects Celsius, but got Fahrenheit
```

This is why (1) read as an ordinary type mismatch rather than as the wrong function being called.

**Why.** Script declarations are never written to the store, and the CLI renders the error after the executor holding them is gone, so the pretty-printer's hash-to-name lookup missed every time.

**Fix.** `LibDB.EphemeralPackages` holds hash-to-location for declarations that exist in the process but not the store, and `PackageManager.pt` falls back to it on the three reverse lookups.

A fallback, not an override. One hash can be bound to many names and `pickLocation` breaks ties by shortest path, so consulting the registry first let a throwaway `type MyErr = | BadFormat` rename `Stdlib.Int.ParseError` for the rest of the process.

Two naming problems surfaced once the hashes were gone, both fixed: scripts declared their contents under a module named after the file, and `CliScript` (like the empty owner `eval` uses) is scaffolding rather than a name anything can be reached by, so both name builders now drop it alongside `Tests`.

## 3. Errors named the wrong one of several valid names

```dark
let f (e: Darklang.Stdlib.Int.ParseError) : Int = 1
```

```diff
- f's 1st parameter `e` expects Darklang.Stdlib.Float.ParseError
+ f's 1st parameter `e` expects Darklang.Stdlib.Int.ParseError
```

**Why.** `Stdlib.Int.ParseError`, `Stdlib.Float.ParseError` and `Stdlib.Uuid.ParseError` are all `| BadFormat`, so they are one type with three names, and choosing from the hash was a guess. Prefix-distance scoring cannot help: a caller in neither module scores zero against both and falls through to a coin toss.

**Fix.** The five errors that print a declared type now carry the type reference as written, next to the `ValueType` rather than instead of it. Each is authoritative about a different thing: the `ValueType` has been through the type symbol table, so it knows `List<'a>` came out as `List<Int>`, while the declaration knows the names. The renderer walks the two together, taking shape from one and names from the other, and drops the declaration for any subtree where they stop lining up.

Walking, rather than reading one name off the top, is what handles nesting:

```diff
- expects (Darklang.Stdlib.Float.ParseError * Darklang.Stdlib.Float.ParseError)
+ expects (Darklang.Stdlib.Int.ParseError * Darklang.Stdlib.Uuid.ParseError)
```

One hash, two positions, two names. Record fields, enum fields and record updates get the same treatment.

Names never reach the hash. This is `originalName`, which already rides alongside the resolution in PT and RT. Putting a name *into* the hash is the fix rejected in `notes/hashing-unresolved-refs-collision-2026-08-19.md`, and is the opposite of this.
@StachuDotNet
StachuDotNet force-pushed the hash-after-resolution branch from 86b7bc7 to 0aee8cd Compare August 19, 2026 20:09
@StachuDotNet
StachuDotNet marked this pull request as ready for review August 19, 2026 20:24
@StachuDotNet
StachuDotNet merged commit 14c4158 into darklang:main Aug 19, 2026
6 checks passed
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.

1 participant