solidity/ReviewChecklist.md

# PR Review Checklist
The Solidity compiler is a critical piece of infrastructure in the Ethereum ecosystem.
For this reason, our review process is quite strict and all PRs have to fulfill certain quality
expectations and guidelines.
The list below is meant to reduce the workload on the core team by helping contributors self-identify
and solve common issues before they are pointed out in the review.
It is also meant to serve as a final checklist for reviewers to go through before approving a PR.

## Before You Submit a PR
- [ ] **Is the issue ready to be worked on?**
    - If the issue does not have a desirability label (`nice to have`, `should have`,
        `must have eventually`, `must have`, `roadmap`) we have not yet decided whether to implement it.
    - If the issue has the `needs design` label, we have not yet decided how it should be implemented.
    - `good first issue candidate` means that the issue will potentially be a `good first issue`
        eventually but at the moment it is not yet ready to be worked on.
- [ ] **Is this a breaking change?** Breaking changes should be based on the `breaking` branch rather than on the `develop` branch.
- [ ] **Does the PR actually address the issue?**
    - [ ] Mention the issue number in the PR description.
        If the PR solves it completely, use the `Fixes #<issue number>` form so that Github will close the issue automatically.
    - [ ] Do not include the issue number in the PR title, branch name or commit description.
- [ ] When submitting a PR from a fork **create a branch and give it a descriptive name.**
    E.g. `fix-array-abi-encoding-bug`.
    Do not submit PRs directly from the `develop` branch of your fork since it makes rebasing and fetching new changes harder.
- [ ] **Does the PR depend on other PRs?**
    - [ ] If the PR has dependencies, mention them in bold in the description.
    - [ ] Avoid basing PRs from forks on branches other than `develop` or `breaking` because
        GitHub closes them when the base branch gets merged.
        Do this only for PRs created directly in the main repo.

## Coding Style and Good Practices
- [ ] Does the PR follow our [coding style](CODING_STYLE.md)?

### Reliability
- [ ] **Use assertions liberally.** If you are certain your assumption will not be broken, prove it with `solAssert()`.
- [ ] **Validate inputs and handle errors**. Note that assertions are **not** validation.

### Readability
- [ ] **Choose good names.**
    - [ ] Is the name straightforward to understand?
        Do you feel the need to jump back to the definition and remind yourself what it was whenever you see it?
    - [ ] Is the name unambiguous in the context where it is used?
    - [ ] Avoid abbreviations.
- [ ] **Source files, classes and public functions should have docstrings.**
- [ ] **Avoid code duplication.** But not fanatically. Minimal amounts of duplication are acceptable if it aids readability.
- [ ] **Do not leave dead or commented-out code behind.** You can still see old code in history.
      If you really have a good reason to do it, always leave a comment explaining what it is and why it is there.
- [ ] **Mark hacks as such.** If you have to leave behind a temporary workaround, make
    sure to include a comment that explains why and in what circumstances it can be removed.
    Preferably link to an issue you reported upstream.
- [ ] **Avoid obvious comments.**
- [ ] **Do include comments when the reader may need extra context to understand the code.**

### Commits and PRs
- [ ] **Avoid hiding functional changes inside refactors.**
    E.g. when fixing a small bug, or changing user-visible behavior, put the change in a separate commit.
    Do not mix it with another change that renames things or reformats the code around, making the fix itself hard to identify.
- [ ] **Whenever possible, split off refactors or unrelated changes into separate PRs.**
    Smaller PRs are easier and quicker to review.
    Splitting off refactors helps focus on the main point of the PR.

### Common Pitfalls
The following points are all covered by the coding style but come up so often that it is worth singling them out here:
- [ ] **Always initialize value types in the definition,** even if you are sure you will assign them later.
- [ ] **Use "east const" style.** I.e. `T const*`, not `const T *`.
- [ ] **Keep indentation consistent.** See our [`.editorconfig`](.editorconfig).
    - [ ] Tabs for C++. But use them for indentation only. Any whitespace later on the line must consist of spaces.
    - [ ] 4 spaces for most other file types.
- [ ] **Use `auto` sparingly.** Only use it when the actual type is very long and complicated or when it is
    already used elsewhere in the same expression.
- [ ] **Indent braces and parentheses in a way that makes nesting clear.**
- [ ] **Use `using namespace` only in `.cpp` files.** Use it for `std` and our own modules.
    Avoid unnecessary `std::` prefix in `.cpp` files (except for `std::move`).
- [ ] **Use range-based loops and destructuring.**
- [ ] **Include any headers you use directly,** even if they are implicitly included through other headers.

## Documentation
- [ ] **Does the PR update relevant documentation?**

### Documentation Style and Good Practices
- [ ] **Use double backticks in RST (``` ``x`` ```). Prefer single backticks in Markdown (`` `x` ``),**
    but note that double backticks are valid too and we use them in some cases for legacy reasons.
- [ ] **Always start a new sentence on a new line.**
    This way you do not have to rewrap the surrounding text when you rewrite the sentence.
    This also makes changes actually easier to spot in the diff.

## Testing

