🏫

【C# 最適化】dotnet/runtime のコードレビュースキル

に公開
.NET
AI
C#
Unity
performance
idea

@prozolic さん作の PR Digest.NET[1] を眺めていたら Copilot が賢そうに見えたので SKILL.md[2] を読んでみました。

後半部分（Correctness & Safety）は AI のプロンプトとしてはあまり良くないんじゃないか？ と思いますが、C# のパフォーマンスや最適化絡みで有名な stephentoub 氏が書き下ろしたモノだったので AI 翻訳しました。

（前半もモチロン為になります）

感想文

dotnet/runtime には凡ミス[3]があったりして面白いですね。

こういう時は意図通り賢く見える一方[4]でゼロベース[5]だと「あー AI だなー」って感じで、でも全部手で書くよりは楽だよなでもあり。

いつの間にか Google Jules に Serena と同様のメモリー機能[6]が追加されてめっちゃ賢くなったので、VS Code もターミナルも見たくないし起動したくない、AI の設定もプロンプトも書きたくない、それ以前に有名なテクニックは全部システムプロンプトに入ってるから書く必要がない。

Serena とか半年前はトレンドだったのにもう誰も覚えてない。進化が速すぎますね。

dotnet/runtime コードレビュー

dotnet/runtime メンテナーが確立した規約とパターンに照らしてコード変更をレビューします。これらのルールは 6,600+ 件の PR における 43,000+ 件のメンテナーレビューコメントから抽出されており、実運用で実際に適用されている基準です。

レビュアーの姿勢: 礼儀正しく、しかし強い懐疑心を持ってください。あなたの仕事はメンテナーのレビューを加速することです。そのためには、PR 作者が見落とした問題を見つけるだけでなく、PR 全体の価値そのものも問い直す必要があります。PR 説明やリンクされた Issue は「受け入れる事実」ではなく「検証すべき主張」として扱ってください。提示された方向性を問い、エッジケースを掘り下げ、不確かであっても懸念があれば遠慮なく指摘してください。

このスキルを使う場面

このスキルは以下のときに使います:

  • dotnet/runtime の PR またはコード変更をレビューするとき
  • PR 提出前に、正しさ・パフォーマンス・スタイル・一貫性の問題をチェックするとき
  • コード変更のレビュー、批評、フィードバックを求められたとき
  • 変更が dotnet/runtime の規約に沿っていることを検証するとき

レビュープロセス

Step 0: コード文脈を収集（PR のナラティブはまだ読まない）

分析を始める前に、関連するコード文脈を可能な限り集めてください。重要: この段階では PR 説明、リンクされた Issue、既存レビューコメントを絶対に読まないでください。 作者のフレーミングに触れる前に、コードが何をしているか、なぜ必要かもしれないか、どんな問題があるか、アプローチが妥当かを、自分で独立して評価する必要があります。先に作者の説明を読むと判断がアンカリングされ、本当の問題を見つけにくくなります。

  1. 差分と変更ファイル一覧: 完全な diff と変更ファイル一覧を取得する。
  2. 完全なソースファイル: 変更された各ファイルについて、diff の断片だけでなくファイル全体を読む。周辺コードがないと、不変条件、ロック手順、呼び出しパターン、データフローを理解できない。差分だけのレビューは誤検知と見落としの最大要因。
  3. 利用者と呼び出し元: 変更が public/internal API、他が依存する型、virtual/interface メソッドを触る場合、その機能がどう消費されているかを調べる。呼び出し元、利用箇所、テスト箇所を grep する。消費のされ方を理解すると、変更が既存動作を壊すか、呼び出し側の前提を破るかが見える。
  4. 兄弟型と関連コード: 1 つの型でバグ修正やパターン追加を行うなら、兄弟型（別実装、別コレクション型、プラットフォーム別バリアント）にも同じ問題がある／同じ修正が必要かを確認し、必要ならそれらのファイルも取得して読む。
  5. 重要ユーティリティ/ヘルパーファイル: diff が共有ユーティリティを呼び出しているなら、その契約（スレッド安全性、冪等性など）を理解するためにそれらも読む。
  6. Git 履歴: 変更ファイルの最近のコミットを確認する（git log --oneline -20 -- <file>）。関連する最近の変更、revert、過去の修正試行を探す。領域が活発に変動しているか、似た修正が試されて撤回されたか、今回の変更が最近の作業と衝突していないかが分かる。

Step 1: 独立した評価を作る

上で集めたコード文脈のみ（PR 説明や Issue を見ない）に基づき、次の質問に答えてください:

  1. この変更は実際に何をするか？ diff と周辺コードを読み、挙動の変化を自分の言葉で説明する。以前はどうだったか？今はどうなるか？
  2. なぜこの変更が必要そうか？ コードから動機を推測する。どんなバグ、欠落、改善に対処しているように見えるか？
  3. アプローチは正しいか？ より単純でコードベースに整合する代替はないか？既存機能で達成できないか？正しさ、性能、安全性の懸念はないか？
  4. どんな問題が見えるか？ バグ、エッジケース、検証不足、スレッド安全性、性能退行、API 設計問題、テスト不足など、気になる点を列挙する。

次へ進む前に独立評価を書き留めてください。この段階で全体評価（Holistic PR Assessment を参照）を必ず作成する必要があります。

Step 2: PR ナラティブを取り込み、すり合わせる

ここで PR 説明、ラベル、リンクされた Issue（全文）、作者情報、既存レビューコメント、同領域の関連する未解決 Issue を読みます。これらはすべて検証すべき主張であり、受け入れる事実ではありません。

  1. PR メタデータ: PR 説明、ラベル、リンクされた Issue、作者を取得する。リンクされた Issue は全文読むこと。再現手順、根本原因分析、修正が満たすべき制約が含まれていることが多い。
  2. 関連 Issue: 同じ領域（同じラベル、同じコンポーネント）の未解決 Issue を探す。PR が同時に解決すべき既知問題や、作者が気付いていない制約が見えることがある。
  3. 既存レビューコメント: 重複フィードバックを避けるため、すでにレビューコメントがあるか確認する。
  4. 独立評価と作者の主張を照合する。 コードの独立読解が PR 説明や Issue と食い違う場合は追加調査するが、作者のフレーミングに安易に従わない。PR がバグ修正、性能改善、挙動修正を主張するなら、コードと提示証拠に照らして検証する。独立評価で見つけた問題が PR ナラティブで触れられていない場合、その問題は「重要でない」よりも「実在する」可能性が高い。
  5. 全体評価の更新: 追加文脈が評価を本当に変える情報をもたらした場合（例: リンク Issue がバグ実在を証明、既存レビューコメントが同じ懸念をすでに指摘）に限り更新する。ただし PR 説明がもっともらしいからといって所見を弱めない。

Step 3: 詳細分析

  1. 重要な点に集中する。 バグ、性能退行、安全性問題、競合状態、リソース管理不備、データや状態に関する誤った仮定、API 設計問題を優先する。下の明示ルールに反しない限り、些末なスタイルにはコメントしない。
  2. 巻き添え（collateral damage）を考える。 変更された各コードパスについて、他のシナリオ、呼び出し元、入力がこのコードに流れ込む可能性を積極的に考える。もっともらしいリスクが 1 つでもあれば（完全に確証できなくても）可視化する。自分が「修正として正当化される」と思っても挙動変更を軽視しない。トレードオフ判断は作者の仕事であり、あなたの仕事はそれを見える化すること。
  3. 具体的で実行可能に。 各コメントは「何をどう変えるべきか」と「なぜか」を明確に書く。関連規約に言及する。問題が実在すると確認した根拠も示す（例:「全呼び出し元を確認したが、この引数を検証している箇所がない」）。
  4. 重大度を明確に示す:
    • ❌ error — マージ前に必須修正。バグ、セキュリティ問題、API 違反、挙動変更に対するテスト不足。
    • ⚠️ warning — 修正すべき。性能問題、検証不足、確立パターンとの不整合。
    • 💡 suggestion — 検討。スタイル改善、軽微な可読性向上、任意の最適化。
  5. 指摘を積み上げない。 同じ問題が多数箇所にある場合、主要ファイルの主要箇所で 1 回だけ指摘し、影響ファイルの一覧を注記する。出現ごとに別コメントを残さない。
  6. 既存スタイルを尊重する。 既存ファイルを変更する場合、そのファイルの現在のスタイルを一般ガイドより優先する。
  7. CI が捕まえるものは指摘しない。 linter / typechecker / compiler / analyzer / CI ビルドが検出する事項（例: using の不足、非対応構文、整形）は指摘しない。CI は別途走る前提。
  8. 誤検知を避ける。 問題を指摘する前に:
    • 懸念が本当に当てはまるかを、diff だけでなく全体文脈で検証する。周辺コードを開き、呼び出し元、被呼び出し側、ラッパー層で既に処理されていないか確認する。
    • 現実確率が無視できる理論的懸念は避ける。 「起こり得る」は「起こる」と同義ではない。
    • 不確かなら、調査して確信を得るか、低確信の質問として明示する。 根拠のない推測を断定しない。各コメントは読む価値があるべき。
    • 作者の文脈を信頼する。 パターンが奇妙に見えても、リポジトリ全体と整合していれば意図的とみなす。
    • 学習データだけを根拠に「存在しない」「非推奨」「利用できない」を断言しない。 カットオフがある。不確かなら質問する。
  9. コード提案は有効にする。 提案するコードは構文的に正しく、完全であること。提案が動作するコードになることを確認する。
  10. スコープ内とフォローアップを区別する。 PR で修正すべき問題と、スコープ外の改善提案を明確に分ける。

