|
| 1 | +--- |
| 2 | +description: 'Code review guidelines for GitHub copilot in this project' |
| 3 | +applyTo: '**' |
| 4 | +excludeAgent: ["coding-agent"] |
| 5 | +--- |
| 6 | + |
| 7 | +# Code Review Instructions |
| 8 | + |
| 9 | +A change note is required for any pull request which modifies: |
| 10 | +- The structure or layout of the release artifacts. |
| 11 | +- The evaluation performance (memory, execution time) of an existing query. |
| 12 | +- The results of an existing query in any circumstance. |
| 13 | + |
| 14 | +If the pull request only adds new rule queries, a change note is not required. |
| 15 | +Confirm that either a change note is not required or the change note is required and has been added. |
| 16 | + |
| 17 | +For PRs that add new queries or modify existing queries, also consider the following review checklist: |
| 18 | +- Confirm that the output format of shared queries is valid. |
| 19 | +- Have all the relevant rule package description files been checked in? |
| 20 | +- Have you verified that the metadata properties of each new query is set appropriately? |
| 21 | +- Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases? |
| 22 | +- Are all the alerts in the expected file annotated as NON_COMPLIANT in the test source file? |
| 23 | +- Are the alert messages properly formatted and consistent with the style guide? |
| 24 | +- Does the query have an appropriate level of in-query comments/documentation? |
| 25 | +- Does the query not reinvent features in the standard library? |
| 26 | +- Can the query be simplified further (not golfed!). |
| 27 | + |
| 28 | +In your review output, list only those checklist items that are not satisfied or are uncertain, but also report any other problems you find outside this checklist; do not mention checklist items that clearly pass. |
| 29 | + |
| 30 | +## Validating tests and .expected files |
| 31 | + |
| 32 | +The test infrastructure for CodeQL that we use in this project involves the creation of a test directory with the following structure: |
| 33 | +- Test root is `some/path/test/path/to/feature` (mirrors `some/path/src/path/to/query`) |
| 34 | +- At least one test `c` or `c++` file, typically named `test.c`/`test.cpp`, with lines annotated `// COMPLIANT` or `// NON_COMPLIANT` |
| 35 | +- A `.ql` file with test query logic, or a `.qlref` file referring to the production query logic |
| 36 | +- A matching `FOO.expected` file to go with each `FOO.ql` or `FOO.qlref`, containing the test query results for the test `c` or `c++` files |
| 37 | +- Note that some test directories simply have a `testref` file, to document that a certain query is tested in a different directory. |
| 38 | + |
| 39 | +As a code reviewer, it is critical to ensure that the results in the `.expected` file match the comments in the test file. |
| 40 | + |
| 41 | +The `.expected` file uses a columnar format: |
| 42 | +- For example, a basic row may look like `| test.cpp:8:22:8:37 | element | message |`. |
| 43 | +- For a query with `select x, "test"`, the columns are | x.getLocation() | x.toString() | "test" |` |
| 44 | +- An alert with placeholders will use `$@` in the message, and have additional `element`/`string` columns for placeholder, e.g. `| test.cpp:8:22:8:37 | ... + ... | Invalid add of $@. | test.cpp:7:5:7:12 | my_var | deprecated variable my_var |`. |
| 45 | +- Remember, there is one `.expected` file for each `.ql` or `.qlref` file. |
| 46 | +- Each `.expected` file will contain the results for all test c/cpp files. |
| 47 | +- The `toString()` format of QL objects is deliberately terse for performance reasons. |
| 48 | +- For certain queries such as "path problems", the results may be grouped into categories via text lines with the category name, e.g. `nodes` and `edges` and `problems`. |
| 49 | + |
| 50 | +Reviewing tests in this style can be tedious and error prone, but fundamental to the effectiveness of our TDD requirements in this project. |
| 51 | + |
| 52 | +When reviewing tests, it is critical to: |
| 53 | +- Check that each `NON_COMPLIANT` case in the test file has a row in the correct `.expected` file referring to the correct location. |
| 54 | +- Check that each row in each `.expected` file has a `NON_COMPLIANT` case in the test file at the correct location. |
| 55 | +- Check that there are no `.expected` rows that refer to test code cases marked as `COMPLIANT`, or with no comment |
| 56 | +- Note that it is OK if the locations of the comment are not precisely aligned with the alert |
| 57 | +- Check that the alert message and placeholders are accurate and understandable. |
| 58 | +- Check that the locations do not refer to files in the standard library, as these have issues in GitHub's Code Scanning UI and complicate our compiler compatibility tests. |
| 59 | +- Consider the "test coverage" of the query, are each of its logical statements effectively exercised individually, collectively? The test should neither be overly bloated nor under specified. |
| 60 | +- Consider the edge cases of the language itself, will the analysis work in non-trivial cases, are all relevant language concepts tested here? This doesn't need to be exhaustive, but it should be thoughfully thorough. |
| 61 | + |
| 62 | +## Validating Query Style |
| 63 | + |
| 64 | +The following list describes the required style guides for a query that **must** be validated during the code-review process. |
| 65 | + |
| 66 | +A query **must** include: |
| 67 | + |
| 68 | +- A use of the `isExcluded` predicate on the element reported as the primary location. This predicate ensures that we have a central mechanism for excluding results. This predicate may also be used on other elements relevant to the alert, but only if a suppression on that element should also cause alerts on the current element to be suppressed. |
| 69 | +- A well formatted alert message: |
| 70 | + - The message should be a complete standalone sentence, with punctuation and a period. |
| 71 | + - The message should refer to this particular instance of the problem, rather than repeating the generic rule. e.g. "Call to banned function x." instead of "Do not use function x." |
| 72 | + - Code elements should be placed in 'single quotes', unless they are formatted as links. |
| 73 | + - Avoid value judgments such as "dubious" and "suspicious", and focus on factual statements about the problem. |
| 74 | + - If possible, avoid constant alert messages. Either add placeholders and links (using $@), or concatenate element names to the alert message. Non-constant messages make it easier to find particular results, and links to other program elements can help provide additional context to help a developer understand the results. Examples: |
| 75 | + - Instead of `Call to banned function.` prefer `Call to banned function foobar.`. |
| 76 | + - Instead of `Return value from call is unused.` prefer `Return value from call to function [x] is unused.`, where `[x]` is a link to the function itself. |
| 77 | + - Do not try to explain the solution in the message; instead that should be provided in the help for the query. |
| 78 | + |
| 79 | +All lines in CodeQL source files and test files should be kept to a maximum of 100 characters. |
| 80 | + |
| 81 | +All public predicates, classes, modules and files should be documented with QLDoc. All QLDoc should follow the following QLDoc style guide: |
| 82 | + |
| 83 | +### General QLDoc requirements |
| 84 | + |
| 85 | +1. Documentation must adhere to the [QLDoc specification](https://codeql.github.com/docs/ql-language-reference/ql-language-specification/#qldoc). |
| 86 | +1. Documentation comments should be appropriate for users of the code. |
| 87 | +1. Documentation for maintainers of the code must use normal comments. |
| 88 | +1. Use `/** ... */` for documentation, even for single line comments. |
| 89 | + - For single-line documentation, the `/**` and `*/` are written on the same line as the comment. |
| 90 | + - For multi-line documentation, the `/**` and `*/` are written on separate lines. There is a `*` preceding each comment line, aligned on the first `*`. |
| 91 | +1. Use code formatting (backticks) within comments for code from the source language, and also for QL code (for example, names of classes, predicates, and variables). |
| 92 | +1. Give explanatory examples of code in the target language, enclosed in ```` ```<target language> ```` or `` ` ``. |
| 93 | + |
| 94 | + |
| 95 | +### Language requirements |
| 96 | + |
| 97 | +1. Use American English. |
| 98 | +1. Use full sentences, with capital letters and periods, except for the initial sentence of the comment, which may be fragmentary as described below. |
| 99 | +1. Use simple sentence structures and avoid complex or academic language. |
| 100 | +1. Avoid colloquialisms and contractions. |
| 101 | +1. Use words that are in common usage. |
| 102 | + |
| 103 | + |
| 104 | +### Requirements for specific items |
| 105 | + |
| 106 | +1. Public declarations must be documented. |
| 107 | +1. Non-public declarations should be documented. |
| 108 | +1. Declarations in query files should be documented. |
| 109 | +1. Library files (`.qll` files) should have a documentation comment at the top of the file. |
| 110 | +1. Query files, except for tests, must have a QLDoc query documentation comment at the top of the file. |
| 111 | + |
| 112 | +### QLDoc for predicates |
| 113 | + |
| 114 | +1. Refer to all predicate parameters in the predicate documentation. |
| 115 | +1. Reference names, such as types and parameters, using backticks `` ` ``. |
| 116 | +1. Give examples of code in the target language, enclosed in ```` ```<target language> ```` or `` ` ``. |
| 117 | +1. Predicates that override a single predicate don't need QLDoc, as they will inherit it. |
| 118 | + |
| 119 | +#### Predicates without result |
| 120 | + |
| 121 | +1. Use a third-person verb phrase of the form ``Holds if `arg` has <property>.`` |
| 122 | +1. Avoid: |
| 123 | + - `/** Whether ... */` |
| 124 | + - `/** Relates ... */` |
| 125 | + - Question forms: |
| 126 | + - ``/** Is `x` a foo? */`` |
| 127 | + - ``/** Does `x` have a bar? */`` |
| 128 | + |
| 129 | +##### Example |
| 130 | + |
| 131 | +```ql |
| 132 | +/** |
| 133 | + * Holds if the qualifier of this call has type `qualifierType`. |
| 134 | + * `isExactType` indicates whether the type is exact, that is, whether |
| 135 | + * the qualifier is guaranteed not to be a subtype of `qualifierType`. |
| 136 | + */ |
| 137 | +``` |
| 138 | + |
| 139 | +#### Predicates with result |
| 140 | + |
| 141 | +1. Use a third-person verb phrase of the form `Gets (a|the) <thing>.` |
| 142 | +1. Use "if any" if the item is usually unique but might be missing. For example |
| 143 | + `Gets the body of this method, if any.` |
| 144 | +1. If the predicate has more complex behaviour, for example multiple arguments are conceptually "outputs", it can be described like a predicate without a result. For example |
| 145 | + ``Holds if `result` is a child of this expression.`` |
| 146 | +1. Avoid: |
| 147 | + - `Get a ...` |
| 148 | + - `The ...` |
| 149 | + - `Results in ...` |
| 150 | + - Any use of `return` |
| 151 | + |
| 152 | +##### Example |
| 153 | +```ql |
| 154 | +/** |
| 155 | + * Gets the expression denoting the super class of this class, |
| 156 | + * or nothing if this is an interface or a class without an `extends` clause. |
| 157 | + */ |
| 158 | +``` |
| 159 | + |
| 160 | +#### Deprecated predicates |
| 161 | + |
| 162 | +The documentation for deprecated predicates should be updated to emphasize the deprecation and specify what predicate to use as an alternative. |
| 163 | +Insert a sentence of the form `DEPRECATED: Use <other predicate> instead.` at the start of the QLDoc comment. |
| 164 | + |
| 165 | +##### Example |
| 166 | + |
| 167 | +```ql |
| 168 | +/** DEPRECATED: Use `getAnExpr()` instead. */ |
| 169 | +deprecated Expr getInitializer() |
| 170 | +``` |
| 171 | + |
| 172 | +#### Internal predicates |
| 173 | + |
| 174 | +Some predicates are internal-only declarations that cannot be made private. The documentation for internal predicates should begin with `INTERNAL: Do not use.` |
| 175 | + |
| 176 | +##### Example |
| 177 | + |
| 178 | +```ql |
| 179 | +/** |
| 180 | + * INTERNAL: Do not use. |
| 181 | + */ |
| 182 | +``` |
| 183 | + |
| 184 | +#### Special predicates |
| 185 | + |
| 186 | +Certain special predicates should be documented consistently. |
| 187 | + |
| 188 | +- Always document `toString` as |
| 189 | + |
| 190 | + ```ql |
| 191 | + /** Gets a textual representation of this element. */ |
| 192 | + string toString() { ... } |
| 193 | + ``` |
| 194 | + |
| 195 | +- Always document `hasLocationInfo` as |
| 196 | + |
| 197 | + ```ql |
| 198 | + /** |
| 199 | + * Holds if this element is at the specified location. |
| 200 | + * The location spans column `startcolumn` of line `startline` to |
| 201 | + * column `endcolumn` of line `endline` in file `filepath`. |
| 202 | + * For more information, see |
| 203 | + * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/). |
| 204 | + */ |
| 205 | +
|
| 206 | + predicate hasLocationInfo(string filepath, int startline, int startcolumn, int endline, int endcolumn) { ... } |
| 207 | + ``` |
| 208 | + |
| 209 | +### QLDoc for classes |
| 210 | + |
| 211 | +1. Document classes using a noun phrase of the form `A <domain element> that <has property>.` |
| 212 | +1. Use "that", not "which". |
| 213 | +1. Refer to member elements in the singular. |
| 214 | +1. Where a class denotes a generic concept with subclasses, list those subclasses. |
| 215 | + |
| 216 | +##### Example |
| 217 | + |
| 218 | +```ql |
| 219 | +/** |
| 220 | + * A delegate declaration, for example |
| 221 | + * ``` |
| 222 | + * delegate void Logger(string text); |
| 223 | + * ``` |
| 224 | + */ |
| 225 | +class Delegate extends ... |
| 226 | +``` |
| 227 | + |
| 228 | +```ql |
| 229 | +/** |
| 230 | + * An element that can be called. |
| 231 | + * |
| 232 | + * Either a method (`Method`), a constructor (`Constructor`), a destructor |
| 233 | + * (`Destructor`), an operator (`Operator`), an accessor (`Accessor`), |
| 234 | + * an anonymous function (`AnonymousFunctionExpr`), or a local function |
| 235 | + * (`LocalFunction`). |
| 236 | + */ |
| 237 | +class Callable extends ... |
| 238 | +``` |
| 239 | + |
| 240 | +### QLDoc for modules |
| 241 | + |
| 242 | +Modules should be documented using a third-person verb phrase of the form `Provides <classes and predicates to do something>.` |
| 243 | + |
| 244 | +##### Example |
| 245 | + |
| 246 | +```ql |
| 247 | +/** Provides logic for determining constant expressions. */ |
| 248 | +``` |
| 249 | +```ql |
| 250 | +/** Provides classes representing the control flow graph within functions. */ |
| 251 | +``` |
| 252 | + |
| 253 | +### Special variables |
| 254 | + |
| 255 | +When referring to `this`, you may either refer to it as `` `this` `` or `this <type>`. For example: |
| 256 | +- ``Holds if `this` is static.`` |
| 257 | +- `Holds if this method is static.` |
| 258 | + |
| 259 | +When referring to `result`, you may either refer to it as `` `result` `` or as `the result`. For example: |
| 260 | +- ``Holds if `result` is a child of this expression.`` |
| 261 | +- `Holds if the result is a child of this expression.` |
0 commit comments