reanalyze: duplicated batch/reactive pipelines have silently drifted; the editor runs the untested one

未关闭
#8,647 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

还没有人认领这个 Issue。

评估

难度
5/5
预计耗时
一周以上
新手友好度
35/100
Issue 类型
缺陷
描述清晰度
基本清楚
活跃度
活跃
技术栈
ocaml

调研方向

从 DeadType.ml、ReactiveTypeDeps.ml 和 reanalyze_server.ml:309 路径入手,比较 re-export 依赖的处理方式。运行 make -C tests/analysis_tests test,然后检查非默认的 reactive targets 和 ci.yml:201。当已签入的 corpus 包含匹配的 batch 和 reactive finding 集(包括 TypeReexport fixtures),并且 CI 检查这种一致性时,即表示完成。

由索引模型根据 Issue 内容生成。

描述

reanalyze computes dead-code liveness twice: a batch pipeline (Dead_type, Liveness, the frozen stores) and a reactive one (Reactive_type_deps, Reactive_liveness, ...). They are meant to agree. They do not, and the way they drifted is worth recording, because the same thing nearly happened again last week.

Which path do users get?

Not the one CI tests.

(* reanalyze_server.ml:309 *)
(* Editor mode only: the server always behaves like `reanalyze -json`. *)
Cli.reactive := true;   (* Force reactive mode in server. *)

Cli.reactive defaults to false (cli.ml:29), so:

who mode
editor, via rescript-tools reanalyze-server reactive
rescript-tools reanalyze on the CLI batch
CI batch

ci.yml:201 is the only place the suite runs: make -C tests/analysis_tests test, which reaches tests-reanalyze/deadcode/test.sh — batch, no -reactive. The two targets that would touch reactive, test-reactive-server and test-reanalyze-order-independence, are not default targets and appear in no workflow; their Makefile says "not run by default (takes longer)". So interactive users run the untested path.

The bug

On master, on the checked-in corpus:

batch     405 issues
reactive  412 issues

All 7 extra reactive findings are false positives on type re-export equations (type y = x = {...}):

TypeReexport.res:27:5-21   UseReexported.originalType.usedField is a record label never used to read a value
TypeReexport.res:48:5-24   UseOriginal.originalType.directlyUsed is a record label never used to read a value
TypeReexport.res:70:5-21   OnlyReexportedDead.originalType.usedField is a record label never used to read a value
TypeReexport.res:91:23     VariantUseReexported.originalType.A is a variant case which is never constructed
TypeReexport.res:109:23    VariantUseOriginal.originalType.A is a variant case which is never constructed
TypeReexportCrossFileA.res:5:3-19  originalRecord.usedField is a record label never used to read a value
TypeReexportCrossFileA.res:0:1     TypeReexportCrossFileA is a dead module as all its items are dead.

Every one of those labels is read, through the re-exporting type. The fixture states the expected behaviour in its own comments:

// Expected: originalType.usedField = LIVE (propagated)
//           originalType.unusedField = DEAD
//   warnings: only originalType.unusedField should be reported

Reactive says the opposite, and reports a whole module dead. This is what the editor shows.

Cause, confirmed by A/B

Disable only the manifest/re-export block at the end of Dead_type.process_type_label_dependencies and re-run the batch pass:

batch without manifest linking   412 issues
does batch-without-manifest == reactive ?  True
  reactive-only vs no-manifest: []
  no-manifest-only vs reactive: []

Set-identical in both directions. The reactive pipeline is missing precisely that linking and nothing else.

How it got there

date
2025-12-28 #8092 reactive pipeline lands, incl. ReactiveTypeDeps.ml
2026-01-09 #8127 reanalyze-server lands; the editor starts using reactive
2026-01-23 #8217 re-export linking added to DeadType.ml only

ReactiveTypeDeps.ml already existed when #8217 was written — it is in the tree at that commit, with twelve sibling Reactive* files. #8217 touched DeadType.ml, DeadValue.ml, DeadCommon.ml, Decl.ml, three new fixtures and expected/deadcode.txt, and no reactive file. The feature was one-sided from birth and has been for ~7 months.

Nothing caught it because the same commit added the fixture and regenerated expected/deadcode.txt in batch mode. The fixture that exists to pin this behaviour passes, in the mode nobody ships.

It just happened again

#8643 (record labels read through a coercion reported dead) has the same shape: a label reached through an aliasing relationship is not credited to the original declaration. The fix in #8645 was written against the batch pass first and would have shipped batch-only — the reactive half was added only because a reviewer asked whether the editor path was covered. Without that question this issue would have had a second entry.

Smaller things noticed nearby

  • Reactive_type_deps.all_type_refs is only same_path_refs, so add_to_refs_builder would drop the impl↔intf refs that all_type_refs_from includes. Harmless today: both add_to_refs_builder helpers are dead code, never called.

What would actually prevent the next one

  1. CI checks the two agree. Run the corpus in both modes, diff the finding sets. Cheap, and it would have failed the day #8217 landed. It fails today for the 7 above, so it lands with the re-export fix.
  2. Delete the duplication. The genuinely mode-specific part is narrow — how declarations are indexed and how an edge is recorded. The rules themselves (which declarations participate, how they pair, which direction the edge runs) can be shared; #8645 does this for coercions with Dead_type.pair_coercion_labels / record_label_of_decl. Applied to re-export linking too, adding a rule to one pass and not the other stops being possible.
  3. Or keep one path. If reactive is what ships, batch is a reference implementation that no user runs and that CI mistakes for the product. Retiring it removes the failure mode outright — but it wants (1) first, so the two are known to agree before one is deleted.
主要语言
OCaml
星标
7.5k
派生
485
平均合并
1 天 2 小时
30 天内合并 PR
55

贡献指南

打开贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

rescript-lang/rescript 的其他 Issue

查看 rescript-lang/rescript 的全部 Issue

相似的 Issue

更多 Compilers Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。