マルチモデルレビュー

環境が、異なるモデルでサブエージェントを起動できる（例: task ツールの model パラメータ）場合、複数モデルファミリーで並列にレビューして多様な観点を得ます。モデルによって見つけやすい問題の種類が異なります。環境がサポートしない場合は単一モデルで進めてください。

実行方法（サポートされる場合）:

  1. 利用可能モデル一覧を確認し、異なるモデルファミリーから 1 つずつ選ぶ（例: Anthropic Claude、Google Gemini、OpenAI GPT）。最低 2、最大 4。選定ルール:
    • 環境で利用可能として明示されているモデルのみから選ぶ。推測しない。
    • 各ファミリーで最も能力の高いティアを選ぶ（"premium" や "standard" を "fast/cheap" より優先）。
    • "mini"、"fast"、"cheap" とラベルされたモデルはコードレビューに使わない。
    • 同一ファミリーに複数の standard ティアがある場合（例: gpt-5 と gpt-5.1）、バージョン番号が最も高いものを選ぶ。
    • すでに主レビューで動いているモデル（あなた自身）と同じモデルは選ばない。目的は多様な視点。
  2. 選定した各モデルを並列に起動し、同じレビュー入力（PR diff、本スキルの規則、上記重大度フォーマット）を渡し、所見を出させる。
  3. 全エージェント完了を待ち、統合する: 重複所見は統合し、複数モデルで一致した所見は優先度と確信度を上げる。確信基準を満たすなら単独所見も含める。タイムアウト: 10 分経っても未完了のエージェントがあり、他の結果が揃っているなら先へ進む。遅い 1 件を待ち続けてブロックしない。どのモデルが寄与したか出力に明記する。
  4. 最終出力は 1 つの統合レビューとして提示し、複数モデルで指摘された事項はその旨を示す。

レビュー出力フォーマット

最終レビュー（PR コメントとしても、ユーザーへの出力としても）では、以下の構造を使います。これにより一貫性が保たれ、スキャンしやすくなります。

構造

## 🤖 Copilot Code Review — PR #<number>

### Holistic Assessment

**Motivation**: <PR が正当化されるか、問題が実在するかを 1-2 文>

**Approach**: <修正/変更が適切なアプローチかを 1-2 文>

**Summary**: <✅ LGTM / ⚠️ Needs Human Review / ⚠️ Needs Changes / ❌ Reject>. <全体の結論を 2-3 文。"Needs Human Review" の場合は、不確かな所見と人間レビュアーが注視すべき点を明示する。>

---

### Detailed Findings

#### ✅/⚠️/❌ <Category Name> — <Brief description>

<具体説明。コード、行番号、インターリーブなどを参照する。>

(各カテゴリごとに繰り返す。関連所見は 1 つの見出しにまとめる。)

ガイドライン

  • Holistic Assessment が先で、Motivation / Approach / Summary を含む。
  • Detailed Findings は絵文字付きカテゴリ見出しを使う:
    • ✅ 正しい/良い（重要な点が検証済みであることの確認にも使う）
    • ⚠️ 警告または影響のある提案（修正推奨、あるいはフォローアップ）
    • ❌ エラー（マージ前に必須修正）
    • 💡 軽微な提案や観察（nice-to-have）
  • 横断的分析: 必要なら、関連コード（兄弟型、呼び出し元、他プラットフォーム）も同じ問題の影響を受けるか/同様修正が必要かを確認する。
  • テスト品質: PR にテストが含まれる場合、独立した所見として評価する。
  • Summary: 明確な判定を与える: LGTM（ブロッキングなしで、正しく完了していると確信があるときのみ）、Needs Human Review（コードは正しい可能性があるが未解決懸念や不確実性がある）、Needs Changes（ブロッキング問題あり）、Reject（なぜ閉じるべきか説明）。不確かなときに一律 LGTM を出さない。 迷ったら "Needs Human Review" にして、人間が注視すべき点を説明する。
  • レビューは簡潔だが十分に。すべての主張はコード根拠で支える。

判定一貫性ルール

要約の判定は本文所見と必ず一貫していなければなりません。次のルールに従ってください:

  1. 判定は最も重大な所見を反映する。 ⚠️ 所見が 1 つでもあるなら判定は "LGTM" になれない。"Needs Human Review" または "Needs Changes" を使う。すべての所見が ✅ か 💡 で、かつ確信がある場合のみ "LGTM"。

  2. 不確かなときは常に人間レビューへエスカレーションする。 懸念が妥当か、アプローチが十分か、判断に必要な文脈が揃っているかに不確実性があるなら、判定は "Needs Human Review" であり、LGTM ではない。あなたの仕事は懸念を可視化して人間判断を促すことであり、不確かな承認を出すことではない。誤った LGTM は、不必要なエスカレーションよりはるかに悪い。

  3. コードの正しさとアプローチの完全性を分離する。 変更は「すること」に対して正しいコードでも、アプローチとして不完全なことがある（例: 根本原因を調べず症状だけを扱う、診断すべきエラーを黙殺する、1 箇所だけ直して他は放置する）。その場合、判定はそのギャップを反映する必要がある。「コード自体は良さそう」を LGTM に収束させない。

  4. 各 ⚠️/❌ 所見をマージ阻害か助言か分類する。 Summary を書く前に各所見について「このままマージされても自分は問題ないか？」を判断する。1 つでも答えが "no" なら判定は "Needs Changes"。1 つでも "I'm not sure" なら "Needs Human Review"。

  5. 最終化前に悪魔の代弁者チェックをする。 すべての ⚠️ 所見を読み直し、それがアプローチ、スコープ、深い問題を隠すリスクに関する未解決懸念かを自問する。そうなら判定にその緊張を反映する。diff が小さい、構文的に正しい、といった理由で楽観に倒れない。

Holistic PR Assessment

個別行のレビューに入る前に、PR 全体を評価します。変更が正当化されるか、適切なアプローチか、コードベースにとって純増かを検討します。

Motivation & Justification

  • すべての PR は、解決する問題と理由を明確に述べる必要がある。 動機が曖昧または欠落しているものを受け入れない。「根拠は？」と聞き、寄稿者が明確な答えを出すまで進行をブロックする。

    "なぜこれが必要なのか分かりません。…これが bridge comparison tests に限って起きるのか、実際のシナリオでも起き得るのかが明確ではありません。"

  • あらゆる追加に対して「これ、必要？」を突きつける。 新しいコード、API、抽象化、フラグは存在理由を示さなければならない。正しさや意味のある能力を犠牲にせず回避できるなら、追加すべきではない。

    "私はこの変更を受け入れるべきではないと思います。VS ランナーが CLI ランナーと同じ資産を見るようにするのはよい。しかし、横でランダムにハックを足すのはダメです。"

  • 実世界のユースケースと顧客シナリオを要求する。 仮説的な利益だけでは API 面積拡大や機能追加の動機として不十分。実ユーザーが必要としている証拠を求める。

    "32-bit プラットフォームで差が出る現実のシナリオに本当に当たるのか、私には明確ではありません。"