### What to Test
- [ ] **Is newly added code adequately covered by tests?** Have you considered all the important corner cases?
- If it is a bugfix:
    - [ ] **The PR must include tests that reproduce the bug.**
    - [ ] **Are there gaps in test coverage of the buggy feature?** Fill them by adding more tests.
    - [ ] **Try to break it.** Can you of any similar features that could also be buggy?
        Play with the repro and include prominent variants as separate test cases, even if they don't trigger a bug.
- [ ] **Positive cases (code that compiles) should have a semantic test.**
- [ ] **Negative cases (code with compilation errors) should have a syntax test.**
- [ ] **Avoid mixing positive and negative cases in the same syntax test.**
    If the test produces an error, we stop at the analysis stage and we will not detect
    problems that only occur in code generation, optimizer or assembler.
    - [ ] If you have to do it, at least mark positive cases inside the file with a short comment.
        - This way, when the test is updated, it is easier to verify that these cases still do not trigger an error.
- [ ] New syntax: **does it have an [`ASTJSON`](test/libsolidity/ASTJSON/) test?**
- [ ] New CLI or StandardJSON option:
    - [ ] **Does it have a [command-line test](test/cmdlineTests/)?**
    - [ ] **Is the option listed for every input mode in [`CommandLineParser` tests](test/solc/CommandLineParser.cpp)?**
- [ ] **Did you consider interactions with other language features?**
    - [ ] Are all types covered? Structs? Enums? Contracts/libraries/interfaces? User-defined value types?
        Value types: integers, fixed bytes, `address`, `address payable`, `bool`? Function pointers?
        Static and dynamic arrays? `string` and `bytes`? Mappings?
        Values of types that cannot be named: literals, tuples, array slices, storage references?
    - [ ] If it accepts a function, does it also accept an event or an error? These have function types but are not functions.
    - [ ] If it affects free functions, what about internal library functions?
    - [ ] Bound library functions? Functions bound with `using for`?
    - [ ] Possible combinations of `storage`, `memory`, `calldata`, `immutable`, `constant`?
        Remember that internal functions can take `storage` arguments.
    - [ ] Does it work at construction time as well? What if you store it at construction time and read after deployment?
    - [ ] What about importing it from a different module or inheriting it?
    - [ ] Have you tested it with the ternary operator?

### Test Style and Good Practices
- [ ] **Make test case file names long and specific enough** so that it is easy to guess what is inside.
    When checking if we have the case already covered the name is usually the only clue we see.
    - [ ] Place them in the right subdirectory.
    - [ ] **Avoid simply appending numbers to the name to distinguish similar cases.**
        Coming up with good names is hard but figuring out if any of hundreds of tests with names that
        match your search actually fits your case is even harder.
- [ ] **Do not include version pragma and the SPDX comment in semantic and syntax test cases**.
    In other test types include them if necessary to suppress warnings.
- [ ] **If you have to use a version pragma, avoid hard-coding version.** Use `pragma solidity *`.
- [ ] **Add `--pretty-print --pretty-json 4` to the `args` file of in command-line tests** to get
    readable, indented output.
- [ ] **When writing StandardJSON command-line tests, use `urls` instead of `content`** and put
    the Solidity or Yul code in a separate file.

## Compiler-specific
- [ ] **Are error messages sensible and understandable to users?**
- [ ] **Are error codes consistent?**
    - [ ] Avoid randomly changing or reassigning error codes.
    - [ ] Avoid defining separate codes for trivial variants of the same issue.
        Make it easy for tools to consistently refer to the same error with the same code.
- [ ] **Error messages should end with a full stop.**
- [ ] **Prefer Ranges v3 to Boost where possible.**

## Take a Step Back
- [ ] **Do you fully understand what the PR does and why?**
- [ ] **Are you confident that the code works and does not break unrelated functionality?**
- [ ] **Is this a reasonable way to achieve the goal stated in the issue?**
- [ ] **Is the code simple?** Does the PR achieve its objective at the cost of significant
    complexity that may be a source of future bugs?
- [ ] **Is the code efficient?** Does the PR introduce any major performance bottlenecks?
- [ ] **Does the PR introduce any breaking changes beyond what was agreed in the issue?**

## Final Checks Before Merging
- [ ] **Is the PR rebased on top of the `develop` branch** (or `breaking` if it is a breaking change)?
- [ ] **Did all CI checks pass?**
    - Note that we have a few jobs that tend to randomly fail due to outside factors, especially external tests (with `_ext_` in the name).
        If these fail, rebase on latest `develop` (or `breaking`) and try rerunning them.
        Note also that not all of these checks are required for the PR to be merged.
- [ ] If the change is visible to users, **does the PR have a [changelog](Changelog.md) entry?**
    - [ ] Is the changelog entry in the right section?
        Make sure to move it up if there was a release recently.
- [ ] **Is commit history simple and understandable?**
    - [ ] Each commit should be a self-contained, logical step leading the goal of the PR, without going back and forth.
        In particular, review fixups should be squashed into the commits they fix.
    - [ ] Do not include any merge commits in your branch. Please use rebase to keep up to date with the base branch.
- [ ] **Is the PR properly labeled?**
    - Use `external contribution` label to mark PRs not coming from the core team.
    - If the PR depends on other PRs, use `has dependencies` and set the base branch accordingly.
    - Labels like `documentation` or `optimizer` are helpful for filtering PRs.