tools/wallclock: decide whether the analyzer should type-check, since the false negatives that remain are the ones a parse cannot reach
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Recorded from #1989, which added the poll ratchet and then spent nine review rounds closing thirteen false negatives in the analyzer. The ones that are left share a property the closed ones did not, and it is worth deciding about deliberately rather than discovering it a fourteenth time.
Problem or observed behavior
tools/wallclock parses rather than builds. That is a real choice with a real payoff — no type information means no build, which is what keeps the plugin modules' tests in scope, since they live in separate modules. The cost is that a wait whose identity is a type-level fact cannot be seen at all.
Every one of the thirteen findings on #1989 was a false negative: the count stays green while the tree gets worse, which is the one direction a ratchet cannot survive. Twelve were syntactic and are now closed — receiver shapes, formatted spellings, four kinds of shadow, parenthesized callees and binders. Closing them taught the lesson that is now written into the file: enumerating semantic cases invites the next one, which is why the poll matcher ended up discarding receiver analysis and the per-file import gate altogether rather than growing another case.
What remains is a different class. These are not shapes anyone forgot; they are facts not present in the file being parsed:
sleep := time.Sleep; sleep(d)— the callee's identity lives in an earlier binding- a testify assertion object returned by a helper in another file and called without ever being named
- a closure inside a
synctestbubble that escapes and runs after the bubble returns
The third was reported and the remedy declined, with the reasoning recorded in Analyze's doc and in the review thread: counting every wait in a nested closure would mis-flag go func(){ time.Sleep(d) }() inside a bubble, which is the canonical synctest idiom and exactly the pattern #1706 exists to encourage. The ratchet would then penalise correct conversions, which is worse than the miss. I measured before deciding — no wait in the tree currently sits in that position — but that measurement has a shelf life.
Evidence
tools/wallclock/analyze.go—Analyze's doc states the criterion and both signs of its error;unparen's doc records why parentheses were peeled rather than enumerated.- #1989's thirteen findings, all false negatives, across nine rounds: r3998424149 through r3998625496.
- The tally never moved — 29 sleeps and 103 polls throughout — so none of the thirteen was costing a miscount when it was found. That is the value of the exercise and also the reason the remaining ones are easy to leave: they cost nothing today.
Desired outcome
A recorded decision on whether the plugin modules' test coverage is worth more than closing the type-level class, so the next person meets an answer instead of the tradeoff.
Suggested approaches
- Stay syntactic. Accept the class as unreachable, and make the doc say so as a decision rather than as an observation. Cheapest, and defensible given that the shapes are contrived and none exists today.
- Type-check the main module, parse the rest. Load the main module with
go/packagesand keep the current parse for the plugin modules, with the table noting which files got which treatment. Closes the class where nearly all the tests live, at the cost of two code paths in one tool. - Type-check everything. Requires building the plugin modules, which is the thing the current design deliberately avoids.
Acceptance criteria
- The tool's doc states which classes of wait it can and cannot see, as a decision with its reasoning, not as a list of known bugs.
- If option 2 or 3 is taken: a regression test for at least one type-level spelling (
sleep := time.Sleep; sleep(d)is the clearest), mutation-checked like the thirteen before it. - If option 1 is taken: the decision names what would reopen it — most obviously, the first real wait in the tree that lands in one of these shapes.
- Either way the tally stays at 29 / 103 unless a real wait is newly found, in which case the table records it with a reason.
Constraints and dependencies
Distinct from #1977, which is about the ratchet doc's per-entry bullets disagreeing with its three-reason rule, and from #1706, which is about converting the remaining polls rather than counting them. A change here touches only the analyzer and its tests; the tables move only if the tool starts seeing something it did not.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Read tools/wallclock/analyze.go, especially Analyze's documentation and the existing analyzer tests covering the prior false negatives. Compare the three proposed analysis strategies and their effect on plugin-module coverage. Done means the documentation records a decision and its reopening condition; if type-checking is chosen, add the stated regression test and preserve the 29 / 103 tally unless a real wait is found.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- devtools
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100