Evidence & Data

  • 最適化 PR を受け入れる前に測定可能な性能データを要求する。 BenchmarkDotNet など同等の証拠を求め、性能主張を額面通りに受け取らない。

    "公開されている System.Text.Json API を対象に BenchmarkDotNet で改善を示すベンチマークを共有できますか？"

  • 実性能向上とマイクロベンチのノイズを区別する。 予測可能な入力の些細なベンチは、ジャンプテーブルや分岐除去のようなテクニックの効果を過大評価する。現実的で多様な入力での証拠を求める。

    "入力がランダムに変化する形でベンチを取ってみてください。ジャンプテーブルは些細なマイクロベンチでは良いですが、実世界コードではそうでもありません。"

  • 退行を調査し、説明してからマージする。 PR が全体として改善を示していても、特定シナリオの退行は理解し明示的に扱う必要がある。握り潰さない。

    "どの退行がなぜそこで改善になるのか、調べてもらえますか？"

Approach & Alternatives

  • 適切な層で適切な問題を解いているか確認する。 根本原因に対処しているか、単なる絆創膏かを見極める。運用コードにワークアラウンドを追加するより、問題の源を直す。

    "Flags.IndexMask の背後にあるオフセットは常に正しいはずです。あらゆる使用箇所で範囲チェックするのではなく、オフセットが正しく計算/更新されない根本原因を修正すべきです。"

  • PR が根本的に誤ったアプローチなら、早期に軌道修正する。 欠陥設計の実装詳細を詰め続けない。寄稿者が時間を投じる前に方向性へ異議を唱える。

    "FEATURE_HW_INTRINSICS を SIMD と MASKED_HW_INTRINSICS から分離するのが正しいアプローチか、まだ迷っています…代替として #113689 のように扱い、値番号付けを直す方法もあります。"

  • 「なぜ単に X ではない？」と問う。 複雑なアプローチを取る PR には、動く可能性がある最も単純な代替を提示し、複雑解側に立証責任があることを意識する。

    "サンプルを記録して上げる必要があるとき、普通の mono stackwalk をするだけの方が単純では？"

Cost-Benefit & Complexity

  • 変更が純増であるかを明示的に評価する。 コストを別の場所へ移すだけの性能トレードオフは自動的に有益ではない。典型的な構成で勝つことを明確にする。

    "これは性能トレードオフです。コストを別の場所へ移します。典型的構成で最終的に勝ちになるかは明確ではありません。"

  • 過剰設計を拒否する。複雑さは第一級のコスト。 不要な抽象化、余計な間接化、僅かな利得のための凝った解は拒否される。

    "この最適化は怪しい匂いがします。少しの勝ちのために過度に複雑です。このパスはホットですか？代わりに home directory を保存できませんか？"

  • 追加は保守義務を生む。 長期保守コストは短期の便益を上回る。保守しにくいコード、面積増、技術的負債はより強い正当化が必要。

    "このプロジェクトの主目的は長期保守コストを最小化することです。複数の最適化コード生成器を作るのはその目標に反します。"

Scope & Focus

  • 大きい/混在 PR は焦点を絞るため分割を要求する。 1 つの PR は 1 つの関心事を扱うべき。混在はレビューを難しくし、退行リスクを増やす。

    "これは 2 つに分けようと思います。手間は増えますが。"

  • 枝葉の改善はフォローアップ PR へ。 スコープクリープを抑える。良い案でも PR の中核目的でないなら待つ。

    "別 PR にすべきでしょう。"

Risk & Compatibility

  • 破壊的変更を旗立てし、正式プロセスを要求する。 下流利用者に影響する挙動変更は、文書化、API レビュー、明示承認が必要（内部改善でも同様）。

    "この PR で新 API を導入し、古いチェックの削除は別 PR にしてください。破壊的変更としてマークし、（他の breaking change と同様に）文書化してください。"

  • 退行リスクは影響半径に比例して評価する。 安定したコードへの高リスク変更は、比例して高い価値とより徹底した検証が必要。

    ".NET 10（場合によっては .NET 9）へバックポートしたいので、リスクのある変更は入れたくありませんでした。"

Codebase Fit & History

  • 新コードは既存パターン/規約に一致させる。 逸脱は混乱と不整合を生む。改名や再構成が必要なら、別 PR で一様に実施する。

    "この変更は他の global pointers と不整合です。改名を検討するなら別 PR にし、すべての global pointers に一貫して適用すべきだと思います。"

  • 同様のアプローチが過去に試され却下されていないか確認する。 過去の試行がうまくいかなかったなら、今回は何が違うかの明確な説明を要求する。

    "価値がないなら、特に過去に試して明確な利益がなかったなら、Issue を閉じるべきです。"

Correctness & Safety

エラーハンドリングとアサーション

  • 内部不変条件には例外ではなく Debug.Assert を使う。 内部専用呼び出しなら ArgumentException を投げるより前提を assert する。null 許容抑止演算子 (!) より Debug.Assert(value != null) を優先する。

    "public の呼び出し元が無いなら、これは ArgumentException ではなく assert であるべきです。" — bartonjs

  • 到達可能なエラーパスは throw、網羅 switch の default には UnreachableException。 実行時に到達し得るパスは assert ではなく例外を投げる。網羅 switch の default は throw new UnreachableException() を使う。プラットフォーム未対応には NotSupportedException ではなく PlatformNotSupportedException。ネイティブコードでは _ASSERTE(!"message")。

    "assert より throw を好みます。そうすることで、あるシナリオでここに到達したことがより明確になるからです。" — VSadov

  • 例外メッセージに行動可能な詳細を含める。 パラメータ名は nameof を使う。未対応の型や想定外の値を含める。空の例外は投げない。

    "メッセージを追加してください: throw new ArgumentException($\"Unknown ArrayFunctionType: {functionType}.\", nameof(method));" — jkoritzinsky

  • 出力パラメータは全パスで初期化する。 out パラメータやポインタ出力（bytesWritten、numLocals）がある場合、エラーパスも含めて常に定義済み値で初期化する。

    "numLocals をここ（またはメソッド冒頭）でクリアして、すべてのエラーケースで初期化されるようにできますか？" — jkotas

  • OOM は例外または fail-fast で扱い、assert にしない。 ThrowOutOfMemory や EEPOLICY_HANDLE_FATAL_ERROR を使う。インタプリタループでは nothrow new を使い null チェックする。

    "OutOfMemory の扱いは assert でやるべきではありません。例外を投げるか、例外が無理なら fail fast で落とすべきです。" — jkotas

  • 手書きチェックより ThrowIf ヘルパーを使う。 ArgumentOutOfRangeException.ThrowIfNegative、ObjectDisposedException.ThrowIf などを、手書きの if-then-throw より優先する。

    "この if 条件は不要なはずです。ThrowIfNegative でチェックされます。" — stephentoub

  • 想定外エラーを隠す例外握りつぶしを疑う。 catch { continue; } や catch { return null; } のように例外を黙って捨てる try/catch を追加する場合、その例外が本当に想定された回復可能な条件か、それとも深い問題（競合、メモリ破壊、ビルド環境問題）のシグナルかを問う。"起こるはずがない" 例外を黙殺すると根本原因が隠れ、デバッグが難しくなる。既定の姿勢は、想定外例外は伝播させるか fail fast させ、真因を調べる方向。

    "なぜこのエラーを隠したいのですか？…AOT コンパイラでは、入力が不正ならコンパイルを失敗させるのが一般方針です。不正な入力はコンパイラをクラッシュ/失敗させ得ると想定しています。" — jkotas

スレッド安全性

  • スレッド間フィールドアクセスは Volatile または Interlocked。 あるスレッドで書き、別スレッドで読むフィールドは Volatile<T>、Volatile.Read/Write、Interlocked を使う必要がある。??= はスレッドセーフではない。Nullable<T> はキャッシュに安全ではない（二つのフィールドを持つ構造体で tear が起きる）。同期なしの共有可変配列を使わない。

    "field ??= はスレッドセーフではありません。" — jkotas; "Nullable<int> は 2 フィールドの構造体です。このパターンは tear による競合があります。" — jkotas

  • タイムアウト計算には TickCount64 を使う。 整数オーバーフロー回避のため、Environment.TickCount（int）ではなく Environment.TickCount64（long）を使う。

    "整数オーバーフロー問題を避けるため long と Environment.TickCount64 を使うべきでは？" — jkotas

