はじめに
自作の C# アナライザー 『CSharp_IngeniousAnalyzer』 について、ユーザーの方からメールでバグ報告をいただきました。
ToList() の削除を提案するルール LINQ002 の誤検知についての報告でした。
最初は「判定を少し直せば済む」と考えていました。
ところが、直しても直しても新しい抜け道が見つかり、最終的にはルールそのものを廃止する判断をしました。
廃止したバージョン 3.4.2 は、すでに NuGet で公開しています。
この記事では、そこに至るまでの経緯と、その過程で分かったことをまとめます。
前回の記事：
LINQ002 とは
🔹 LINQ002: 不要なコレクションの実体化の削除
- タイトル: 不要なコレクション確定の削除
-
メッセージ:
'{0}' は列挙後に再利用されていません。不要なコレクションの実体化（メモリ確保）を回避するため、呼び出しを削除してください。 -
解説:
foreachで1回回すだけなのに.ToList()や.ToArray()で一時的なコレクションを作っているコードを検知します。
Fix を適用すると呼び出しが削除され、遅延評価のまま列挙する形になります。
// Before
var filtered = srcList.Where(n => 0 < n).ToList();
foreach (var item in filtered) { }
// After (Fix適用後)
var filtered = srcList.Where(n => 0 < n);
foreach (var item in filtered) { }
いただいたバグ報告
報告の内容は、次のとおりでした。
LINQ002 は、foreach の前に列挙元のコレクションがクリアされていても ToList() の削除を提案する。
Fix を適用するとスナップショットが失われ、ループは取り込んだはずの要素を黙ってすべて読み飛ばしてしまう。
var snapshot = srcList.Where(n => 0 < n).ToList();
srcList.Clear();
foreach (var item in snapshot) { }
この ToList() は無駄ではありません。
Clear() する前の内容を残しておくための、意図的なスナップショットとして使われています。
Fix で ToList() を消すと、Where() は遅延評価なので、要素を読みにいくのは foreach の時点になります。
そのころには srcList が空なので、ループは1回も回りません。
例外も出ないため、黙って動作が変わるタイプの不具合でした。
1回目の修正：列挙元に触れていたら警告しない
もともと LINQ002 は、ループ本体で列挙元を Remove() している場合だけを除外していました。
宣言から foreach までの間は、チェックの対象外だったわけです。
そこで「ToList() の後で列挙元が1回でも参照されていたら警告しない」と修正し、3.4.1 として公開しました。
ところが、修正を見直すうちに、これでは足りないことに気づきます。
2回目の修正：別メソッド経由と、ラムダが捕まえた変数
別メソッドの中で変更される
var snapshot = _items.Where(n => 0 < n).ToList();
ResetItems(); // メソッドの中で _items.Clear() している
foreach (var item in snapshot) { }
呼び出し先で何をしているかは、アナライザーからは見えません。
ラムダが捕まえた変数が変更される
列挙元に一切触れていなくても、結果が変わるケースがありました。
var src = new List<int> { 1, 5, 10, 50, 100 };
var limit = 3;
var xs = src.Where(n => n > limit).ToList();
limit = 30;
// foreach (var x in xs) ...
実際に動かした結果を示します。
ToListあり: 5, 10, 50, 100
ToListなし: 50, 100 ← Fix適用後
ラムダ n => n > limit は、limit の値ではなく変数そのものを覚えています。
ToList() を消すとラムダの実行が foreach の時点まで後ろにずれ、変更後の limit でフィルターされてしまいます。
そこで、「ToList() からループの終わりまでに、メソッド呼び出しや、チェーンが使う変数への書き込みがあれば警告しない」という判定に作り直しました。
別の視点からのレビューで、穴が次々と見つかる
判定が正しいかを確かめるため、私の設計を前提にしないレビューを行いました。
「アナライザーで解析 → Fix を適用 → 修正前後のプログラムを実際に動かして出力を比べる」という仕組みを作り、怪しい書き方を片っ端から試す方法です。
結果、Fix を適用すると出力が変わるケースが 8種類 見つかりました。
一部を挙げます。
- 先に作った遅延クエリ
var big = src.Where(x => x > limit);を列挙元にして、ループ本体でlimitを書き換える -
for (var q = src.Where(...).ToList(); ...; ...)のように for の初期化子で宣言する（周回ごとに再評価される） -
strs.ToList<object>()を消すと、ループ変数の型がobjectからstringに変わり、==の意味が参照比較から値比較に変わる -
Enumerable.ToList(src)の形で書くと、Fix 後にコンパイルエラーになる
すべて直しましたが、ここで方針を考え直しました。
3回目：「100% 不要と言えるもの」だけを警告する
ここまでは「危ないものを除外する（ブラックリスト）」方式でした。
これでは、想定していない書き方がすり抜けてしまいます。
そこで、「安全だと証明できる書き方だけを認める（ホワイトリスト）」方式に作り直しました。
-
var v = ….ToList();の直後の文でforeachしている（間に文を挟まない） - 列挙元は配列か、メソッド内で
new List<T>()したローカル変数 - チェーンは
Where／Select／Skip／Takeなどの決まったメソッドだけ - ラムダとループ本体は、リテラル・変数・フィールド・自動実装プロパティ・組み込みの演算子など、認めた要素だけ
- チェーンが例外を投げうる場合は、ループ本体が例外を投げず、途中で抜けず、try の中にない
かなり厳しい条件になりました。
ループ本体でメソッドを1つでも呼んでいれば、もう警告は出ません。
それでも抜け道は見つかった
この方式に対しても、レビューを2回行いました。
結果は、9種類、続いて 5種類 の抜け道でした。
List<T> の派生クラス
void M(List<int> items) // 実体が List<int> の派生クラスかもしれない
{
foreach (var n in items.Skip(1).ToList()) { }
}
派生クラスがインターフェイスを再実装していると、Skip がその処理を呼びます。
ToList の有無で、その呼び出し回数が変わってしまいます。
引数の List<T> は、実体が何か解析では分かりません。
本体のないプロパティが、本当に「何もしない」とは限らない
int X { get; } と書かれていても、virtual や abstract なら派生クラスの getter が動きます。
構造体の静的コンストラクター
明示的な静的コンストラクターを持つ構造体は、自動実装プロパティの getter を最初に呼んだ時点で、静的コンストラクターが実行されます。
default で作った値なら、それまで一度も実行されていません。
ToList を消すと、この「最初の呼び出し」が起きるかどうか自体が変わります。
現在のカルチャー
"v" + 1.5 のような数値の文字列化は、現在のカルチャーを使います。
カルチャーを自作のクラスに差し替えられると、文字列化のたびにその処理が動きます。
回を追うごとに抜け道は特殊になっていきました。
それでも、「もう抜け道はない」とは最後まで証明できませんでした。
その一方で、警告を出せる範囲はほとんど残っていませんでした。
廃止という判断
ここで、ようやく問題の本質に気づきました。
その ToList() が必要か不要かは、書いた人の意図と、実行時の振る舞いで決まります。
コードの字面だけで、完璧に見分けることはできません。
判定を厳しくするほど安全にはなります。
しかしそれは、警告を出せる範囲が狭くなることと引き換えでした。
そこまでしても、黙ってコードを壊しうる Fix が残る可能性は消せません。
本アナライザーには、「動作が変わる可能性が少しでもある書き換えは提供しない」という方針があります。
たとえば ASYNC001 では .ConfigureAwait(false) を自動で追記せず、UNI001 では見えない文字を自動で書き換えていません。
LINQ002 も、この方針に照らせば残しておけないルールでした。
そこで、3.4.2 で LINQ002 を廃止しました。
これまで LINQ002 が出ていたコードでは、警告が出なくなります。
.editorconfig に dotnet_diagnostic.LINQ002.severity を書いていても、エラーにはならず無視されるだけです。
この経験から得たこと
「直せる」と「直してよい」は違う
アナライザーの Fix は、ボタン1つで大量のコードを書き換えられます。
便利な反面、間違っていても誰にも気づかれずに広がる危うさがあります。
「たいていのケースで正しい」では、自動で書き換える理由になりません。
検証の仕組みに助けられた
今回は、次の3つの検証を組み合わせました。
- テスト：最終的に、LINQ002 だけで260件まで増えました
- ミューテーションテスト：判定を1つずつ無効にして、テストが失敗するかを確かめました（152パターン中150パターンで失敗を確認）
- 実行比較：修正前後のプログラムを実際に動かし、出力を比べました（84本）
テストが通っていても、テストの書き方が甘くて別の判定で弾かれているだけ、ということが何度もありました。
ミューテーションテストがなければ、気づけなかったと思います。
やめる判断も、改善のひとつ
機能を削るのは、少し寂しい判断でした。
それでも、黙って動作を変えてしまうかもしれない機能を残すより、ずっと誠実だと考えました。
おわりに
今回の件は、ユーザーの方からの報告がなければ、ずっと気づかないままでした。
報告をくださった方に、この場を借りてお礼を申し上げます。
ありがとうございました！
もし
- 「Fix を適用したら動作が変わった」
- 「このコードで警告が出るのはおかしい」
- 「こういう書き方も検知してほしい」
といったことがありましたら、ぜひ本記事のコメント欄や、GitHub の Issues、NuGet ページから気軽にお知らせください！
最後までお読みいただき、ありがとうございました！