Hazel reports · ^report(31) · reviews · 2026-10-23 · work in progress
Reviews Still Open
Matthew: look at our open PRs on hazel, find new reviews we haven't integrated yet. Create a report for each one, or a combined one --- with subsections.
(^ticket(572))
Short answers.
- 11 pieces of feedback are open across 4 pull requests: #2608, #2651, #2613, #2583; 3 more are unclear (the evidence below says why).
- 5 of Matthew's open pull requests have feedback from someone else; 31 have none.
- #2608 (Elaborations without whitespace): Alexander's "changes requested" of 10-21 has no reply and no commit after it. He measured secondary (whitespace and comments) still surviving in elaborations across the docs corpus, about 13.5% of the marshaled size, mostly through
Typ.normalize; he offers two fixes, asks for a test over every node kind in the corpus, and four comments on lines are untouched. - #2651 (A module's definition analyzed once): newest, today 11:34. Andrew answered "yes" (a module defined as a function should be an error), and Alexander asks to either make the non-recursive analysis check it is a module, or give modules their own case apart from lets. Also unclear there: the
names_itselfthread, whose promised prototype is now draft #2717 (a module binding as a non-recursive let), which #2651 does not mention. - #2583 (Deep links) and #2613 (Colors): "changes requested" reviews still standing though the work is in. #2583's rework landed in 077a68f9f8 (10-20), unannounced on the PR and not re-reviewed; one question, "Do we want to support tutorial sharing?", has no reply. #2613's blocking point was fixed (28fcabfffc), but its question to Andrew (keep the focused breadcrumb's light green pill?) has no answer.
- #2681 (A value known to have a type is not walked again): a thought, not a request. Alexander: "I'm still thinking about this. I feel like at a certain point we're getting back towards the cast calculus"; Matthew's later comment does not take it up.
Pull request by pull request#
| PR | title | reviewers | open | unclear | answered |
|---|---|---|---|---|---|
| #2608 | Leave the source's whitespace and comments out of elaborations | 7h3kk1d | 5 | 0 | 0 |
| #2651 | Statics: a module binding analyzes its definition once, not twice | 7h3kk1d, disconcision | 3 | 1 | 5 |
| #2613 | Colors: every var() a stylesheet reads is one the theme defines | 7h3kk1d | 2 | 0 | 1 |
| #2583 | Deep links: the URL can say which slide, which panel, and where the cursor is | 7h3kk1d | 1 | 1 | 14 |
| #2681 | A value known to have a type is not walked again for it | 7h3kk1d | 0 | 1 | 0 |
#2608: Leave the source's whitespace and comments out of elaborations#
elab-no-secondary into dev · last commit 2026-09-30 · reviewers: 7h3kk1d · 5 open, 0 unclear, 0 answered
I don't think we want the elaboration specific terminology in here.
open: No commit since 66f789d916 (2026-09-30) and no reply; IdTagged.re still defines unwrap_elab at the PR head.
Left to do: Replace unwrap_elab with unwrap(strip_secondary(x)) and keep elaboration wording out of IdTagged.
🤖 `unwrap_elab` is `unwrap` with the secondary cleared, so it can be written as `unwrap(strip_secondary(x))`. That way `IdTagged` keeps only `strip_secondary`, which is named for what it does. The two call sites in `Statics` become `Exp.unwrap(IdTagged.strip_secondary(uexp))` and `Pat.unwrap(IdTagged.strip_secondary(upat))`, and any explanation about elaborations can go there.
open: Same thread; no commit or reply after it.
Left to do: Make the two Statics call sites Exp.unwrap(IdTagged.strip_secondary(...)) / Pat.unwrap(...).
🤖 This header says why the change was made ("they were about a third of each program sent to the eval worker"). That history belongs in the PR description. The test name already says what is being checked, so I would drop the header, or keep only the first clause.
open: No commit or reply after it.
Left to do: Drop or shorten the history-style header of test/Test_ElabSize.re.
🤖 This is a statics test, so I would move it into `test/statics/`. `Test_Statics_Properties.re` already has a check of the same kind over elaborations (`elab_ids_outside_user_term`), and `Test_Statics_Prelude.re` has a `statics` helper this test could use instead of its own parsing. The group name `ElabSize` also does not fit, since the test checks that no secondary is left and does not measure size.
open: No commit or reply after it.
Left to do: Move the test into test/statics/, reuse the statics helper, and rename the ElabSize group.
🤖 I checked that nothing reads an elaboration's secondary. The only reader is `wrap_with_secondary` in `PreserveExact` mode, and `FastParse` is the only caller that sets that mode. Incremental reuse compares terms with `fast_equal` and `lexeme_trace`, neither of which looks at secondary, so reuse doesn't change either. The inline comments are small. The main issue is below. ### Some nodes still keep their secondary The test checks that no expression or pattern keeps secondary, but its program is only a `let` and some arithmetic. I ran a temporary probe over the 72 programs in `hazel-programs/docs/reference` and `docs/b2t2`. It counted elaborated nodes of every kind that still carry seconda …
open: CHANGES_REQUESTED, never dismissed or followed by APPROVED; no commit since 2026-09-30 and no reply from matthewhammer.
Left to do: Strip secondary on the remaining paths (Typ.normalize, TypFun tpat, TupLabel labels, builtin apps, helpers) or once at the worker boundary, and test every node kind over the docs corpus.
#2651: Statics: a module binding analyzes its definition once, not twice#
statics-module-def-once into dev · last commit 2026-10-21 · reviewers: 7h3kk1d, disconcision · 3 open, 1 unclear, 5 answered
@disconcision idk if you first added the module keyword but should this be an error defining a module to be a function?
open: disconcision answered "yes" on 10-23 09:25, and 7h3kk1d then asked @matthewhammer (10-23 11:34) to either have the non-recursive let analysis also check it is a module, or add a separate case for modules. No reply or commit since.
Left to do: Decide how a module bound to a function becomes a static error (in the non-recursive let analysis, or a separate module case) and implement it.
yes
open: disconcision's "yes" answers 7h3kk1d's question whether defining a module to be a function should be an error; nothing on this PR implements it yet.
Left to do: See the thread: the error is not implemented.
@matthewhammer with that context we either want to let analysis that we're adding for non-recursive to also make sure it's a module or we can consider just making a different case for modules from lets
open: Latest comment on the PR (10-23 11:34); no reply or commit after it.
Left to do: Same as the thread above: make a module bound to a function an error.
🤖 `names_itself` also matches a fixpoint the user wrote. `module m = fix f -> fun x -> … f(x - 1) in m(3)` elaborates to `Let(_, FixF(…), _)` because `fix` itself elaborates to a `FixF`, so the read-back is skipped and the definition is analyzed twice. On modules-ii part 1 with this PR merged, a temporary print in this match fired for every `module … = fix …` I tried (four programs) and not for a `fix` inside a module body. The results match dev, so it's only the speedup that's lost. But the check doesn't actually test what `names_itself` claims to. Telling the two apart from the elaboration's shape is hard. "Skip when the definition is a `fix`" would also skip `module m = fix g -> fun x -> …
unclear: matthewhammer replied 4340772054 that a prototype making a module binding non-recursive was in progress; it exists as draft PR #2717 (a0a781effe "A module binding is a non-recursive let, by an argument to the let", plus fix tests ea23dd4930), but #2651 itself still has names_itself and nothing on #2651 points to #2717.
Left to do: Mention #2717 on the thread, or fold it into this PR.
5 answered
This matches how labeled tuples work. I could see an argument to make incompatible signatures stuck but that's not how much of hazel works so I'm fine with this. In the signature sealing pr I have it would make y private.
answered: matthewhammer raised a singleton-label case in reply; 7h3kk1d chose "Option 2: accept the stuck result" (4340660934) and matthewhammer agreed (4340771544) that the test goes on Modules II part 1.
🤖 Preference, not blocking: these two blocks read as the history of the PR rather than a description of the code. - The second block starts with "Unless…", so it only makes sense as a continuation of the first. One decision ends up described in two stacked comments. - Both sit above `names_itself`, but the first is about the `def_elab_direct` lookup below it, hence "the one case below". - Some of it belongs in the commit message: "not analyzed a second time", "every module binding cost twice what it should", "as before". After merge there's no second time and no before. - "add records exactly the elab_term it returns" doesn't hold under `autolabel_singleton_tuple` (the singleton-label lifti …
answered: fa3eef62ce "Comments say what the code does, one per binding"; matthewhammer replied 4340596675 "Done in fa3eef62ce".
🤖 Option 2: accept the stuck result. A bare value under a module signature should be stuck *and* a static error. modules-ii/1-sig-types already does both: signatures are `Typ.Sig` there, so the singleton-label lift never runs, and `module M : { let x : Int } = 5 in M.x` reports "Expecting type { let x:Int } but got inconsistent type Int" and evaluates to `(5:({ let x:Int })).x`, with or without this PR. On dev the missing error is temporary, so no special case is needed here. For the test you proposed for this program: either pin it as stuck with a note that the static error comes with Modules II part 1, or leave it out of this PR and add it on part 1, where it can assert both the stuck ter …
answered: matthewhammer replied 4340771544: "Agreed, option 2: no special case here. The test for this program goes on Modules II part 1". No pushback.
🤖 My agent complained about this: the name says neither what makes the case special (a member the signature doesn't declare) nor what's asserted (the declared member's value). Without the old comment's comparison with dev, "runs as itself" doesn't tell a reader much when it fails. ```suggestion test_case("Projecting a declared member of a module wider than its signature", `Quick, () => { ```
answered: b5a3e1d90a "The evaluator test's name says what it checks, as Alexander suggested"; reply 4340770782 "Applied in b5a3e1d90a".
🤖 For the record, the 6 → 4 `Measured.WARNING: id … not found` lines on `docs/reference/modules.hz` in the description are a small improvement, not a change in output. `hazel analyze -W` at the PR base (`34f9605ddf`) and at `fa3eef62ce`, three runs each: always 6 and 4, and the rest of the output is byte-for-byte identical. A temporary print of each diagnostic whose id `Measured` can't place shows what they are (id prefixes; the suffix is random per run): | id | error | base | PR | |---|---|---|---| | `4f52` | Expecting Int, got `x=Int, y=Int` in `{ let x = 1; let y = 2 }` | ✓ | ✓ | | `5a52` | Expecting `x=Int`, got `x=x=Int, y=Int` in `x={ … }` | ✓ | ✓ | | `6053` | same as `4f52` | ✓ | — …
answered: Informational ("a small improvement, not a change in output"); asks nothing of this PR. It flags, "not for this PR", that the singleton-label lift records error infos at fresh ids as a separate dev issue; whether that was filed is unknown.
#2613: Colors: every var() a stylesheet reads is one the theme defines#
configuration-undefined-colors into dev · last commit 2026-10-20 · reviewers: 7h3kk1d · 2 open, 0 unclear, 1 answered
🤖 @disconcision this one is a visible change to the probe stack focus bar, so it's your call. `--G5` was never defined on `dev`, so the focused breadcrumb has had no background: just green, underlined text. With this change it gets a light green pill (`--surface-success-soft`). The same goes for the `.unknown.focused` rule at line 434. To see it, probe the `x` in the body of `let f = fun x -> x * 2 in f(1) + f(2)` (Cmd+E) and look at the `f` breadcrumb in the bar. How do you want it? Either drop the `background-color` line in both rules to keep today's look, or keep the pill if that was the original intent.
open: Addressed to @disconcision; no reply from him or matthewhammer. At head 8ca7a15634, style.css still has background-color: var(--surface-success-soft) on .breadcrumb-entry.focused.
Left to do: Get Andrew's call: keep the light green pill or drop background-color in both focused rules.
Great to have this done, sorry it took me so long to look at. The Label issue needs to be fixed and I'll leave the stack focus background up to andrew. Other than that good to go.
open: CHANGES_REQUESTED, never dismissed or followed by APPROVED. Its blocking item (Label) was fixed in 28fcabfffc on 10-20, but nothing on the PR says so, and the stack-focus question it defers to Andrew is unanswered.
Left to do: Ask 7h3kk1d to re-review after 28fcabfffc, and settle the --G5 background with disconcision.
1 answered
🤖 `--surface-label` and `--token-label` are the same color in the theme (`theme-generated.css:199` and `:260`), so this rule draws the label header's text on a band of its own color. To see it, put the cursor on a tuple label like `` `ink` `` in the color scheme slide. On dev the status bar reads "LABEL / Tuple literal". On this branch's preview it's a solid teal bar with only the Γ and the `/` visible. Other sorts don't hit this because the header reuses that sort's pale `--shard-*` tint from the editor, and a label isn't a sort, so it has no shard. Dropping the line keeps the header as it looks on dev and still removes the undefined name: ```suggestion ```
answered: 28fcabfffc "Drop the label header's background, which drew its text on its own color" removes exactly the suggested line from cursor-inspector.css (#cursor-inspector .ci-header.ok.Label).
#2583: Deep links: the URL can say which slide, which panel, and where the cursor is#
deep-links into dev · last commit 2026-10-20 · reviewers: 7h3kk1d · 1 open, 1 unclear, 14 answered
Feature works. Most feedback here is surface level but I think it deserves a once over before I approve. Biggest conceptual issue is we now have the scratch pad url encoding and this name-based documentation slide url encoding and they're completely separate and neither work in tutorial or other modes. Outside of that tests would be nice but possibly not necessary.
open: Never dismissed or followed by an APPROVED; reviewDecision is still CHANGES_REQUESTED. The rework 077a68f9f8 (10-20) postdates it, but no comment on the PR announces that commit and 7h3kk1d has not reviewed since.
Left to do: Tell 7h3kk1d the rework landed in 077a68f9f8 and ask for a re-review (the PR's requested reviewer is still cyrus-).
Do we want to support tutorial sharing?
unclear: No direct reply on GitHub (the 10-15 reply links an external notes page). 077a68f9f8 says outside a documentation slide the copied link "carries the editor's text as a share", so tutorial mode gets a scratchpad snapshot, not a link into the tutorial; whether that settles the question is not stated.
Left to do: Say on the thread whether tutorial links are out of scope or covered by the share fallback.
14 answered
The selection in url seems useful. We already have a way to encode the slides contents into the url. I think doing it via name or via content have different semantics depending on the use case so it's quite hard to think though who would want what/when. This is probably more useful than just screenshots though. If we ever switch to cloudflare and can afford per-build deploys I think it would be great to encode commit sha, selection, and slide content into a url as that's actually reproducible.
answered: matthewhammer replied 09-19 and 09-22 (pointer vs snapshot; merge now, refine later); 7h3kk1d moved on to a code review on 09-23 without pushing back on this point.
This feels like I'm reading a poem or ancient translation of something
answered: 077a68f9f8 message: "Small ones: ... a short header"; DeepLink.re now opens with a short plain header comment.
I don't think it's very clear what `key` is supposed to be for. Based on the comment above it seems like slide names are fuzzy matched. Not sure if it's just for that.
answered: 077a68f9f8: "key renamed loose_name", with a comment saying it compares names loosely.
I think we have List.find_index as of ocaml 5.1
answered: 077a68f9f8: "List.find_index"; DeepLink.re slide_index now uses List.find_index.
In general nothing in this file is super testable. I would probably pass in the query param as an argument here or figure out some other way this can be tested without the browser.
answered: 077a68f9f8: "parse takes the query lookup, so parsing, the editor actions and the URL are tested without a browser (test/Test_DeepLink.re, 21 cases)".
It's not a big deal but I think this would be simpler if you just added the enumerate/show mapping here rather than having to deal with aliases and the default show inside variant.
answered: 077a68f9f8: "The panel names are one switch beside the panel type (SidebarModel), and its inverse."
panel request should be passed into this function
answered: At 077a68f9f8 DeepLink.settings takes (request, settings) and reads request.panel.
The PanelRequest type in this file is the best pattern for this. I think we'd benefit from a type that represents the entire `DeepLink` request. Then separate out the "parsing"/"fetching" from the browser's query params from the logic that actually takes this and applies it to settings and the editor. Ideally I think we would have this somewhat subsume the scratchpad url encoding so there would be a variant that either points to an existing slide by name/index or a new scratchpad by contents. Then the selection/sidebar stuff works as described.
answered: 077a68f9f8 adds DeepLink.request with target Documentation | Scratchpad; "the ?name=&share= encoding now goes through the same type, so ScratchMode no longer reads the URL".
If we did this we should be able to test the validation/parsing logic, the editor actions, and the url generation all separately.
answered: Same commit 077a68f9f8: parse, editor actions and to_params tested separately in test/Test_DeepLink.re, including parse(to_params(r)) == r.
We could probably just define this function in Sidebar.re as a switch to override the default show for the panel key.
answered: 077a68f9f8 puts the panel-name switch in src/web/app/sidebar/SidebarModel.re (not Sidebar.re, but the same idea).
I wonder if this should just be handled via an action in Main.re like the other actions are
answered: 077a68f9f8: "Main schedules FollowDeepLink(request) at startup, handled in Page ... instead of Globals.load reading the URL."
Not blocking, but I'm not sure `slide_name` belongs on `Globals`. It's Documentation-mode state, derived per frame in `Page.main_view`, living on an app-wide record so that one editor feature can read it. (It's also under `// State:` rather than `// Calculated:`.) An alternative: `CodeEditable` only knows the span, so it could raise a global action carrying just that, e.g. `inject_global(CopyDeepLink(span))`, and let the Page-level update, which already knows the mode, the slide and the sidebar, build the URL and write it to the clipboard. That keeps the logic where the data lives and leaves `Globals.Model.t` unchanged. It would also take care of `~linkable: bool`, which is currently `glob …
answered: 077a68f9f8: "slide_name leaves Globals: CodeEditable raises CopyDeepLink({position, text}) and the Page builds the link ... ~linkable leaves the context menu."
It's not clear to me why we default select the enclosing term as opposed to just sharing the caret position. I'm not opposed to it just unclear why
answered: 077a68f9f8: "The copied position is the selection if there is one, else the caret (not the enclosing term)."
@matthewhammer I can't easily comment on the draft page so just leaving stuff here. Lmk if you'd prefer something else. ```reason type target = | Slide(string) /* ?slide= a documentation slide, by name: a pointer */ | Share(string, string); /* ?name=&share= a scratchpad's contents: a snapshot */ ``` I think this would be better with constructors like `Documentation` and `Scratchpad`. Also maybe records instead of tuples for the stuff with multiple fields. Maybe the AI doesn't care but my human :brain: can't handle it. ```reason type request = { target: option(target), panel: option(panel_request), /* ?panel= */ position: option(p …
answered: 077a68f9f8 uses constructors Documentation(string) and Scratchpad({name, text}) and a place = {target, position} type so a position cannot exist without a target ("A position only means something inside a target").
#2681: A value known to have a type is not walked again for it#
ascriptions-has-type into dev · last commit 2026-10-20 · reviewers: 7h3kk1d · 0 open, 1 unclear, 0 answered
I'm still thinking about this. I feel like at a certain point we're getting back towards the cast calculus which might be much more maintainable with 🤖 help.
unclear: Not a change request: the reviewer is undecided. matthewhammer's later comment (10-22) proposes a depth-1 limit for perf but does not respond to the cast-calculus point, and there is no reviewer reply since.
Left to do: A response to whether the has-type memo is drifting back towards a cast calculus, and a decision from 7h3kk1d.
How this was put together#
For each of Matthew's open pull requests on hazelgrove/hazel (2026-10-23), every review (with its body), every comment on a line and every comment, from anyone other than Matthew and the bots, read through GitHub's API; each judged against the commits after it (by committer date) and the replies: answered when a later commit or a reply plainly settles it, open when nothing does or the reply promised work not done, unclear otherwise, with the evidence for each. Pull requests with no feedback from others: #2490, #2491, #2536, #2577, #2592, #2597, #2603, #2605, #2606, #2610, #2614, #2616, #2635, #2650, #2652, #2661, #2671, #2672, #2673, #2675, #2676, #2678, #2689, #2690, #2691, #2697, #2698, #2700, #2711, #2717, #2719. The data is 031/data/pr-reviews.json.
The data#
- The numbers, for programs too: every table in this report, and what its numbers measure, is also in this page as JSON, in its
<script type="application/json" id="report-data">element, so a reader or an agent that fetches the page alone has the dataset. The same, with any raw files that survive, is in031/data/.