Fix CLI scripts calling the wrong function, and the errors that hid it - #5715
Merged
Merged
Conversation
StachuDotNet
marked this pull request as ready for review
August 19, 2026 16:44
StachuDotNet
marked this pull request as draft
August 19, 2026 16:47
StachuDotNet
force-pushed
the
hash-after-resolution
branch
3 times, most recently
from
August 19, 2026 20:02
93f95b5 to
86b7bc7
Compare
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
force-pushed
the
hash-after-resolution
branch
from
August 19, 2026 20:09
86b7bc7 to
0aee8cd
Compare
StachuDotNet
marked this pull request as ready for review
August 19, 2026 20:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Worse than the error: a function whose signature says
TAwould accept aTBand 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.stabilizeonce 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
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.EphemeralPackagesholds hash-to-location for declarations that exist in the process but not the store, andPackageManager.ptfalls back to it on the three reverse lookups.A fallback, not an override. One hash can be bound to many names and
pickLocationbreaks ties by shortest path, so consulting the registry first let a throwawaytype MyErr = | BadFormatrenameStdlib.Int.ParseErrorfor 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 ownerevaluses) is scaffolding rather than a name anything can be reached by, so both name builders now drop it alongsideTests.3. Errors named the wrong one of several valid names
Why.
Stdlib.Int.ParseError,Stdlib.Float.ParseErrorandStdlib.Uuid.ParseErrorare 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
ValueTyperather than instead of it. Each is authoritative about a different thing: theValueTypehas been through the type symbol table, so it knowsList<'a>came out asList<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:
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 innotes/hashing-unresolved-refs-collision-2026-08-19.md, and is the opposite of this.