はじめに

自作のC# Roslynアナライザー「CSharp_IngeniousAnalyzer」をNuGetで公開しています。
LINQ001、COLL001など、コードの安全性やパフォーマンスを機械的にチェックしてくれるルールをいくつか実装してきました。

今回はその中の1つ、LINQ002（不要な.ToList()/.ToArray()を削除するルール）が、自分自身の実プロジェクトで牙を剥いてきた話です。
Fixを適用したら、ランタイムでInvalidOperationExceptionが発生しました。

前提条件

LINQ002は、こういうコードを検知するルールです。

// Before
var chaosDel2 = srcList.Where(n => 0 < n).ToList();
foreach (var item in chaosDel2) { }

.ToList()で確定させた結果を、その後1回もリスト操作(インデクサアクセスや.Countなど)に使わず、ただforeachで舐めているだけなら、.ToList()は無駄なメモリ確保です。
Fixを当てるとこうなります。

// After
var chaosDel2 = srcList.Where(n => 0 < n);
foreach (var item in chaosDel2) { }

判定ロジックはシンプルで、最終的にはこの1行に集約されていました。

return nonAssignmentRefCount == 1 && foundForEach != null;

「変数が再代入されず、foreachで1回だけ参照されているか」。
これだけです。

やってみたこと

実際に自分のプロジェクトで踏んだコードは、こういう形でした。

var filtered = srcList.Where(n => 0 < n).ToList();
foreach (var item in filtered)
{
    srcList.Remove(item);
}

filteredは確かに再代入もされていないし、foreachで1回しか参照されていません。
条件を満たしているので、LINQ002は「不要な確定です」と警告し、Fixで.ToList()を消しました。

var filtered = srcList.Where(n => 0 < n);
foreach (var item in filtered)
{
    srcList.Remove(item); // ここで InvalidOperationException
}

.ToList()を消した瞬間、filteredは遅延評価のクエリに戻ります。
foreachのたびにsrcListを舐め直すので、ループ中にsrcList.Remove()で元のコレクションをいじると「コレクションが変更されました」で例外です。

列挙中に要素を削除するなら、本来は逆順ループで回すのがセオリーだと思う

for (int i = srcList.Count - 1; i >= 0; i--) { }

とはいえToList()によるスナップショットも実務では普通に使われる有効な対処法で、この記事で問題にしたいのは「どちらの書き方が優れているか」ではありません。
書き方の優劣に関わらず、アナライザーが書かれた意図を判定せずに機械的に壊してしまった、という点です。

原因：「使われ方」しか見ていなかった

判定ロジックが見ていたのは「確定した変数がどう使われるか」だけでした。
「foreachの中で列挙元そのものをどう変更しているか」は一切見ていなかったんです。

対策：LINQチェーンの起点まで遡る

LINQチェーンの起点(この例ではsrcList)まで辿るヘルパーを追加しました。
foreachループ本体の中でそのシンボルに対するメソッド呼び出しが1つでもあれば、安全性を証明できないとみなして警告を止めるようにしました。

var rootSymbol = GetChainRootSymbol(invocation, model, cancellationToken);
return rootSymbol == null || !ForEachBodyMutatesSymbol(foundForEach, rootSymbol, model, cancellationToken);

このとき、次の2点を意識しています。

RemoveやAddのようなメソッド名では判定しない。同じシンボルへの呼び出しなら、読み取り専用の可能性があっても保守的に「疑わしい」として扱う
静的解析だけで「このメソッドは本当に列挙を壊さないか」を型情報だけから断定するのは難しい。誤ってFixを提供してしまうリスクの方が、拾いすぎて警告を出さないリスクより明らかに高くつく

検証：回帰なし、カバレッジも実測

既存のLINQ002のテスト：19件 → 回帰なし
バグを再現するテストを追加：23件
coverletで分岐カバレッジを実測し、新しく追加したコードの分岐に見落としがないかも確認(この辺りの手法は前回の記事と同じ)
プロジェクト全体：234件 のテストが通ることを確認してからリリース

わかったこと

「変数が1回しか参照されていない」は、そのコードが安全に削除できることの証明にはならない

というのが今回の一番の学びです。

.ToList()や.ToArray()のような確定処理は、パフォーマンスのために書かれているとは限りません。

「これから列挙元を変更するから、その前にスナップショットを取っておく」という、防御的なコピーとして書かれていることがあります。

副作用(列挙元への破壊的な操作)まで含めて検証しない限り、「使われていないように見えるコード」を消す自動化は原理的に危険を伴う。

今回はそのことを身をもって学びました。

おわりに

自分自身の実プロジェクトで自作ツールにバグを踏まされる、というなかなか皮肉な体験でした。
しかしそのおかげで良い改善ができたので、結果的には良かったと思っています。

ご意見・アイデアがありましたら、ぜひ本記事のコメント欄や、GitHubのIssues / Discussions、NuGetページから気軽にお知らせください！

NuGet: CSharp_IngeniousAnalyzer
GitHub: IngeniousDesign/CSharp_IngeniousAnalyzer

最後までお読みいただき、ありがとうございました！