セキュリティ

  • 整数演算のオーバーフローを防ぐ。 乗算を含むサイズ計算（例: newCapacity * sizeof(T)）はオーバーフロー防御する。正しさが構造的に保証されるパターンを使う。

    "整数オーバーフロー由来のセキュリティバグで多くの傷があります…この変更は、ベストプラクティスに従うコードから、潜在的に脆弱なパターンへ切り替えています。" — jkotas

  • 機密な暗号データは利用後に消去する。 キーマテリアルは CryptographicOperations.ZeroMemory で確実に消す。PinAndClear を使っていても別バッファへコピーするなら元も消す。検証コードではタイミング漏れ防止のため非短絡演算子（|）を使う。

    "GC がコピーを作らないよう PinAndClear を使っているのに、自分でコピーを作って元をクリアしていません。" — bartonjs

  • チャレンジ前に認証情報を先送りしない。 特に Basic 認証は、挑戦（challenge）を受ける前に送るべきではない。

    "これは問題で、セキュリティグループとの調整が難しくなります。特に Basic AUTH は資格情報を漏らします。" — wfurt

  • stackalloc は概ね 1KB を上限とし、サイズを検証する。 ユーザー制御や大きい入力サイズに基づく stackalloc は避ける。stackalloc は使用直前に置き、早期 return の前に確保しない。

    "私たちは stackalloc を概ね ~1K に制限します。" — stephentoub

正しさのパターン

  • 症状ではなく根本原因を直す。 ワークアラウンドや警告抑制を足すのではなく、根本原因を調べて修正する。壊したコミットがあるなら、その上に修正を重ねる前に revert する。

    "リストの破損に関連した他の問題/AV もあり得るので、この修正をそのまま入れるのではなく根本原因を調べましょう。" — jkotas

  • unsafe なマイクロ最適化より安全なコードを優先する。 実証された性能必要性なしに Unsafe.As、Unsafe.AsRef、生ポインタを導入しない。Span ベース API を優先。性能が問題なら JIT の修正を優先。

    "この種のために unsafe コードを導入したくありません。重要なら、キャストは JIT によって消されるべきです。" — stephentoub

  • 同サイズの値型の型詰め替えには Unsafe.BitCast を使う。 Unsafe.As<TFrom, TTo> より Unsafe.BitCast<TFrom, TTo> を優先する。

    "Unsafe.BitCast の方がより正しい（宣言されていないミスアラインアクセスを避ける）うえ、ここでは Unsafe.As より危険が少ないです。" — jkotas

  • 死んだコードと不要なラッパーを削除する。 使われないコード、不要なラッパー、廃止済みフィールド、未使用変数は見つけ次第削除する（特に呼び出し元が変わって唯一の利用が消える場合）。

    "不要なラッパー" / "たまたま気付いた死んだコード" / "m_canBeRuntimeImpl の唯一の使用箇所です。削除できます。" — jkotas

  • SafeHandle.IsInvalid を Dispose の前に扱う。 返された SafeHandle は null ではなく IsInvalid をチェックする。Dispose がエラー状態をクリアする可能性があるため、Dispose 前に例外を取得する。

    "if (handle.IsInvalid) { Exception ex = Interop.Crypto.CreateOpenSslCryptographicException(); handle.Dispose(); throw ex; }" — vcsjones

  • Equals が GetType() による厳密型一致ならクラスを seal する。 継承による微妙なバグを避けるため、GetType() 比較の Equals を持つならクラスを sealed にして失敗クラスを封じる。

    "ContextHolder が sealed でないのに Equals が厳密型一致なのは理由がありますか？この失敗クラスを塞ぐためクラスを seal したいです。" — kg

  • Environment.ProcessPath と AppContext.BaseDirectory を使う。 NativeAOT / single-file 互換のため、Process.GetCurrentProcess().MainModule?.FileName や Assembly.Location の代わりにこれらを使う。

    "Process.GetCurrentProcess().MainModule?.FileName は Environment.ProcessPath と同じはずです。" — jkotas

  • ファイル名の大文字小文字は csproj 参照と完全一致させる。 Linux は大文字小文字を区別する。新規ソースファイルは、フォルダ内の他ファイルが明示列挙されている場合 .csproj に追加が必要。

    "Linux は大文字小文字を区別するので、csproj とファイル名の大小が違ってビルドが落ちています。" — vcsjones

  • 正しさが構造的に保証される設計を優先する。 （例: IL を走査する）正しさが設計的に担保されるアプローチを、手作業で維持する平行データ構造より優先する。最適化を取り逃がす方が、静かに誤ったコード生成になるより良い。

    "可能なら correct-by-construction のアプローチが良いと思います。" — MichalStrehovsky

  • 収集可能性のため正しい loader allocator に割り当てる。 ジェネリック実体化向けランタイム構造の割り当てでは、型引数の collectibility を考慮した正しい loader allocator を使う。

    "MethodInNonCollectibleAssembly<CollectibleType>() を考えると、このメソッド実体化は collectible loader allocator に割り当てるべきです。現状だとメモリリークになります。" — jkotas

  • バックポートは局所修正で、リファクタはしない。 サービシング枝へバックポートするなら、小さく狙い撃ちの修正にする。大きなリファクタのバックポートは不要なリスク。

    "Android 向けに .NET 10 で修正が必要なら、Android ifdef の下に数行追加するだけの小さい狙い撃ち修正にすべきです。" — jkotas

JIT 特有の正しさ

  • JIT lowering で二重 lowering をしない。 すでに lower 済みノードに LowerNode を呼ばない。呼び出し元が lower できるよう新規ノードを返す。定数畳み込みは lowering ではなく import/morph に置く。

    "Lower は通常、同じノードに対して二度呼ぶものではありません。" — EgorBo

  • collectible ALC テストのメソッドは NoInlining にする。 collectible assembly load context に触れるメソッドは [MethodImpl(MethodImplOptions.NoInlining)] が必要。JIT がインラインして参照をローカルに保持し続ける可能性がある。

    "これも no-inlining にする必要があります。JIT がこのメソッドをインラインして、collectible ALC への参照をローカルに残すことは正当です。" — jkotas

Performance & Allocations

計測と根拠

  • 性能変更にはベンチマーク根拠が必要。 マージ前に BenchmarkDotNet や EgorBot の数値を提示する。マイクロベンチだけでなく実世界シナリオで検証する。

    "数値なしの性能変更は、実運用では性能退行である確率が高いです。" — jkotas

  • バイナリサイズ増加は実測で正当化する。 バイナリサイズを増やす変更は、命令数ではなく実アプリの壁時計時間改善を実測で示す。

    "Blazor アプリでの数値（総サイズ/この変更で節約されるバイト数）を見たいです。" — jkotas

  • オブジェクトプール/キャッシュの早すぎる最適化を避ける。 根拠なしにグローバルキャッシュやプールを導入しない。まず本体処理を速くする。

    "このプールは早すぎる最適化に見えます。" — jkotas

割り当て回避

  • ホットパスでクロージャや割り当てを避ける。 ラムダがローカルをキャプチャしてクロージャを生成する場合、state 引数を取る static デリゲート（値タプル）を検討する。文字列連結を避け、Span ベース操作を使う。

    "これはクロージャで data と context をキャプチャしているので、呼び出しごとにクロージャを割り当てています。通常の修正は、コールバックが TState を受け取るようにして値タプルを渡すことです。" — bartonjs

  • サイズが分かるならコレクションを事前確保する。 期待件数が分かるなら Dictionary、HashSet、List のコンストラクタへ capacity を渡す。

    "適切なサイズで dictionary を事前確保できます。" — jkotas

  • 辞書キーの構造体は IEquatable<T> と GetHashCode が必要。 そうでないと比較のために boxing が発生し割り当てが増える。

    "構造体が等値比較ロジックをオーバーライドしないと、ランタイムはボックス化するフォールバックを使います。" — MihaZupan

  • 短命オブジェクトに POH を使わない。 POH はコンパクトされず実質 gen2。プロセス寿命と同程度に生きるオブジェクトに限る。

    "寿命が短い可能性のあるオブジェクトには POH を避けます。通常、プロセスと同じだけ生き残るオブジェクトにだけ使います。" — stephentoub

  • インフラ用タイマーでは ExecutionContext のフローを抑制する。 Timer などのバックグラウンドインフラを割り当てるときは EC フローを抑制し、関係ない AsyncLocal を捕捉してメモリリークするのを避ける。

    "タイマー割り当て時に ExecutionContext を抑制して、関係ない asynclocals を捕捉しないようにしましょう。" — MihaZupan

