Conversation
In package.yaml the executable's flags were written as
- -with-rtsopts=-N
-Wall
-fwarn-incomplete-patterns
-fwarn-incomplete-uni-patterns
which is one YAML scalar continued over four lines, not four list items.
It happens to work — cabal splits the value on whitespace, and the
generated onix.cabal shows all four flags — but only by accident, and the
next edit to that block can silently drop the warning flags. Write them
as separate entries.
The test suite had no -Wall at all, so nothing under test/ was checked
for incomplete patterns or unused bindings. Add it. This is worth doing
alongside the GHC 9.8 move in the base branch: warnings are how a jump of
that size tells you what it changed, and the tests were the part not
saying anything.
Raised in review of #60.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
レビュー差分そのものは正しく、PR 本文の主張も裏が取れました。変更前の 必須なし。 推奨1.
|
| フラグ | 件数 |
|---|---|
-Wunused-imports |
19 |
-Wmissing-signatures |
8 |
-Wunused-matches |
2 |
-Wunused-local-binds |
1 |
| 合計 | 30 |
-Wunused-imports19 件:test/*.hs5 ファイルすべてが同じ import ブロックのコピペを持っていて、その大半が使われていません。例:test/TestCode.hs:6import qualified Data.Map as M（M.の使用ゼロ）test/TestCode.hs:7import Data.Text (Text, pack, unpack)test/TestCode.hs:11import Text.XML (def, parseText, readFile)— これはPrelude.readFileを隠しかねない import なので、消えると素直に嬉しいtest/TestCode.hs:14import qualified Xsd as X（X.の使用ゼロ）test/TestCode.hs:10/TestMixed.hs:9/TestModel.hs:9/TestParser.hs:8/TestUtils.hs:7はTest.HUnitのTestListが未使用- 他に
TestMixed.hs:6,8,10、TestModel.hs:6,11、TestParser.hs:6,7、TestUtils.hs:5,6、Spec.hs:6
-Wmissing-signatures8 件:TestCode.hs:16 tests、TestModel.hs:15,25,32,56 (expected1..4)と92 tests、TestParser.hs:12 expectedと91 tests。TestMixed.hsだけはtests :: [Test]が付いています。全部[Test]か具体型を書くだけです。- なぜ重要か: 19 件は削除するだけ、8 件は 1 行足すだけで、判断を要するものが 1 つもありません。ここを放置すると
make testの出力が毎回 30 行の既読スルー領域になり、後から入る「本当に見るべき警告」が埋まります。-Wallを足した目的（5 世代ジャンプで何が変わったかを警告に語らせる）が、掃除しないと成立しません。逆に言うと掃除コストが極小なので、この PR で一緒にやってしまうのが一番安いです。
3. -Wunused-matches / -Wunused-local-binds の 3 件は「本物の指摘」なので個別に見てほしい
-Wall を足した価値が一番出ているのがここです。
test/TestModel.hs:123scm <- getSchema "./fixtures/test_model_atomic.xsd"→ その直後のactual = typeToText expected1はscmを使っていません。このテストは fixture を読んでいるだけで、読んだ結果を一切検証していません。test/TestModel.hs:147も同様（test_model_atom_ref_bylist.xsdを読んでtypeToText expected4を検証）。test/TestModel.hs:203key = makeTargetQName "Annotation"が未使用。このケースはactual = collectElements scmが[]であることだけを assert しており、keyは書きかけの痕跡に見えます。- 修正案: 未使用の
scm <-を機械的に消すとテストの意味が変わるので、ここだけは「本当に fixture 越しに検証したかったのか、それとも純粋なtypeToTextの単体テストだったのか」を判断してから直してください。単体テストでよいならgetSchemaの行を消す、fixture を検証したかったならtypeToTextの引数をscmから取り出した型に差し替える、のどちらかです。
任意
4. -Wno-x-partial を入れるなら tests スタンザだけ。全体には反対
head/tailの警告（[GHC-63394] [-Wx-partial]）は-Wallとは無関係で、GHC 9.8.1 から default on です（-Wx-⟨category⟩::since: 9.8.1,:default: on）。- 実際、この PR より前の Move to lts-23.25 (GHC 9.8.4) and stop pinning dependency versions #65 の CI ログ（job 103694322422、tests にまだ
-Wallが無い状態）でもtest/に-Wx-partialが 16 件出ています。この PR が増やしたものではありません。 - 内訳:
src/5 箇所（src/Code.hs:108,129,178,200とsrc/Model.hs:150）、test/16 箇所。 - 判断:
src/の 5 箇所は本番コードの部分関数で、9.8 が正しく指摘している実バグ予備軍です。ここを黙らせるべきではありません。一方test/のheadはすべて「このフィクスチャには必ず 1 件以上ある」前提の取り出しで、失敗すればテストが落ちて意味が分かるので、警告の価値は低い。抑制するならtestsスタンザにだけ-Wno-x-partialを足す形にしてください。package.yaml全体やライブラリに付けるのは反対です。 - ただしこれは Move to lts-23.25 (GHC 9.8.4) and stop pinning dependency versions #65 から続く既存事象なので、この PR のスコープ外として後続に回しても構いません。
5. -Wcompat を library / executable に足す
- 5 世代（8.8 → 9.8）飛んだ直後なので、次の飛躍を小さくするのに一番効くのが
-Wcompatです。-Wsemigroup/-Wnoncanonical-monoid-instances/-Wnoncanonical-monad-instances/-Wcompat-unqualified-imports/-Wtype-equality-out-of-scopeが入ります。 - 今の
src/は-Wall下で-Wx-partial以外の警告がゼロ（CI ログで確認）なので、追加コストはほぼ増分ゼロで測れます。 - ただし警告が増える可能性はあるので、指摘 2 の掃除と同時にやらないほうが差分が読みやすいです。後続 PR 推奨。
6. -Werror は package.yaml に入れず、CI 側だけにする
package.yamlに-Werrorを書くとローカル開発と将来の GHC アップグレードが両方壊れます。入れるなら.github/workflows/test.ymlのmake testをstack test --pedantic（=-Wall -Werror）相当にするか、--ghc-options=-Werrorを CI からだけ渡す形。- 前提条件: 現状ユニークで 51 件の警告が出ているので、指摘 2・4 を片付けるまでは入れられません。掃除が終わってからの後続タスクとして。
7. -Wall は hpack のトップレベル ghc-options: に寄せてもよい
- 今回の事故の本質は「3 スタンザに同じ警告フラグをコピペしていて、tests だけ付け忘れていた」ことです。hpack はトップレベルの
ghc-options:を全セクションに継承させられるので、-Wall（と入れるなら-Wcompat）をトップレベルに置き、-threaded -rtsopts -with-rtsopts=-Nだけをスタンザ側に残すと、同じ付け忘れが構造的に起きなくなります。 - 生成される
onix.cabalのフラグの並び順は変わるので、ハッシュ再計算が必要です。挙動は変わらないため完全に任意。
検証したこと / できなかったこと
検証したこと
(1) YAML の主張（PyYAML でパース）
$ python3 -c "import yaml,json; ..." # before = origin/claude/haskell-resolver-lts23, after = origin/claude/fix-ghc-options
=== before.yaml ===
executable onix-exe type= list len= 3
["-threaded", "-rtsopts", "-with-rtsopts=-N -Wall -fwarn-incomplete-patterns -fwarn-incomplete-uni-patterns"]
joined: -threaded -rtsopts -with-rtsopts=-N -Wall -fwarn-incomplete-patterns -fwarn-incomplete-uni-patterns
test onix-test type= list len= 3
["-threaded", "-rtsopts", "-with-rtsopts=-N"]
=== after.yaml ===
executable onix-exe type= list len= 6
["-threaded", "-rtsopts", "-with-rtsopts=-N", "-Wall", "-fwarn-incomplete-patterns", "-fwarn-incomplete-uni-patterns"]
joined: -threaded -rtsopts -with-rtsopts=-N -Wall -fwarn-incomplete-patterns -fwarn-incomplete-uni-patterns
test onix-test type= list len= 4
["-threaded", "-rtsopts", "-with-rtsopts=-N", "-Wall"]
→ before は 3 要素で 3 番目が折り畳みスカラ、after は 6 要素。join 結果が同一なので生成 cabal 行は before でも正しかった＝PR 本文の「偶然」という説明どおりです。
(2) hpack ハッシュの再計算
アルゴリズムは hpack 本体から起こしました（src/Hpack.hs: calculateHash (CabalFile cabalVersion _ _ body _) = sha256 (unlines $ cabalVersion ++ body)、src/Hpack/CabalFile.hs: span isComment <$> span (not . isComment) → 先頭の非コメント行群 + コメントヘッダ + dropWhile null した本体）。つまり L1-2 と L9-末尾を連結して sha256。
main.cabal (hpack が実際に生成した既知のファイル。アルゴリズム検証用)
stated d894bbda696a3e4398796a16a928bd9923bbeaae2b4a61ad371f193c8322fa0d
computed d894bbda696a3e4398796a16a928bd9923bbeaae2b4a61ad371f193c8322fa0d MATCH
base.cabal (#65)
stated 07b4003dd4819b94016dc0a6629b5b634c96677c275eb19e5e97c2a6ac30ffd5
computed 07b4003dd4819b94016dc0a6629b5b634c96677c275eb19e5e97c2a6ac30ffd5 MATCH
after.cabal (本 PR)
stated 3926f8a5f904cf97dc479ae5599b082559e417ae2568fb51d2a7fd1931177343
computed 3926f8a5f904cf97dc479ae5599b082559e417ae2568fb51d2a7fd1931177343 MATCH
(3) 3 スタンザのレンダリング一致
onix.cabal の差分はハッシュ行と L85 の 1 行のみ。L42 / L66 / L85 は package.yaml のリストを順序どおり空白 join したものと完全一致しています。
42: ghc-options: -Wall -fwarn-incomplete-patterns -fwarn-incomplete-uni-patterns
66: ghc-options: -threaded -rtsopts -with-rtsopts=-N -Wall -fwarn-incomplete-patterns -fwarn-incomplete-uni-patterns
85: ghc-options: -threaded -rtsopts -with-rtsopts=-N -Wall
(4) -Wall が tests に実際に効いているか（CI ログの実測）
本 PR の run（job 103694787643, ghc-9.8.4）と base #65 の run（job 103694322422）の両方をパースして比較しました。
| base #65 | 本 PR #66 | |
|---|---|---|
src/ -Wx-partial |
5 | 5 |
test/ -Wx-partial |
16 | 16 |
test/ -Wunused-imports |
0 | 19 |
test/ -Wmissing-signatures |
0 | 8 |
test/ -Wunused-matches |
0 | 2 |
test/ -Wunused-local-binds |
0 | 1 |
app/ |
0 | 0 |
→ -Wall が tests に確実に届いていること、-Wx-partial はこの PR とは無関係に default on であること、executable 側は（折り畳み時代も含め）そもそも警告ゼロだったことが確認できました。
(5) -Wall のフラグ集合（GHC 9.8 ユーザーズガイド原典）
docs/users_guide/using-warnings.rst（ghc-9.8 ブランチ）より:
-Wに-Wincomplete-patternsが含まれる-Wallは「-Wextra（=-W）の全部 + …」で、その+側に-Wincomplete-uni-patternsと-Wmissing-signaturesと-Wname-shadowingが明記されている-Wx-⟨category⟩は:since: 9.8.1/:default: on/:reverse: -Wno-x-⟨category⟩-fwarn-⟨wflag⟩について「This spelling is deprecated, but still accepted for backwards compatibility.」
検証できなかったこと
hpackを実際に走らせてonix.cabalがバイト単位で一致するかは未検証です。この環境に GHC / stack / cabal / hpack がなく、haskell.org は egress ポリシーでブロックされているため入れられませんでした。代替として (a) hpack ソースから起こしたハッシュアルゴリズムを既知の hpack 生成物で検証したうえでの自己整合性確認、(b) CI ログから読み取った実効フラグの確認、の 2 つで担保しています。特に (b) は「生成 cabal がどうであれ、実際にコンパイラに渡ったフラグ」を見ているので、この PR の目的に対しては十分な検証だと考えます。-Wcompat/-Wno-x-partialを足した場合に増減する警告の実数は未測定です（ビルドできないため）。指摘 5・6 を後続 PR に回すことを勧めているのはこのためです。-Wname-shadowing/-Wincomplete-patternsがtest/で何も出していないのは CI ログ上の事実であって、ソース読解による推測ではありません（ログに 1 件も現れませんでした）。
Generated by Claude Code
Review checked this against GHC 9.8: -Wincomplete-patterns comes in via -W, and -Wincomplete-uni-patterns moved into -Wall in 9.2, so both spellings were redundant everywhere they appeared. The deprecated -fwarn-* form is still accepted silently — the CI log carries no deprecation output — so this costs nothing per build; it was just six lines implying the project asks for something -Wall does not give. ghc-options now reads as what it is: library -Wall executable -threaded -rtsopts -with-rtsopts=-N -Wall test -threaded -rtsopts -with-rtsopts=-N -Wall Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
|
レビューありがとうございます。推奨 1 を反映しました (4fd2d64)。残りは方針をお伝えします。 推奨 1:
|
CI caught it immediately:
Error: [S-305] Failed to generate a Cabal file using the Hpack library
YAML parse exception at line 52, column 20:
mapping values are not allowed in this context
My own doing. I removed the -fwarn-* lines with a list of replacement
strings that had the four-space forms before the six-space ones, and the
four-space form is a substring of the six-space form. So on the
executable's lines it matched two characters in and deleted from there,
leaving a stray " " on each line that then ran into the following
`dependencies:` and indented it by eight spaces.
Restore the correct indentation. package.yaml now parses, and the four
stanzas read as intended:
library ['-Wall']
executable ['-threaded','-rtsopts','-with-rtsopts=-N','-Wall']
test ['-threaded','-rtsopts','-with-rtsopts=-N','-Wall']
with both dependency lists intact. Checked with a YAML parser this time,
which is what I should have done before pushing the previous commit —
onix.cabal was unaffected and its hash is still self-consistent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
#71 inherited the broken YAML through its base. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
|
前のコミットで 原因
- -Wall
dependencies: ← 4 スペースであるべき
- onix修正と検証インデントを戻し、今度は YAML パーサで検証しました。 両方の 前のコミットを push する前にこの検証をすべきでした。同じ変更でワークフローの YAML は検証していたのに、 #71 は base 経由でこの破損を引き継いでいたので、そちらにもマージ済みです。 Generated by Claude Code |
1 participant
#60 のレビューで指摘された、
package.yamlの警告フラグの事故を直します。base は #65（resolver 更新）。何が起きていたか
executablesのghc-optionsはこう書かれていました。これは 4 つのリスト要素ではなく、4 行に折り畳まれた 1 つのスカラ値です。結果的には動いています（cabal が空白で分割するため、生成された
onix.cabalには 4 つのフラグが並びます）が、成立しているのは偶然です。このブロックを次に編集した人が、気づかないまま警告フラグを落とす形になります。テストスイートには
-Wallが無かったtestsスタンザにはそもそも警告フラグがなく、test/配下は不完全パターンも未使用束縛も一切チェックされていませんでした。追加します。これを #65（GHC 8.8.3 → 9.8.4）と一緒に入れるのには理由があります。5 世代分の跳躍で何が変わったかを教えてくれるのは警告であり、テストコードだけが黙っている状態は避けたいためです。
確認
onix.cabalの 3 スタンザすべてでghc-optionsが期待どおりになることを確認-Wallを足したことで新しい警告が出る可能性はありますが、警告はエラーではないので CI は緑のままのはずです🤖 Generated with Claude Code
https://claude.ai/code/session_01P8rZakwAiz1jpViUX34x1Z
Generated by Claude Code