Skip to content

Commit 5ca2ca3

Browse files
cameelcameel
authored
Merge pull request #13553 from ethereum/review-checklist
Review checklist
2 parents 2ba8fc1 + b3b9b88 commit 5ca2ca3

File tree

2 files changed

+174
-0
lines changed

2 files changed

+174
-0
lines changed

‎ReviewChecklist.md

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
# PR Review Checklist
2+
The Solidity compiler is a critical piece of infrastructure in the Ethereum ecosystem.
3+
For this reason, our review process is quite strict and all PRs have to fulfill certain quality
4+
expectations and guidelines.
5+
The list below is meant to reduce the workload on the core team by helping contributors self-identify
6+
and solve common issues before they are pointed out in the review.
7+
It is also meant to serve as a final checklist for reviewers to go through before approving a PR.
8+
9+
## Before You Submit a PR
10+
- [ ] **Is the issue ready to be worked on?**
11+
- If the issue does not have a desirability label (`nice to have`, `should have`,
12+
`must have eventually`, `must have`, `roadmap`) we have not yet decided whether to implement it.
13+
- If the issue has the `needs design` label, we have not yet decided how it should be implemented.
14+
- `good first issue candidate` means that the issue will potentially be a `good first issue`
15+
eventually but at the moment it is not yet ready to be worked on.
16+
- [ ] **Is this a breaking change?** Breaking changes should be based on the `breaking` branch rather than on the `develop` branch.
17+
- [ ] **Does the PR actually address the issue?**
18+
- [ ] Mention the issue number in the PR description.
19+
If the PR solves it completely, use the `Fixes #<issue number>` form so that Github will close the issue automatically.
20+
- [ ] Do not include the issue number in the PR title, branch name or commit description.
21+
- [ ] When submitting a PR from a fork **create a branch and give it a descriptive name.**
22+
E.g. `fix-array-abi-encoding-bug`.
23+
Do not submit PRs directly from the `develop` branch of your fork since it makes rebasing and fetching new changes harder.
24+
- [ ] **Does the PR depend on other PRs?**
25+
- [ ] If the PR has dependencies, mention them in bold in the description.
26+
- [ ] Avoid basing PRs from forks on branches other than `develop` or `breaking` because
27+
GitHub closes them when the base branch gets merged.
28+
Do this only for PRs created directly in the main repo.
29+
30+
## Coding Style and Good Practices
31+
- [ ] Does the PR follow our [coding style](CODING_STYLE.md)?
32+
33+
### Reliability
34+
- [ ] **Use assertions liberally.** If you are certain your assumption will not be broken, prove it with `solAssert()`.
35+
- [ ] **Validate inputs and handle errors**. Note that assertions are **not** validation.
36+
37+
### Readability
38+
- [ ] **Choose good names.**
39+
- [ ] Is the name straightforward to understand?
40+
Do you feel the need to jump back to the definition and remind yourself what it was whenever you see it?
41+
- [ ] Is the name unambiguous in the context where it is used?
42+
- [ ] Avoid abbreviations.
43+
- [ ] **Source files, classes and public functions should have docstrings.**
44+
- [ ] **Avoid code duplication.** But not fanatically. Minimal amounts of duplication are acceptable if it aids readability.
45+
- [ ] **Do not leave dead or commented-out code behind.** You can still see old code in history.
46+
If you really have a good reason to do it, always leave a comment explaining what it is and why it is there.
47+
- [ ] **Mark hacks as such.** If you have to leave behind a temporary workaround, make
48+
sure to include a comment that explains why and in what circumstances it can be removed.
49+
Preferably link to an issue you reported upstream.
50+
- [ ] **Avoid obvious comments.**
51+
- [ ] **Do include comments when the reader may need extra context to understand the code.**
52+
53+
### Commits and PRs
54+
- [ ] **Avoid hiding functional changes inside refactors.**
55+
E.g. when fixing a small bug, or changing user-visible behavior, put the change in a separate commit.
56+
Do not mix it with another change that renames things or reformats the code around, making the fix itself hard to identify.
57+
- [ ] **Whenever possible, split off refactors or unrelated changes into separate PRs.**
58+
Smaller PRs are easier and quicker to review.
59+
Splitting off refactors helps focus on the main point of the PR.
60+
61+
### Common Pitfalls
62+
The following points are all covered by the coding style but come up so often that it is worth singling them out here:
63+
- [ ] **Always initialize value types in the definition,** even if you are sure you will assign them later.
64+
- [ ] **Use "east const" style.** I.e. `T const*`, not `const T *`.
65+
- [ ] **Keep indentation consistent.** See our [`.editorconfig`](.editorconfig).
66+
- [ ] Tabs for C++. But use them for indentation only. Any whitespace later on the line must consist of spaces.
67+
- [ ] 4 spaces for most other file types.
68+
- [ ] **Use `auto` sparingly.** Only use it when the actual type is very long and complicated or when it is
69+
already used elsewhere in the same expression.
70+
- [ ] **Indent braces and parentheses in a way that makes nesting clear.**
71+
- [ ] **Use `using namespace` only in `.cpp` files.** Use it for `std` and our own modules.
72+
Avoid unnecessary `std::` prefix in `.cpp` files (except for `std::move`).
73+
- [ ] **Use range-based loops and destructuring.**
74+
- [ ] **Include any headers you use directly,** even if they are implicitly included through other headers.
75+
76+
## Documentation
77+
- [ ] **Does the PR update relevant documentation?**
78+
79+
### Documentation Style and Good Practices
80+
- [ ] **Use double backticks in RST (``` ``x`` ```). Prefer single backticks in Markdown (`` `x` ``),**
81+
but note that double backticks are valid too and we use them in some cases for legacy reasons.
82+
- [ ] **Always start a new sentence on a new line.**
83+
This way you do not have to rewrap the surrounding text when you rewrite the sentence.
84+
This also makes changes actually easier to spot in the diff.
85+
86+
## Testing
87+
88+
### What to Test
89+
- [ ] **Is newly added code adequately covered by tests?** Have you considered all the important corner cases?
90+
- If it is a bugfix:
91+
- [ ] **The PR must include tests that reproduce the bug.**
92+
- [ ] **Are there gaps in test coverage of the buggy feature?** Fill them by adding more tests.
93+
- [ ] **Try to break it.** Can you of any similar features that could also be buggy?
94+
Play with the repro and include prominent variants as separate test cases, even if they don't trigger a bug.
95+
- [ ] **Positive cases (code that compiles) should have a semantic test.**
96+
- [ ] **Negative cases (code with compilation errors) should have a syntax test.**
97+
- [ ] **Avoid mixing positive and negative cases in the same syntax test.**
98+
If the test produces an error, we stop at the analysis stage and we will not detect
99+
problems that only occur in code generation, optimizer or assembler.
100+
- [ ] If you have to do it, at least mark positive cases inside the file with a short comment.
101+
- This way, when the test is updated, it is easier to verify that these cases still do not trigger an error.
102+
- [ ] New syntax: **does it have an [`ASTJSON`](test/libsolidity/ASTJSON/) test?**
103+
- [ ] New CLI or StandardJSON option:
104+
- [ ] **Does it have a [command-line test](test/cmdlineTests/)?**
105+
- [ ] **Is the option listed for every input mode in [`CommandLineParser` tests](test/solc/CommandLineParser.cpp)?**
106+
- [ ] **Did you consider interactions with other language features?**
107+
- [ ] Are all types covered? Structs? Enums? Contracts/libraries/interfaces? User-defined value types?
108+
Value types: integers, fixed bytes, `address`, `address payable`, `bool`? Function pointers?
109+
Static and dynamic arrays? `string` and `bytes`? Mappings?
110+
Values of types that cannot be named: literals, tuples, array slices, storage references?
111+
- [ ] If it accepts a function, does it also accept an event or an error? These have function types but are not functions.
112+
- [ ] If it affects free functions, what about internal library functions?
113+
- [ ] Bound library functions? Functions bound with `using for`?
114+
- [ ] Possible combinations of `storage`, `memory`, `calldata`, `immutable`, `constant`?
115+
Remember that internal functions can take `storage` arguments.
116+
- [ ] Does it work at construction time as well? What if you store it at construction time and read after deployment?
117+
- [ ] What about importing it from a different module or inheriting it?
118+
- [ ] Have you tested it with the ternary operator?
119+
120+
### Test Style and Good Practices
121+
- [ ] **Make test case file names long and specific enough** so that it is easy to guess what is inside.
122+
When checking if we have the case already covered the name is usually the only clue we see.
123+
- [ ] Place them in the right subdirectory.
124+
- [ ] **Avoid simply appending numbers to the name to distinguish similar cases.**
125+
Coming up with good names is hard but figuring out if any of hundreds of tests with names that
126+
match your search actually fits your case is even harder.
127+
- [ ] **Do not include version pragma and the SPDX comment in semantic and syntax test cases**.
128+
In other test types include them if necessary to suppress warnings.
129+
- [ ] **If you have to use a version pragma, avoid hard-coding version.** Use `pragma solidity *`.
130+
- [ ] **Add `--pretty-print --pretty-json 4` to the `args` file of in command-line tests** to get
131+
readable, indented output.
132+
- [ ] **When writing StandardJSON command-line tests, use `urls` instead of `content`** and put
133+
the Solidity or Yul code in a separate file.
134+
135+
## Compiler-specific
136+
- [ ] **Are error messages sensible and understandable to users?**
137+
- [ ] **Are error codes consistent?**
138+
- [ ] Avoid randomly changing or reassigning error codes.
139+
- [ ] Avoid defining separate codes for trivial variants of the same issue.
140+
Make it easy for tools to consistently refer to the same error with the same code.
141+
- [ ] **Error messages should end with a full stop.**
142+
- [ ] **Prefer Ranges v3 to Boost where possible.**
143+
144+
## Take a Step Back
145+
- [ ] **Do you fully understand what the PR does and why?**
146+
- [ ] **Are you confident that the code works and does not break unrelated functionality?**
147+
- [ ] **Is this a reasonable way to achieve the goal stated in the issue?**
148+
- [ ] **Is the code simple?** Does the PR achieve its objective at the cost of significant
149+
complexity that may be a source of future bugs?
150+
- [ ] **Is the code efficient?** Does the PR introduce any major performance bottlenecks?
151+
- [ ] **Does the PR introduce any breaking changes beyond what was agreed in the issue?**
152+
153+
## Final Checks Before Merging
154+
- [ ] **Is the PR rebased on top of the `develop` branch** (or `breaking` if it is a breaking change)?
155+
- [ ] **Did all CI checks pass?**
156+
- Note that we have a few jobs that tend to randomly fail due to outside factors, especially external tests (with `_ext_` in the name).
157+
If these fail, rebase on latest `develop` (or `breaking`) and try rerunning them.
158+
Note also that not all of these checks are required for the PR to be merged.
159+
- [ ] If the change is visible to users, **does the PR have a [changelog](Changelog.md) entry?**
160+
- [ ] Is the changelog entry in the right section?
161+
Make sure to move it up if there was a release recently.
162+
- [ ] **Is commit history simple and understandable?**
163+
- [ ] Each commit should be a self-contained, logical step leading the goal of the PR, without going back and forth.
164+
In particular, review fixups should be squashed into the commits they fix.
165+
- [ ] Do not include any merge commits in your branch. Please use rebase to keep up to date with the base branch.
166+
- [ ] **Is the PR properly labeled?**
167+
- Use `external contribution` label to mark PRs not coming from the core team.
168+
- If the PR depends on other PRs, use `has dependencies` and set the base branch accordingly.
169+
- Labels like `documentation` or `optimizer` are helpful for filtering PRs.

‎docs/contributing.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@ Finally, please make sure you respect the `coding style
7777
for this project. Also, even though we do CI testing, please test your code and
7878
ensure that it builds locally before submitting a pull request.
7979

80+
We highly recommend going through our `review checklist <https://github.com/ethereum/solidity/blob/develop/ReviewChecklist.md>`
81+
before submitting the pull request.
82+
We thoroughly review every PR and will help you get it right, but there are many
83+
common problems that can be easily avoided, making the review much smoother.
84+
8085
Thank you for your help!
8186

8287
Running the Compiler Tests

0 commit comments

Comments
 (0)