Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

docs(rome_js_analyze): improve CONTRIBUTING #4280

Merged
merged 1 commit into from
Mar 8, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 38 additions & 30 deletions crates/rome_analyze/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,23 @@ New rules should always be considered unstable/not exhaustive.
In addition to selecting a group, rules may be flagged as `recommended`.
The **recommended rules** are enabled in the default configuration of the _Rome_ linter.
As a general principle, a recommended rule should catch actual programming errors.
For instance detecting a coding pattern that will throw an exception at runtime.
Pedantic rules that check for certain unwanted patterns but may have high false positive rates,
For instance, detecting a coding pattern that will throw an exception at runtime.
Pedantic rules that check for specific unwanted patterns but may have high false positive rates,
should be left off from the recommended set.
Rules intended to be recommended should be flagged as such even if they are still part of the `nursery` group,
as unstable rules are only enabled by default on unstable builds.
This gives to the project time to test the rule, find edge cases, etc.
This gives the project time to test the rule, find edge cases, etc.

## Lint rules

When creating or updating a lint rule, you need to be aware that there's a lot of generated code inside our toolchain.
Our CI makes sure that this code is not out of sync and fails otherwise.
Our CI ensures that this code is not out of sync and fails otherwise.
See the [code generation section](#code-generation) for more details.

To create a new rule, you have to create and update several files.
Because it is a bit tedious, _Rome_ provides an easy way to create and test your rule using [Just](https://just.systems/man/en/).
_Just_ is not part of the rust toolchain, you have to install it with [a package manager](https://just.systems/man/en/chapter_4.html).

### Choose a name

_Rome_ follows a naming convention according to what the rule do:
Expand All @@ -68,12 +72,12 @@ _Rome_ follows a naming convention according to what the rule do:

### Create and implement the rule

Let's say we want to create a new rule called `useAwesomeTricks`, which uses the semantic model.
Let's say we want to create a new rule called `myRuleName`, which uses the semantic model.

1. Run the command

```shell
> just new-lintrule crates/rome_js_analyze/src/analyzers/nursery myRuleName
> just new-lintrule crates/rome_js_analyze/src/semantic_analyzers/nursery myRuleName
```

Rules go in different folders, and the folder depend on the type of query system your rule
Expand All @@ -94,11 +98,12 @@ Let's say we want to create a new rule called `useAwesomeTricks`, which uses the
3. The `State` type doesn't have to be used, so it can be considered optional.
However, it has to be defined as `type State = ()`.

4. The `run` function must be implemented.
4. Implement the `run` function:

This function is called every time the analyzer finds a match for the query specified by the rule,
and may return zero or more "signals".

5. Implement the optional `diagnostic` function, to tell the user where's the error and why:
5. Implement the `diagnostic` function, to tell the user where's the error and why:

```rust,ignore
impl Rule for UseAwesomeTricks {
Expand All @@ -108,7 +113,7 @@ Let's say we want to create a new rule called `useAwesomeTricks`, which uses the
```

While implementing the diagnostic, please keep [Rome's technical principals](https://rome.tools/#technical) in mind.
This function is called of every signal emitted by the `run` function, and it may return
This function is called for every signal emitted by the `run` function, and it may return
zero or one diagnostic.

6. Implement the optional `action` function, if we are able to provide automatic code fix to the rule:
Expand All @@ -120,12 +125,16 @@ Let's say we want to create a new rule called `useAwesomeTricks`, which uses the
}
```

This function is called of every signal emitted by the `run` function.
This function is called for every signal emitted by the `run` function.
It may return zero or one code action.
When returning a code action, you will need to pass `category` and `applicability` fields.
`category` must be `ActionCategory::QuickFix`, while `applicability` must be `Applicability:MaybeIncorrect`.
When returning a code action, you must pass the `category` and the `applicability` fields.
`category` must be `ActionCategory::QuickFix`.
`applicability` is either `Applicability:MaybeIncorrect` or `Applicability:Always`.
`Applicability:Always` must only be used when the code transformation is safe.
In other words, the code transformation should always result in code that does no change the behavior of the code.
In the case of `noVar`, it is not always safe to turn `var` to `const` or `let`.

Don't forget to format your code with `cargo fmt` and lint with `cargo clippy`.
Don't forget to format your code with `cargo format` and lint with `cargo lint`.

That's it! Now, let's test the rule.

Expand All @@ -136,24 +145,23 @@ The test infrastructure is rigid around the association of the pair "group/rule
_**your test cases are placed inside the wrong group, you won't see any diagnostics**_.

Since each new rule will start from `nursery`, that's where we start.

If you used `just new-lintrule`, a folder that use the name of the rule should exist.

Otherwise, you have two options:

- Create a single file called like the rule name, e.g. `useAwesomeTricks.js`, `useAwesomeTricks.tsx`, etc.
- Create a single file called like the rule name, e.g. `myRuleName.js`, `myRuleName.tsx`, etc.
The extension of the file matters based on which kind of file you need to test;

- Create a folder called `useAwesomeTricks/`, and then create various files where you want to create different cases.
- Create a folder called `myRuleName/`, and then create various files where you want to create different cases.
These options are useful if your rules target different super languages, or you want to split your cases among different files.

If you chose to create a folder, a common pattern is to create files prefixed by `invalid` or `valid`.
The files prefixed by `invalid` contain code that are reported by the rule.
The files prefixed by `valid` contain code that are not reported by the rule.

Files ending with the extension `.jsonc` are handled in a different way.
Files ending with the extension `.jsonc` are differently handled.
These files should contain an array of strings where each string is a code snippet.
For instance for the rule `noVar`, the file `invalidScript.jsonc` contains:
For instance, for the rule `noVar`, the file `invalidScript.jsonc` contains:

```jsonc
[
Expand All @@ -163,9 +171,9 @@ For instance for the rule `noVar`, the file `invalidScript.jsonc` contains:
```

Note that code in a file ending with the extension `.jsonc` are in a _script environment_.
That's mean that you cannot use syntax that belongs to ECMAScript modules such as `import` and `export`.
This means that you cannot use syntax that belongs to _ECMAScript modules_ such as `import` and `export`.

Run the command `just test-lintrule useAwesomeTricks` and if you've done everything correctly,
Run the command `just test-lintrule myRuleName` and if you've done everything correctly,
you should see some snapshots emitted with diagnostics and code actions.

Check our main [contribution document](https://github.com/rome/tools/blob/main/CONTRIBUTING.md#snapshot-tests)
Expand All @@ -177,7 +185,7 @@ The doc-comment for the rule is mandatory and is used to generate the
documentation page for the rule on the website.
Importantly, the tool used to generate those pages also runs tests on the
code blocks included in the documentation written in languages supported by
the Rome toolchain (JavaScript, JSX, TypeScript, ...) similar to how
the _Rome_ toolchain (JavaScript, JSX, TypeScript, ...) similar to how
`rustdoc` generates tests from code blocks written in Rust. Because code
blocks in Rust doc-comments are assumed to be written in Rust by default
the language of the test must be explicitly specified, for instance:
Expand Down Expand Up @@ -269,7 +277,7 @@ Stage and commit your changes:

```shell
> git add -A
> git commit -m 'feat(rome_js_analyze): noVar'
> git commit -m 'feat(rome_js_analyze): myRuleName'
```

To test if everything is ready, run the following command:
Expand All @@ -278,11 +286,10 @@ To test if everything is ready, run the following command:
> just check-ready
```

For more details on the available automations, look at our `justfile` at the root of the repository.

### Rule configuration

Some rules may allow customization using configuration. The first step is to setup a struct to represent the rule configuration.
Some rules may allow customization using configuration.
The first step is to setup a struct to represent the rule configuration.

```rust,ignore
#[derive(Debug, Clone, Serialize, Deserialize)]
Expand Down Expand Up @@ -501,7 +508,7 @@ impl Rule for UseYield {

### `declare_rule`

This macro is used to declare an analyzer rule type, and implement the [RuleMeta] trait for it
This macro is used to declare an analyzer rule type, and implement the [RuleMeta] trait for it.

The macro itself expect the following syntax:

Expand All @@ -511,14 +518,14 @@ use rome_analyze::declare_rule;
declare_rule! {
/// Documentation
pub(crate) ExampleRule {
version: "0.7.0",
name: "ruleName",
version: "next",
name: "myRuleName",
recommended: false,
}
}
```

#### Category Macro
### Category Macro

Declaring a rule using `declare_rule!` will cause a new `rule_category!`
macro to be declared in the surrounding module. This macro can be used to
Expand All @@ -533,7 +540,8 @@ declare_rule! {
/// Documentation
pub(crate) ExampleRule {
version: "next",
name: "ruleName",
name: "myRuleName",
recommended: false,
}
}

Expand Down