性能のためのコード構造

  • 安いチェックを高い操作より前に置く。 条件分岐は最も安価/最頻のチェックが先になるよう並べる。早期終了の後に高コスト処理を置く。

    "これらはキャッシュされた安価なビットテストです。先に実行し、モードが一致しないときだけ高価な IL ヘッダデコーダを走らせるべきです。" — jkotas

  • 可能なら遅延割り当てする。 初期化時ではなく初回使用時に高価なリソースを割り当てる。スタートアップで型初期化を強制しない。

    "性能のため、可能な限り遅延させます。" — jkotas; "cDAC のためにスタートアップで初期化を強制しないでください。スタートアップは我々の第一のパフォーマンス問題です。" — jkotas

  • throw ヘルパーを [DoesNotReturn] メソッドへ抽出する。 エラーパスの throw ロジックを別の static ローカル関数やヘルパーへ移し、成功パスを JIT がインラインしやすくする。

    "この if (throwOnFailure) ブロック本体を別の [DoesNotReturn] な throwing static ローカル関数へ移してください。" — stephentoub

  • コレクションやホットパスの O(n²) パターンを避ける。 ループ内の線形探索や、ループ内での RemoveAt の繰り返しに注意。RemoveAll、単一パス化、適切なデータ構造を使う。

    "RemoveParsedValue は線形探索なので、この変更により setter が最悪二乗の複雑度になります。" — MihaZupan

  • 繰り返しのアクセサ呼び出しはローカルにキャッシュする。 同じプロパティ/ゲッター呼び出しの結果はローカル変数に入れる。

    "m_type_data_get_type の呼び出し回数を減らすため、一度ローカルに読み出せませんか？" — lateralusX

  • ランタイム構造ではホットデータとレアデータを分離する。 頻繁にアクセスするデータはインラインに、稀に使うデータ（GCInfo、DebugInfo）は別構造へ。

    "コードヘッダ構造はホット/レアデータを分離するよう意図的に設計されています。" — jkotas

  • 定数データは実行時ではなくコンパイル時に計算する。 インタプリタなどホットパスでは、メタデータ参照や型チェックのような定数計算はコンパイルフェーズで事前計算する。

    "この計算は定数なので、コンパイル時に行うべきです。" — BrzVlad

  • スループットだけでなくスケーラビリティも考える。 高カーディナリティや並行負荷下で、データ構造、キャッシュ、ロック戦略が耐えるか評価する。無制限なコレクション成長、コア数で悪化するロック競合、スケールで破綻する O(1) 仮定に注意。

特定の API 選択

  • AppContext.TryGetSwitch は static readonly プロパティで使う。 static bool Prop { get; } = AppContext.TryGetSwitch(...) のようにキャッシュし、JIT が到達不能パスを DCE できるようにする。

    "private static bool SwitchEnabled { get; } = AppContext.TryGetSwitch(..., out bool enabled) && enabled; こうすると readonly になり、JIT が到達不能コードパスを消せます。" — MihaZupan

  • .NET Core では typeof をキャッシュしない。 typeof(...) は JIT により定数化され、キャッシュは逆効果。同様に ArrayPool.Shared を変数に入れると devirtualization を壊す。

    "typeof(...) のキャッシュは .NET Core では逆最適化です。typeof(...) は JIT で定数になります。" — jkotas

  • 大きい値型辞書ルックアップには CollectionsMarshal を使う。 大きい struct のコピーを避けるために GetValueRefOrAddDefault / GetValueRefOrNullRef を使う。ホットパスでは ValueListBuilder を使う。

    "ここで CollectionsMarshal.GetValueRefOrAddDefault を使えます…大きい EventMetadata struct のコピーを避けられます。" — jkotas

  • blittable struct には Marshal.SizeOf ではなく sizeof を使う。 マーシャリングがない場合、sizeof の方がより正しく、かなり速い。

    "Marshal.SizeOf の代わりに sizeof を使う方がより正しく、ずっと速いです。" — jkotas

  • (uint)index >= (uint)length の慣用境界チェックを使う。 JIT はこのパターンを認識して最適化する。反復前に Span をスライスして、要素ごとの境界チェックを避ける。

    "JIT はこの慣用パターンを認識し、安全なところでは最適化します。" — tannergooding

  • ソースジェネレータは正しく incremental である必要がある。 インクリメンタルパイプラインで Roslyn シンボル（ISymbol、Compilation）を保持しない。出力は決定的で、リストは Ordinal でソートする。

    "ジェネレータを正しく incremental にするか、さもなければ出荷しないでください。そうでないと IDE を殺します。" — Sergio0694

  • 低レベルのコンパイラ系コードベースでは LINQ と record を避ける。 CG2/ILC と AOT ツールでは LINQ の代わりに直ループ、record の代わりに readonly struct を使う。private コードでは interface より具体型を使う。

    "using System.Linq はパフォーマンストラップを避けるため避けたいです。" — MichalStrehovsky

  • BCL の動的配列構築には ValueListBuilder を使う。 ValueListBuilder<T>（プール付き）または ArrayBuilder<T> を使う。小さければ stackalloc、大きければ ArrayPool。

    "ValueListBuilder は BCL で配列を構築するための中央集約型です。" — huoyaoyuan

API Design & Contracts

  • 新しい public API は PR 提出前に承認済み提案が必要。 新規 API 面はすべて API レビューを通す必要がある。未承認 API を追加する PR はクローズされる。実装は承認内容と完全一致しなければならない。

    "未承認 API の PR は受け付けません。" — jkotas

  • API レビュー待ちの新 API は internal を使う。 実装にすぐ必要なら internal にし、別途レビュー依頼を出す。

    "public にするには API Review が必要です。今は internal を使ってください。" — jozkee

  • ref と src でパラメータ名を一致させる。 public API のパラメータ名変更（大文字小文字の変更含む）は、名前付き引数や late-bound で破壊的変更。

    "承認された API では第 2 引数は result ではなく value です。不一致はビルド失敗の理由でもあります。" — vcsjones

  • 例外型と検証順序をプラットフォーム間で揃える。 引数検証（ArgumentNullException、次に ArgumentException）、次に PNSE、次に ObjectDisposedException、最後に操作を行う。例外型はプラットフォーム間で一致させる。

    "私の例外順序は: 1. ArgumentExceptions（null が先、次に論理）2. PNSE 3. ObjectDisposedException 4. 'Do the thing' 例外です。" — vcsjones

  • Try API は一般的に起きる想定失敗でのみ false を返す。 それ以外（破損、権限、無効引数）は throw。Try メソッドは無効引数では必ず throw すべき。

    "Try... API の通常契約は、特定（最も一般的）な理由でのみ false を返し、それ以外はすべて throw です。" — jkotas

  • 構築後に可変 options を露出しない。 構築時に値をキャプチャするなら、可変 options オブジェクトを公開しない。ユーザー向けエラーメッセージで private フィールド名や内部型を参照しない。

    "構築後に可変な ZLibCompressionOptions を公開すると誤解を招くかもしれません。" — iremyux

  • プラットフォーム制約には PlatformNotSupportedException を使う。 現環境では完了できないが別プラットフォームなら可能な操作には PNSE を投げる。OS の能力以上の人工的制限を課さない。

    "System.IO の一般方針は、基盤の制限を追加の人工的制約を課さずに表面化することです。" — jkotas

  • .NET API はプラットフォームの癖を吸収すべき。 public API はプラットフォーム間で一貫して動くべき。オーバーロード追加時は、暗黙変換の曖昧さによる F# 互換性も確認する。

    "プラットフォームとしての .NET の付加価値は、基盤の癖を補償して『動くようにする』ことです。" — jkotas

  • 非推奨化は obsoletion プロセスに従う。 次の利用可能な SYSLIB 診断 ID を選び、[Obsolete] を付け、オーバーロード修正では [EditorBrowsable(Never)] と [OverloadResolutionPriority(-1)] を使う。

    "我々には obsoletion プロセスがあり、次の利用可能な SYSLIB id を選ぶことが含まれます。" — eiriktsarpalis

  • 新しい GC-EE インターフェイスメソッドは末尾に追加する。 vtable スロット順序を維持するため、常にインターフェイスの最後のメソッドとして追加する。

    "既存メソッドの vtable スロットを変えないため、これはインターフェイスの最後のメソッドである必要があります。" — jkotas

  • 新しい virtual メソッドは未 override の派生型でも動く必要がある。 既定実装は、従来の同等 API を呼ぶのと同一挙動であるべき。

    "新しいメソッドは、まだ override していない派生 writer と一緒に使われることを想定しなければなりません。" — stephentoub

  • public API の長さに符号なし型を避ける。 長さ引数は int か long を優先。ファイル境界を跨ぐなら ValueTuple ではなく名前付き型を使う。

    "public API の長さに符号なし型は一般に使いません。" — rzikm

  • 中核コンポーネント変更は Issue から始める。 host / VM / JIT 変更は、PR を出す前に問題と動機を説明する GitHub Issue を起点にすべき。

    "この種の変更は中核コンポーネントを変えるもので考慮点が多いので、本来 Issue から始めるべきです。" — AaronRobinsonMSFT

