From d0a7817996f813fa5b776122c0ae1ede02e08d14 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Fri, 16 Oct 2026 15:25:53 -0700 Subject: [PATCH 1/2] Experimental Gemini Code Assist configuration Add configuration for the Gemini Code Assist GitHub app to experiment with automatic code reviews. The hope is that this will especially help with initial feedback for PRs from new contributors. Also add code review instructions in .gemini/styleguide.md. These instructions are derived from our code review history and will help guide the reviews from GCA. --- .gemini/config.yaml | 16 ++++ .gemini/styleguide.md | 176 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 .gemini/config.yaml create mode 100644 .gemini/styleguide.md diff --git a/.gemini/config.yaml b/.gemini/config.yaml new file mode 100644 index 00000000000..4467e058285 --- /dev/null +++ b/.gemini/config.yaml @@ -0,0 +1,16 @@ +# Configuration for Gemini Code Assist on GitHub. +# See https://docs.cloud.google.com/gemini/docs/code-review/customize-repo-review +have_fun: false +code_review: + disable: false + comment_severity_threshold: MEDIUM + max_review_comments: -1 + pull_request_opened: + help: false + summary: false + code_review: true + include_drafts: false +ignore_patterns: + - "test/lit/help/**" + - "test/spec/testsuite/**" + - "third_party/**" diff --git a/.gemini/styleguide.md b/.gemini/styleguide.md new file mode 100644 index 00000000000..76dbae42b3c --- /dev/null +++ b/.gemini/styleguide.md @@ -0,0 +1,176 @@ +# Binaryen Code Review Guide + +Binaryen is a compiler and toolchain infrastructure library for WebAssembly, +written in C++20. Focus code reviews on **correctness, WebAssembly and Binaryen +IR invariants, soundness of optimizations, test quality, and code clarity** +while avoiding low-signal stylistic nitpicks. + +## Review Priorities & Noise Reduction + +* **High signal-to-noise ratio:** Only comment when there is a concrete + correctness bug, missed edge case, violated project convention, or clear + simplification. +* **Do not comment on formatting, whitespace, or braces:** C++ formatting and + brace style are enforced by deterministic CI linters (`git clang-format`). +* **Do not flag auto-generated `CHECK` lines in `test/lit/`:** `CHECK:` and + `CHECK-NEXT:` lines in `.wast` and `.test` files are generated by + `scripts/update_lit_checks.py` and `scripts/update_help_checks.py`. Instead, + inspect the `CHECK` output to verify that the test actually exercises the + intended code path and produces sound output. +* **Do not ask for `CHANGELOG.md` entries on bug fixes or minor improvements:** + `CHANGELOG.md` only tracks breaking changes, new passes, and major + user-visible features. + +## Binaryen IR & Optimization Pass Invariants + +* **Keep `src/wasm.h` minimal:** Avoid adding pass-specific state, annotations, + or single-use helpers to `src/wasm.h`. Keep comments in `src/wasm.h` focused + on high-level IR semantics rather than pass-specific algorithmic details. Do + not add default member initializers to `Expression` subclasses in + `src/wasm.h`; IR nodes should be constructed via `Builder` + (`src/wasm-builder.h`). +* **Type updating & `ReFinalize`:** + * Whenever a transformation can change or refine an expression's type (e.g., + replacing a node with one of a more specific subtype, replacing an + expression with `(unreachable)`, or refining function parameter/result + types), `ReFinalize` must be run so enclosing expressions update their + types. + * `ReFinalize` is a function-parallel pass, so + `ReFinalize().run(getPassRunner(), wasm)` only walks function bodies. Passes + that modify or construct expressions in module-level locations (global + initializers, table initializers, element segments, data segment offsets) + must also call `ReFinalize().walkModuleCode(wasm)` if types can change. +* **Unreachable code & subtyping:** + * Inside function bodies, any expression operand may have `Type::unreachable` + (whereas module-level initializers cannot contain `unreachable`). Always + check for `Type::unreachable` (or check `type.isRef()`, `type.isTuple()`, + etc.) before calling accessors like `getHeapType()` that assert on + `unreachable`. + * A parent expression has `Type::unreachable` whenever *any* of its child + operands is `unreachable`. Do not assume `curr->type != Type::unreachable` + is required for another child operand to have a concrete or reference + type—check the child operand's type directly. + * In Binaryen IR, expressions of any type (such as `Type::none` calls) can + appear at `block->list.back()` after an earlier `unreachable` element, even + when the block or function has a concrete result type. Passes inspecting + tail position must verify `Type::isSubType(expr->type, expectedType)`. + * Prefer `Type::isSubType(sub, super)` and `HeapType::isSubType(sub, super)` + over `==` unless exact type equality is required. +* **Traps, effects, and `PassOptions`:** + * Never optimize a guaranteed trap into `(nop)` (even in unreachable or + closed-world code); replace guaranteed traps with `(unreachable)` so DCE and + subsequent passes can remove dead code after the trap. + * Check `PassOptions` flags appropriately: respect `options.trapsNeverHappen` + (`-tnh`) before assuming traps cannot occur at runtime, + `options.closedWorld` before making closed-world assumptions, and + `options.shrinkLevel == 0` before performing optimizations that increase + code size. + * Use `EffectAnalyzer` (`src/ir/effects.h`) to check side effects and + instruction reordering validity (`orderedAfter`, + `hasUnremovableSideEffects`, etc.). Note that `mayNotReturn` (e.g., infinite + loops) is distinct from `transfersControlFlow()` (branches, returns, + throws). +* **Legacy Exception Handling `pop`s:** + * Legacy EH `catch` bodies use `pop` expressions, which must remain direct + children of the `catch` body and cannot be nested inside an inner `block`. + Passes that introduce a `Block` around arbitrary expressions (e.g., via + `Builder::makeSequence` or `ChildLocalizer`) must call + `EHUtils::handleBlockNestedPops(func, *getModule())` when `pop`s may be + present. +* **Walkers, visitors, and pass structure:** + * Distinguish `Visitor` (visits a single expression) from `Walker` (traverses + a subtree, such as `PostWalker`, `ExpressionStackWalker`, + `ControlFlowWalker`, `CFGWalker`). Give walker/visitor classes descriptive + names (e.g., `ReferenceScanner`) rather than generic `Visitor`. + * In function-parallel passes (`isFunctionParallel() == true`), each function + gets a fresh pass instance, so per-function member state does not need + manual clearing in `doWalkFunction`. + * Do not manually invoke general cleanup passes like `dce` inside an + optimization pass unless required for correctness; rely on the pass pipeline + to run cleanup passes. + * Reuse existing helpers in `src/ir/` (`Properties`, `Bits`, `ChildLocalizer`, + `ModuleUtils`, `TableUtils`, `MemoryUtils`, `LUBFinder`, + `SubtypingDiscoverer`, `Match` in `src/ir/match.h`) instead of duplicating + logic. + +## Validation, Binary Format, and New Instructions + +* **Validator control flow (`src/wasm/wasm-validator.cpp`):** `shouldBeTrue` and + `shouldBeEqual` record an error and return `false`; they do **not** return + early from the visitor. If subsequent checks dereference pointers, call type + accessors like `getStruct()`, or index into vectors (e.g., `fields[index]`) + assuming the check passed, guard them with `if (!shouldBeTrue(...)) { return; }`. +* **Binary encoding (`src/wasm/wasm-stack.cpp`, `src/wasm/wasm-binary.cpp`):** + All prefixed opcodes (`AtomicPrefix`, `GCPrefix`, `SIMDPrefix`, `MiscPrefix`) + must encode their sub-opcode using `U32LEB(...)`, not `int8_t`. +* **New instruction checklist:** When adding a new instruction or type, verify + that all relevant components from `Contributing.md` are updated (IR, builder, + traversal, validator, interpreter, `effects.h`, parser, printer, binary + reader/writer, and fuzzer support in `src/tools/fuzzing/`). + +## C++ Coding Style & Idioms + +* **Naming:** Use `PascalCase` for types/classes/structs and `camelCase` for + functions, methods, parameters, and variables (avoid `snake_case` variable + names and trailing underscores on private fields). +* **Control flow:** Prefer early returns (`if (!cond) { return; }`) to reduce + indentation. Omit redundant early returns immediately before a loop that + naturally does nothing when the container is empty. +* **Assertions & errors:** Use `assert(...)` for internal invariants and + `Fatal() << ...` for unhandled cases or fatal errors. Because Binaryen enables + assertions in Release builds by default (`-UNDEBUG`), avoid placing expensive + linear scans inside hot-path `assert(...)` checks. +* **Types & C++20 idioms:** + * Use `Index` and `Address` instead of raw `int` or `size_t` for Wasm indices + and memory offsets. + * Use `expr->dynCast()` for conditional casts and `expr->cast()` (or + `expr->is()`) when the dynamic type is known. + * Prefer `.contains(...)` over `.count(...)` on associative containers, and + prefer pre-increment (`++i`) over post-increment (`i++`). + * Prefer explicit named setters (e.g., `setEmitModuleName(...)`) or enums over + opaque boolean constructor parameters. +* **Comments:** + * Place comments on their own lines above the code rather than as trailing + end-of-line comments. + * Explain non-obvious invariants, algorithms, and edge cases in comments, and + update or remove stale comments when nearby code changes. + +## Testing Guidelines (`test/lit/`, `test/gtest/`) + +* **Test coverage:** Bug fixes, pass improvements, and new features must include + tests. Pure NFC refactorings do not require new tests. +* **Lit test organization (`test/lit/**/*.wast`):** + * Add test cases to existing `.wast` files when appropriate; otherwise give new + files descriptive names (never name test files after GitHub issue numbers). + * Write `.wast` tests using folded S-expressions. + * Place comments explaining individual test functions **inside** the + `(func ...)` body (not immediately above `(func ...)`), so + `scripts/update_lit_checks.py` does not insert `CHECK` lines between the + comment and the function. + * Add module-level `;;` comments above `(module ...)` blocks in multi-module + tests to explain what each module tests. + * Do **not** use `foreach` in the `RUN:` line of a `.wast` file that only + contains a single `(module ...)`. + * Keep tests minimal and targeted at the specific pass or feature under test; + verify that the `CHECK` output demonstrates the expected transformation and + that no stale `check-prefix` expectations remain. + +## PR Descriptions & Commit Messages + +* **Concise, direct prose:** Commit messages and PR descriptions should use + concise, natural prose paragraphs that explain *why* the change is being made + (the bug, motivation, or invariant) and the high-level approach. Trivial + fixes only need a sentence or two and an issue reference (e.g., `Fixes #1234`). +* **Describe only the final state:** Commit messages and PR descriptions should + describe the final state of the change, not the chronological history of + intermediate attempts or revisions made during review. +* **No AI slop:** Flag verbose, formulaic LLM-generated PR descriptions and + commit messages, including: + * Boilerplate section headers (`## Summary`, `## Key Changes`, `## Test Plan`, + `## Impact`) and robotic bulleted file-by-file summaries that merely restate + the diff. + * Filler or promotional language ("comprehensive", "robust", "seamless", + "enhances maintainability"). + * AI attribution or co-author trailers (e.g., `Co-Authored-By: Claude ...`, + `Generated with ...`). Per `Contributing.md`, the human contributor is + always the sole author and is fully accountable for the submission. From 75f896204a0df7637f802bdce9805010b08ae54a Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Fri, 16 Oct 2026 17:14:36 -0700 Subject: [PATCH 2/2] remove styleguide for now --- .gemini/styleguide.md | 176 ------------------------------------------ 1 file changed, 176 deletions(-) delete mode 100644 .gemini/styleguide.md diff --git a/.gemini/styleguide.md b/.gemini/styleguide.md deleted file mode 100644 index 76dbae42b3c..00000000000 --- a/.gemini/styleguide.md +++ /dev/null @@ -1,176 +0,0 @@ -# Binaryen Code Review Guide - -Binaryen is a compiler and toolchain infrastructure library for WebAssembly, -written in C++20. Focus code reviews on **correctness, WebAssembly and Binaryen -IR invariants, soundness of optimizations, test quality, and code clarity** -while avoiding low-signal stylistic nitpicks. - -## Review Priorities & Noise Reduction - -* **High signal-to-noise ratio:** Only comment when there is a concrete - correctness bug, missed edge case, violated project convention, or clear - simplification. -* **Do not comment on formatting, whitespace, or braces:** C++ formatting and - brace style are enforced by deterministic CI linters (`git clang-format`). -* **Do not flag auto-generated `CHECK` lines in `test/lit/`:** `CHECK:` and - `CHECK-NEXT:` lines in `.wast` and `.test` files are generated by - `scripts/update_lit_checks.py` and `scripts/update_help_checks.py`. Instead, - inspect the `CHECK` output to verify that the test actually exercises the - intended code path and produces sound output. -* **Do not ask for `CHANGELOG.md` entries on bug fixes or minor improvements:** - `CHANGELOG.md` only tracks breaking changes, new passes, and major - user-visible features. - -## Binaryen IR & Optimization Pass Invariants - -* **Keep `src/wasm.h` minimal:** Avoid adding pass-specific state, annotations, - or single-use helpers to `src/wasm.h`. Keep comments in `src/wasm.h` focused - on high-level IR semantics rather than pass-specific algorithmic details. Do - not add default member initializers to `Expression` subclasses in - `src/wasm.h`; IR nodes should be constructed via `Builder` - (`src/wasm-builder.h`). -* **Type updating & `ReFinalize`:** - * Whenever a transformation can change or refine an expression's type (e.g., - replacing a node with one of a more specific subtype, replacing an - expression with `(unreachable)`, or refining function parameter/result - types), `ReFinalize` must be run so enclosing expressions update their - types. - * `ReFinalize` is a function-parallel pass, so - `ReFinalize().run(getPassRunner(), wasm)` only walks function bodies. Passes - that modify or construct expressions in module-level locations (global - initializers, table initializers, element segments, data segment offsets) - must also call `ReFinalize().walkModuleCode(wasm)` if types can change. -* **Unreachable code & subtyping:** - * Inside function bodies, any expression operand may have `Type::unreachable` - (whereas module-level initializers cannot contain `unreachable`). Always - check for `Type::unreachable` (or check `type.isRef()`, `type.isTuple()`, - etc.) before calling accessors like `getHeapType()` that assert on - `unreachable`. - * A parent expression has `Type::unreachable` whenever *any* of its child - operands is `unreachable`. Do not assume `curr->type != Type::unreachable` - is required for another child operand to have a concrete or reference - type—check the child operand's type directly. - * In Binaryen IR, expressions of any type (such as `Type::none` calls) can - appear at `block->list.back()` after an earlier `unreachable` element, even - when the block or function has a concrete result type. Passes inspecting - tail position must verify `Type::isSubType(expr->type, expectedType)`. - * Prefer `Type::isSubType(sub, super)` and `HeapType::isSubType(sub, super)` - over `==` unless exact type equality is required. -* **Traps, effects, and `PassOptions`:** - * Never optimize a guaranteed trap into `(nop)` (even in unreachable or - closed-world code); replace guaranteed traps with `(unreachable)` so DCE and - subsequent passes can remove dead code after the trap. - * Check `PassOptions` flags appropriately: respect `options.trapsNeverHappen` - (`-tnh`) before assuming traps cannot occur at runtime, - `options.closedWorld` before making closed-world assumptions, and - `options.shrinkLevel == 0` before performing optimizations that increase - code size. - * Use `EffectAnalyzer` (`src/ir/effects.h`) to check side effects and - instruction reordering validity (`orderedAfter`, - `hasUnremovableSideEffects`, etc.). Note that `mayNotReturn` (e.g., infinite - loops) is distinct from `transfersControlFlow()` (branches, returns, - throws). -* **Legacy Exception Handling `pop`s:** - * Legacy EH `catch` bodies use `pop` expressions, which must remain direct - children of the `catch` body and cannot be nested inside an inner `block`. - Passes that introduce a `Block` around arbitrary expressions (e.g., via - `Builder::makeSequence` or `ChildLocalizer`) must call - `EHUtils::handleBlockNestedPops(func, *getModule())` when `pop`s may be - present. -* **Walkers, visitors, and pass structure:** - * Distinguish `Visitor` (visits a single expression) from `Walker` (traverses - a subtree, such as `PostWalker`, `ExpressionStackWalker`, - `ControlFlowWalker`, `CFGWalker`). Give walker/visitor classes descriptive - names (e.g., `ReferenceScanner`) rather than generic `Visitor`. - * In function-parallel passes (`isFunctionParallel() == true`), each function - gets a fresh pass instance, so per-function member state does not need - manual clearing in `doWalkFunction`. - * Do not manually invoke general cleanup passes like `dce` inside an - optimization pass unless required for correctness; rely on the pass pipeline - to run cleanup passes. - * Reuse existing helpers in `src/ir/` (`Properties`, `Bits`, `ChildLocalizer`, - `ModuleUtils`, `TableUtils`, `MemoryUtils`, `LUBFinder`, - `SubtypingDiscoverer`, `Match` in `src/ir/match.h`) instead of duplicating - logic. - -## Validation, Binary Format, and New Instructions - -* **Validator control flow (`src/wasm/wasm-validator.cpp`):** `shouldBeTrue` and - `shouldBeEqual` record an error and return `false`; they do **not** return - early from the visitor. If subsequent checks dereference pointers, call type - accessors like `getStruct()`, or index into vectors (e.g., `fields[index]`) - assuming the check passed, guard them with `if (!shouldBeTrue(...)) { return; }`. -* **Binary encoding (`src/wasm/wasm-stack.cpp`, `src/wasm/wasm-binary.cpp`):** - All prefixed opcodes (`AtomicPrefix`, `GCPrefix`, `SIMDPrefix`, `MiscPrefix`) - must encode their sub-opcode using `U32LEB(...)`, not `int8_t`. -* **New instruction checklist:** When adding a new instruction or type, verify - that all relevant components from `Contributing.md` are updated (IR, builder, - traversal, validator, interpreter, `effects.h`, parser, printer, binary - reader/writer, and fuzzer support in `src/tools/fuzzing/`). - -## C++ Coding Style & Idioms - -* **Naming:** Use `PascalCase` for types/classes/structs and `camelCase` for - functions, methods, parameters, and variables (avoid `snake_case` variable - names and trailing underscores on private fields). -* **Control flow:** Prefer early returns (`if (!cond) { return; }`) to reduce - indentation. Omit redundant early returns immediately before a loop that - naturally does nothing when the container is empty. -* **Assertions & errors:** Use `assert(...)` for internal invariants and - `Fatal() << ...` for unhandled cases or fatal errors. Because Binaryen enables - assertions in Release builds by default (`-UNDEBUG`), avoid placing expensive - linear scans inside hot-path `assert(...)` checks. -* **Types & C++20 idioms:** - * Use `Index` and `Address` instead of raw `int` or `size_t` for Wasm indices - and memory offsets. - * Use `expr->dynCast()` for conditional casts and `expr->cast()` (or - `expr->is()`) when the dynamic type is known. - * Prefer `.contains(...)` over `.count(...)` on associative containers, and - prefer pre-increment (`++i`) over post-increment (`i++`). - * Prefer explicit named setters (e.g., `setEmitModuleName(...)`) or enums over - opaque boolean constructor parameters. -* **Comments:** - * Place comments on their own lines above the code rather than as trailing - end-of-line comments. - * Explain non-obvious invariants, algorithms, and edge cases in comments, and - update or remove stale comments when nearby code changes. - -## Testing Guidelines (`test/lit/`, `test/gtest/`) - -* **Test coverage:** Bug fixes, pass improvements, and new features must include - tests. Pure NFC refactorings do not require new tests. -* **Lit test organization (`test/lit/**/*.wast`):** - * Add test cases to existing `.wast` files when appropriate; otherwise give new - files descriptive names (never name test files after GitHub issue numbers). - * Write `.wast` tests using folded S-expressions. - * Place comments explaining individual test functions **inside** the - `(func ...)` body (not immediately above `(func ...)`), so - `scripts/update_lit_checks.py` does not insert `CHECK` lines between the - comment and the function. - * Add module-level `;;` comments above `(module ...)` blocks in multi-module - tests to explain what each module tests. - * Do **not** use `foreach` in the `RUN:` line of a `.wast` file that only - contains a single `(module ...)`. - * Keep tests minimal and targeted at the specific pass or feature under test; - verify that the `CHECK` output demonstrates the expected transformation and - that no stale `check-prefix` expectations remain. - -## PR Descriptions & Commit Messages - -* **Concise, direct prose:** Commit messages and PR descriptions should use - concise, natural prose paragraphs that explain *why* the change is being made - (the bug, motivation, or invariant) and the high-level approach. Trivial - fixes only need a sentence or two and an issue reference (e.g., `Fixes #1234`). -* **Describe only the final state:** Commit messages and PR descriptions should - describe the final state of the change, not the chronological history of - intermediate attempts or revisions made during review. -* **No AI slop:** Flag verbose, formulaic LLM-generated PR descriptions and - commit messages, including: - * Boilerplate section headers (`## Summary`, `## Key Changes`, `## Test Plan`, - `## Impact`) and robotic bulleted file-by-file summaries that merely restate - the diff. - * Filler or promotional language ("comprehensive", "robust", "seamless", - "enhances maintainability"). - * AI attribution or co-author trailers (e.g., `Co-Authored-By: Claude ...`, - `Generated with ...`). Per `Contributing.md`, the human contributor is - always the sole author and is fully accountable for the submission.