picatz / picatz/flowstate

tools/wallclock: decide whether the analyzer should type-check, since the false negatives that remain are the ones a parse cannot reach

Open
#2,033 0 comments 0 reactions 0 assignees View on GitHub

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 synctest bubble 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.goAnalyze'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

  1. 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.
  2. Type-check the main module, parse the rest. Load the main module with go/packages and 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.
  3. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.