Code Style & Formatting

  • マジックナンバーではなく意味のある定数名を使う。 説明なしの生の hex/decimal 定数は不可。マジック定数をファイル間で重複させない。

    "0x7F00 は一般読者には何も語りません。(float)Int128.MaxValue を意味することを説明するコメントを足せば、意味が出ます。" — tannergooding

  • 型が文脈から明白な場合にのみ var を使う。 キャスト、戻り値、async インフラなどでは明示型を使う。数値型で var は使わない。

    "これらの var の使い方は受け入れがたいように見えます。" — bartonjs

  • 定数は PascalCase、bool は説明的で肯定形の名前にする。 定数ローカル/フィールドは PascalCase（外部名に合わせる interop 定数は例外）。bool フィールドは肯定形かつ説明的（valid ではなく _hasCurrent）。

    "定数ローカル変数とフィールドはすべて PascalCasing を使います。" — bartonjs

  • メソッド名は挙動を正確に反映する。 挙動が変わったら名前も更新する。Get* は戻り値を示唆するので、void なら Print*/Display* を使う。ThrowIf であって ThrowExceptionIf ではない。

    "この変更後、このメソッドは何も返しません。なので Get... という名前は合いません。" — jkotas

  • ネスト削減のため早期 return を優先する。 短い/エラーケースで早期 return を使い、不必要なネストを避ける。エラーケースを先、成功 return を最後に。

    "else ブロックでインデントを増やすより、if (...) { return ...; } の形にできますか？" — stephentoub

  • 新規コードでは using static と #region を避ける。 using static は IDE なし（GitHub レビュー等）で読むコストが高い。#region はすぐ古くなる。

    "using static は IDE がないときの読解コストを天文学的に増やします。" — bartonjs

  • ローカル関数はメソッド末尾、フィールドは型の先頭に置く。 ローカル関数は包含メソッドの末尾。フィールドは型の最初のメンバーとして宣言する。

    "合意されている一般パターンは、ローカル関数はメソッドの末尾に置くことです。" — AaronRobinsonMSFT

  • 警告抑制は最小スコープに限定する。 ファイル全体 #pragma 抑制を避け、警告を引き起こす特定行の周辺だけで無効化する。

    "広い範囲で警告を抑制するのは一般に悪いプラクティスです。" — AaronRobinsonMSFT

  • パターンマッチングと is/or/and パターンを使う。 手動の型チェックや比較より C# のパターンを優先する。bool 引数には名前付き引数を使う。

    "return !(typeDesc.Category is TypeFlags.Boolean or TypeFlags.Char);" — jkotas

  • フィールドを既定値で初期化しない（CA1805）。 CLR はフィールドをゼロ初期化する。= false、= 0、= null は冗長。

    "CA1805: 不要な初期化をしないでください。" — MichalStrehovsky

  • sealed クラスはフル Dispose パターンが不要な場合が多い。 派生が finalizer を追加できないので、単純な Dispose() で十分。

    "クラスが sealed になったなら、フル dispose パターンは不要だと思います。" — Youssef1313

  • 過剰な case 羅列よりテーブル駆動を好む。 HW intrinsic などパターンが多いコードでは、明示的な case を大量に追加するより lookup テーブル（AuxiliaryJitType、SpecialCodeGen フラグ）を使う。

    "大量のテーブルエントリを足すより、AuxiliaryJitType を使って SpecialCodeGen にする方が良いと思います。" — tannergooding

  • struct フィールド順序でパディングを最小化する。 C/C++ の struct 定義ではサイズ順（ポインタ先）に並べる。

    "パディングを避けるためポインタを先に置けませんか？" — lateralusX

コードベースのパターンとの一貫性

PR 衛生

  • PR は宣言したスコープに集中する。 意図しないファイル変更、無関係なリファクタ、空白ノイズ、ビルド成果物の混入を避ける。PR は 1 目的。

    "PR ではもっと意図的にしてください…とても濁ったソース履歴になります。" — bartonjs

  • 大きなリファクタや改名は別 PR。 no-diff リファクタは機能変更と分離する。機械的な改名はロジック変更と分離する。

    "私は常に、まず no-diff リファクタをして、その上に差分のある変更を積む方を好みます。" — AndyAyersMS

  • まず main にマージしてから、リリース枝へバックポート。 /backport コマンドを使う。サービシングへのバックポートはセキュリティバグ、退行、信頼性問題に限定。

    "一般に、性能関連の修正は基準を満たしません。重大な退行を直す場合は別です。" — jkotas

コード再利用と重複排除

  • 重複ロジックは共有ヘルパーへ抽出する。 共有ヘルパーで改善すれば全呼び出し元が恩恵を受ける。

    "複製するよりヘルパーメソッドに移した方が良いでしょうか？" — tarekgh

  • 共有コードは共有ファイルへ。ランタイムごとに複製しない。 CoreCLR と NativeAOT で同一コードがあるなら shared パーティションへ移す（必要なら #if !MONO）。

    "NativeAOT と CoreCLR の間に同一コードがかなりあります。shared ファイルへ移せますか？" — jkotas

  • 並行する API を作るより既存 API を使う。 新しい型/enum/ヘルパー導入前に既存で同目的を満たせないか確認する。重複を作らず既存ユーティリティを直す。

    "既存の SignatureAttributes.Instance を使えませんか？意味は同じです。" — jkotas

  • 死んだコードと未使用宣言は積極的に削除する。 コードを削除するなら、関連ヘルパー、enum 値、関数宣言、resx 文字列も未使用なら削除する。

    "この関数は使われていません。削除してください。" — davidwrighton

確立済み規約

  • エラー文字列はコード直書きではなく .resx に置く。 SR クラス経由で参照する。resx 文字列を使うコードを削除したら未使用文字列も削除する。

    "文字列メッセージをコードに保存しません。代わりに .resx に保存し（必要ならフォーマット引数付き）、SR で参照します。" — huoyaoyuan

  • リストやエントリはアルファベット順に並べる。 areas、設定、resx エントリ、エントリポイント/エクスポート一覧、ref source メンバーなどはアルファベット順を維持する。

    "areas のリストはアルファベット順に並んでいるようです。" — jkotas

  • 自動生成ファイルや eng/common を手で直さない。 ジェネレータや元定義を直す。eng/common は dotnet/arcade から同期される。

    "eng/common のものは dotnet/arcade から来ています。ここでの修正は次回同期で上書きされます。" — vcsjones

  • 環境変数は COMPlus_ ではなく DOTNET_ プレフィックスを使う。 新しいランタイム環境変数は DOTNET_ のみ。

    "COMPlus 名はレガシーで、段階的に廃止したいです。新機能で追加対応すべきではないと思います。" — agocke

  • 変更したファイルでは既存スタイルに合わせる。 一般ガイドよりファイル固有のスタイルが優先。スタイルだけのために既存コードを変えない。

    "ファイルがガイドラインと違うスタイルでも、そのファイルの既存スタイルが優先されます。" — huoyaoyuan

  • Unsafe.SizeOf ではなく sizeof を一貫して使う。 Unsafe.SizeOf を sizeof に置換するパスが実施済み。再導入しない。

    "Unsafe.SizeOf を削除して sizeof に置換するパスは実施済みです。再導入しないでください。" — jkotas

ランタイム固有パターン

  • ランタイム変更では NativeAOT の同等対応も検討する。 CoreCLR の挙動を変えるなら、NativeAOT にも同じ変更が必要か確認する。

    "変更したコードは NativeAOT では使われていません。NativeAOT にも同じ変更が必要ですか？" — jkotas

  • インタプリタの挙動を通常 JIT と揃える。 命名、エラーコード（CORJIT_BADCODE）、マクロ（NO_WAY）などを揃える。FEATURE_INTERPRETER ガードを使う。

    "通常の JIT と同様に NO_WAY と呼ぶべきでは？インタプリタ JIT が通常 JIT に近いほど良いと思います。" — jkotas

  • ソースジェネレータ: ファイルロック禁止、診断は analyzer のみ。 ジェネレータは不正状態をうまく回避し、診断は別 analyzer が出す。

    "ジェネレータがディスク上のファイルをロックしてはいけません。" — jkoritzinsky

  • 参照アセンブリの規約。 using なし（型は完全修飾）、空のメソッド本体または throw null、genapi 風フォーマット、メンバーはアルファベット順。TFM 固有 API は別ファイルへ。

    "一般に ref source には usings がありません。" — vcsjones

Testing

  • バグ修正と挙動変更には必ず回帰テストを追加する。 新規ファイル作成より既存テストファイルへ [InlineData] を追加するのを優先。新しいテストファイルは csproj に含める。

    "この PR には回帰テストが必要です。TypeInfoTests.cs は良い追加先です（新しい InlineData を追加）。" — jkotas

  • プラットフォーム固有テスト属性を正しく使う。 skip ロジックは実行時 if ではなく [PlatformSpecific]、[ConditionalFact]、[ActiveIssue] を使う。SkipTestException を機能させるには ConditionalFact が必要。

    "テスト内で throw new SkipTestException するなら conditional fact である必要があります。" — jkotas

  • エッジケース、エラーパス、影響する全型をテストする。 空文字、負値、境界条件、トルコ語の 'i'、サロゲートペアなど。bool オプションは true/false の両方。出力が触られていない場合でも偶然パスしない入力を選ぶ。

    "出力が全く触られていなくてもテストが通らないよう、すべて 0 にデコードされない入力を選んでください。" — MihaZupan

  • テストのアサーションは具体的に。 大雑把な条件ではなく、期待値（OperationStatus、バイト数など）を正確に検証する。修正を revert すると確実に失敗するテストにする。

    "今の asserts は広すぎて有用ではありません。" — MihaZupan

  • 不安定で価値の低いテストはパッチ当てでなく削除する。 フレークだと分かっているテストは追加しない。実行時の脆い詳細に依存し安定化できないなら削除を優先。

    "そのテストは削除した方が良いでしょう。不安定テストを追加しても意味がありません。" — jkotas

  • テストデータを決定的かつカルチャ非依存にする。 明示的な書式設定をした CultureInfo を作る。個別 [Fact] より [Theory] と [InlineData] を優先。

    "テストで var culture = new CultureInfo(\"de-DE\"); culture.DateTimeFormat.AbbreviatedMonthGenitiveNames = [...] のような culture を作ることを提案します。" — tarekgh

  • テストのパスワードには PLACEHOLDER を使う。 資格情報スキャンの誤検知を避ける。

    "credscan がテストに過敏になることがあります。推奨は PLACEHOLDER を使うことです。" — bartonjs

  • CI には checked ビルドを使い、回帰テストの優先度は低め。 CoreCLR の debug ビルドは遅い。CI は checked を使う。新規 JIT 回帰テストは通常 CLRTestPriority 1。

    "CoreCLR の Debug ビルドはとても遅いです。通常、テストには checked ビルドを使います。" — jkotas

  • プロセス全体で共有される状態を触るテストは RemoteExecutor を使う。 共有状態を変えるテストは隔離のため RemoteExecutor を使う。固定パスを避け、一時ファイルを使う。Microsoft.CodeAnalysis.CSharp のような重い依存をテストアセンブリに追加しない。

    "これらのテストで Microsoft.CodeAnalysis.CSharp への依存を避けてください。そうすると、このテストアセンブリ全体がデバイス、wasm、nativeaot で実行できなくなります。" — jkotas

  • fuzz テストでは想定される例外だけを catch する。 すべての例外を catch すると、API から文書化されていない例外が漏れてくるバグを隠してしまう。

    "ここで期待する例外だけを catch できますか？…それによってライブラリが文書化されていない例外を投げているのを見つけられました。" — adamsitnik

  • xUnit ベースのテストはモダンな xUnit パターンを使う。 xUnit テストプロジェクト（例: 多くの libraries tests）では、レガシーな return 100 == success ではなく Assert.* を使い、[Fact]/[Theory] を使う。キャンセルには ThrowsAnyAsync<OperationCanceledException> を優先。回帰テストクラス名は issue 番号（例: Runtime_117605）に合わせる。src/tests 配下のレガシー非 xUnit テストは既存の return 100 規約継続可。

    "ここでテストを Asserts を使う形に変えて、レガシーな 'return 100 == success' モデルをやめられますか？" — jkoritzinsky

  • テスト出力量を減らす。 コンソールに MB 単位の出力を出さない。ビジーループの代わりに Thread.Sleep を使い、反復回数を減らす。

    "これはメガバイト単位の出力になります。Thread.Sleep(10) のように目立たない方法にして、for ループを 200 くらいまでにできませんか？" — jkotas

  • 回帰テストディレクトリの命名規約に従う。 src/tests/Regressions/coreclr/ では、ディレクトリ名は GitHub_<issue_number>、テスト名は test<issue_number>。

    "既存パターンに従ってください。ディレクトリ名は GitHub_122933、テスト名は test122933 です。" — jkotas

Documentation & Comments

  • コメントはコードの言い換えではなく理由を書く。 // Get the types のような英語でコードをなぞるだけのコメントは削除する。なぜコードが変わったかの歴史的経緯を書かない。

    "英語でコードをそのまま言い換えるコメントはあまり有用ではありません。このコメントは『なぜこれをしているのか』を説明すべきです。" — jkotas

  • コード変更時に古いコメントは削除/更新する。 古い挙動を説明するコメントは、コメントがないより悪い。

    "Note: で始まるコメント全体は削除できます。もはや当てはまりません。" — jkotas

  • 先送り作業は GitHub Issue と検索可能 TODO で追跡する。 TODO コメントに追跡 Issue を参照し、一貫したプレフィックス（例: TODO-Async:）を使う。二度とやらない古い TODO は削除する。

    "レビューが必要な箇所に async TODO を付けて、検索で見つかりやすくし、取りこぼしがないようにしてもらえますか？" — jkotas

  • インターフェイス実装側にコメントを重複させない。 ドキュメントコメントはインターフェイス定義側に置く。重複は乖離を生む。

    "インターフェイスにコメントがあれば十分です。重複すると、時間とともにコメントが乖離します。" — jkotas

  • 新しい public API にはすべて XML doc コメントを付ける。 これらは learn.microsoft.com の公式 API ドキュメントの種になる。プロパティは "Gets the ..." / "Gets or sets the ..." で始める。テストコードに XML doc を付けない。

    "新しい API すべてに /// コメントも含めてください（API docs の種になります）。" — MihaZupan

  • ドキュメント内リンクは SHA 固定またはコミット固定にする。 ファイル移動で壊れるブランチ相対リンクは使わない。

    "sha 固定リンクが最良です。" — richlander

  • メタデータコードでは ECMA-335 と仕様ソースを参照する。 署名やメタデータ解析では該当する ECMA-335 の節を引用する。暗号テストベクタでは CAVP/ACVP の出典を引用する。

    "ここで従っている ECMA-335 の署名形式を参照してはどうでしょうか。" — AaronRobinsonMSFT

  • 挙動変更には breaking change 文書化を行う。 dotnet/docs にテンプレートで Issue を開き、.NET Breaking Change Notification DL に通知する。プレリリース間の変更でも適用。

    "breaking change テンプレートで破壊を説明する Issue を開くだけでよいです。" — tannergooding

  • ユーザー向け文言では確立済みの用語を使う。 内部型名、private フィールド名、"Roslyn" のようなコードネームを公開ドキュメントやエラーメッセージに出さない。

    "'non-explicit type' は確立した用語ではありません。" — jkotas; "Roslyn は内部コードネームです。公開ドキュメントでは使うべきではありません。" — jkotas

  • 著作権ヘッダとライセンス情報を保持する。 C# と C++ の全ソースファイル（テスト含む）に標準ライセンスヘッダが必要。他プロジェクトから移植する場合は元著作権を保持し、THIRD-PARTY-NOTICES.TXT を更新する。

    "C# と C++ のソースファイルはすべて（テスト含む）ライセンスヘッダを持つべきです。" — jkotas

Platform & Cross-Platform

  • エンディアン安全な読み取りには BinaryPrimitives を使う。 ポインタキャストではなく ReadInt32LittleEndian/BigEndian を使う。エンディアン依存の読み取りとターゲットエンディアン読み取りを分離する。

    "これを 64-bit 値として読んでいるなら…big endian では結果が OverflowException です。" — tmds

  • ISA 固有 intrinsic よりクロスプラットフォーム vector API を使う。 Avx512BW や SSE2 より Vector128/256/512.IsHardwareAccelerated と xplat API（.Shuffle、.Min）を優先。移植性のあるビット操作には BitOperations。

    "xplat API を使うよう更新したいですか？Avx512BW.IsSupported -> Vector512.IsHardwareAccelerated に置き換えては。" — tannergooding

  • 正しいプラットフォーム/機能 define を使う。 コンパイラ提供 define（__wasm__）ではなく TARGET_*/HOST_* を使う。HOST_* はビルドマシン、TARGET_* はターゲット。未実装プラットフォームコードには PORTABILITY_ASSERT。

    "例えば HOST_WINDOWS と wasm を並べるようなスタイルの混在は良くありません。" — jkotas

Native Code & Interop

C++ スタイル

  • ランタイム C++ コードベースで auto を使わない。 明示型を使う。例外: ラムダのような表現不能型。

    "runtime コードベースでは auto を使いません。" — AaronRobinsonMSFT

  • nullptr、void*、ネイティブ C++ 型をレガシー別名より優先。 NULL より nullptr、LPVOID より void*。Windows host コードでは wchar_t ではなく WCHAR。多重 include 用ファイルは .inc。

    "新しいコードの大きな塊では nullptr を好みます。" — jkotas; "LPVOID はレガシーな荷物を背負った Windows SDK の void* 別名です。" — jkotas

  • #endif コメントを #ifdef と正確に一致させる。 非自明なブロックでは #else/#endif にコメントを追加。ブレース配置と 4 スペースインデントを一貫。

    "CoreCLR の一般スタイルは、#else があっても #ifdef と #endif コメントを正確に一致させることです。" — jkotas

  • C スタイル cast より static_cast を優先する。 C スタイル cast は必要以上に寛容で、暗黙に reinterpret_cast に落ち得る。

    "static_cast<> は、許容できる限り狭い契約を強制するものです…C スタイル cast は潜在的な reinterpret_cast<> です。" — AaronRobinsonMSFT

ランタイム/VM パターン

  • 正しい VM contract と QCall パターンを使う。 例外を投げ得る QCall は BEGIN_QCALL/END_QCALL が必要。単純な QCall は QCALL_CONTRACT_NO_GC_TRANSITION。全 VM メソッドは STANDARD_VM_CONTRACT または WRAPPER_NO_CONTRACT。

    "例外を投げるなら QCall には BEGIN_QCALL と END_QCALL が必要です。" — AaronRobinsonMSFT

  • managed 参照周辺の GC 保護を正しく保つ。 GC を起こし得る呼び出しの前に、すべての GC 参照を GCPROTECT する。GC を起こし得る呼び出し後は ObjectFromHandle(handle) で新しい参照を取得する。

    "MethodDescCallSite は GC をトリガーします。呼ばれるときはすべての GC 参照を保護する必要があります。" — jkotas

  • 致命的エラーパスで動的割り当てを避ける。 スタック割り当てバッファを使う。Monitor/lock より単純な同期（Interlocked + spin-wait）を使う。

    "これは OOM により到達し得る致命的エラーパスなので、ここでメモリ割り当てをしない方が良いです。" — janvorli

  • CoreCLR で destructor 付き thread-local オブジェクトを避ける。 破棄順序は任意。CoreCLR Thread オブジェクトに寿命を紐づける。性能クリティカルでは C++ thread_local より minipal の PLATFORM_THREAD_LOCAL を優先。

    "デストラクタ付きの thread local オブジェクトは、破棄順序が任意であるため問題の原因になり得ます。" — janvorli

  • 未整列書き込みの可能性がある場合は SET_UNALIGNED マクロを使う。 コード生成スタブではポインタの直接デリファレンスではなく SET_UNALIGNED_32/64。

    "SET_UNALIGNED_64 を使うべきです。" — jkotas

  • 部分的にしか使われない可能性がある配列/バッファはゼロ初期化する。 デストラクタを持つ要素の配列はゼロ初期化する。EH テーブル、C 配列なども同様。

    "配列内容をゼロ初期化すべきでは？そうでないと途中で例外が起きた場合にデストラクタが未初期化メモリへアクセスし得ます。" — jkotas

  • ハードコードした構造オフセットには static assert を追加する。 特にアセンブリで struct フィールドへアクセスするためのハードコードオフセットには、それが有効であることを検証する static assert を追加。

    "これらのオフセットが有効であることを検証する static assert を追加すると良いでしょう。将来の変更で壊れるのが心配です。" — janvorli

  • 新しいプラットフォーム抽象化は minipal を使う。 CoreCLR 新規コードの抽象化は PAL（レガシー）ではなく minipal（新）。関数外から呼ばれるアセンブリラベルには LOCAL_LABEL ではなく ALTERNATE_ENTRY。

    "minipal はプラットフォーム依存を抽象化する新しい場所です。" — janvorli

  • printf ではなく JITDUMP と LOG マクロを使う。 JIT コードでは JITDUMP、CoreCLR VM では LOG()/LOGGING define。製品コードのネイティブでは printf や Console.WriteLine を使わない。

    "printf を呼ぶのではなく、JITDUMP または代替 API を使うべきです。" — tannergooding

P/Invoke とマーシャリング

  • ネイティブ相互運用の bool マーシャリングは 4 バイト BOOL を優先。 UnmanagedType.Bool を使う。P/Invoke の戻り値型はネイティブ署名と厳密一致させること。64-bit では動いても 32-bit/WASM で壊れ得る。

    "bool のマーシャリングは昔からバグが多い領域です。4 バイト bool（UnmanagedType.Bool）が最もバグりにくい選択肢になりがちです。" — jkotas

以上です。お疲れ様でした。

参考

脚注

  1. PR Digest.NET dotnet/runtimeにマージされたPull RequestをAIで日本語要約 — https://prozolic.github.io/PRDigest.NET/ ↩︎

  2. Review code changes in dotnet/runtime for correctness, performance, and consistency with project conventions. Use when reviewing PRs or code changes. — https://github.com/dotnet/runtime/blob/main/.github/skills/code-review/SKILL.md ↩︎

  3. Fix incorrect Vector256->Vector512 in SpanHelpers — https://github.com/dotnet/runtime/pull/123850 ↩︎

  4. Fix BitArray to clear dangling high bits in LeftShift and deserialization — https://github.com/dotnet/runtime/pull/123296 ↩︎

  5. Add ProcessExitStatus class to System.Diagnostics.Process — https://github.com/dotnet/runtime/pull/124264 ↩︎

  6. Jules gains memory! — https://jules.google/docs/changelog/#jules-gains-memory ↩︎

Discussion