Miguel Raz Guzmán Macedo
ca2b5741bc
## Objective
Sometimes we can do a hash lookup twice, which is slow and unidiomatic.
We should use the Entry API instead.
## Solution
Add another `dylint` lint called `map_lookup_then_insert` that catches
those cases.
This works with HashMap, BTreeMap, HashSet, BTreeSet and
entry-compatible ADTs.
## What it catches
See the examples in the `tooling/lints/ui/map_lookup_then_insert.rs`.
It's mostly things of the form
```rust
if let Some(value) = map.get(&key) {
*value
} else {
map.insert(key, 1);
1
}
```
which should be
```rust
*map.entry(key).or_insert(1)
```
The two key expressions are matched up to borrows/derefs,
value-preserving
conversions (`clone`, `to_owned`, `to_string` on string types, `to_vec`
on
slices, `String::from` on `&str` — each resolved by definition, not by
name),
and immutable local aliases. So looking up `&key` and inserting
`key.clone()`
is caught too. When a conversion had to be peeled, the diagnostic
carries a
note that an entry-API rewrite would perform that copy on hits as well
as
misses.
## What it deliberately does not catch
Forms Clippy already catches.
## Findings
A `--workspace` run finds 31 warnings at 30 unique sites in `zed`:
the 16 the plain key matching finds, plus 15 more that require the
conversion/alias matching (e.g. `tiles_by_key.get(key)` followed by
`tiles_by_key.insert(key.clone(), tile)` in `metal_atlas.rs`).
Will post those fixes in a follow up PR.
## Testing
UI tests cover every `Entry` method positively and negatively, BTreeMap
mirrors, set cases, and the Clippy-deferral boundary. The
key-normalization logic is covered by nine positive cases (cloned keys,
UFCS forms, `to_owned`/`to_string`/`to_vec`/`String::from`, deref
layers, local aliases, sets) and six negative cases (inherent methods
merely named `clone`/`to_owned`, mutated aliases, a clone of a different
key, `to_string` on a non-string key, and side-effecting initializers).
## Self-Review Checklist:
- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content adheres to Zed's UI standards
([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
and
[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)
guidelines)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable
Release Notes:
- N/A
88 lines
3.6 KiB
Markdown
88 lines
3.6 KiB
Markdown
# lints
|
|
|
|
A [dylint](https://github.com/trailofbits/dylint) library that flags various bad patterns in our codebase.
|
|
|
|
Install `dylint`, a pinned nightly toolchain and the necessary tools with
|
|
|
|
```
|
|
cargo install cargo-dylint dylint-link
|
|
cd tooling/lints
|
|
rustup toolchain install
|
|
```
|
|
|
|
The channel and its components (`rustc-dev`, `rust-src`, `llvm-tools-preview`)
|
|
are declared in `tooling/lints/rust-toolchain.toml`, so `rustup toolchain install`
|
|
picks them up automatically when run from that directory.
|
|
|
|
# Demo
|
|
|
|
```
|
|
./single-lint blocking_io_on_foreground
|
|
```
|
|
|
|
|
|
## Current lints
|
|
- `shared_string_from_str_literal` — `SharedString::new/from` etc where `SharedString::from_static` should be used instead.
|
|
- `async_block_without_await` — `async { … }` blocks whose body contains no `.await` expression.
|
|
- `entity_update_in_render` — `Entity::update`/`WeakEntity::update` mutating an entity inside `Render::render`.
|
|
- `map_lookup_then_insert` — map value lookups that branch into an insertion for the same key, on any map with an `entry` method (`HashMap`, `BTreeMap`, `FxHashMap`, `hashbrown`, `indexmap`, ...), and the same pattern on `HashSet`/`BTreeSet` (where a bare `insert` already searches only once). Keys are matched up to borrows/derefs, value-preserving conversions (`clone`, `to_owned`, `to_string` on string types, `to_vec` on slices, `String::from` on `&str` — each resolved by definition, not name), and immutable local aliases, so looking up `&key` and inserting `key.clone()` is caught.
|
|
- `notify_in_render` — `Context::notify()` called inside `Render::render`.
|
|
- `owned_string_into_shared` — `String::from(<lit>).into()` / `<lit>.to_string().into()` / `<lit>.to_owned().into()` whose target is `SharedString`, `Arc<str>`, `Rc<str>`, or `Cow<'_, str>`.
|
|
- `blocking_io_on_foreground` - Catch blocking IO calls that are called on the main thread (but not on closures or background threads)
|
|
|
|
## How to run
|
|
|
|
Ideally you run this as part of the `clippy` script in the `zed/scripts` directory since this will also run our other linters.
|
|
|
|
### Prerequisites
|
|
|
|
Install both tools (version 6 or later):
|
|
|
|
```
|
|
cargo install cargo-dylint dylint-link
|
|
```
|
|
|
|
- `cargo-dylint` is the `cargo` subcommand that builds and runs the lints; `dylint-link` is the linker used to build the lint library.
|
|
|
|
The workspace registers this library under `[workspace.metadata.dylint]` in the
|
|
root `Cargo.toml`, so Dylint discovers it automatically — you do not pass a
|
|
`--path`. The first run builds the library against its pinned nightly (see
|
|
`rust-toolchain.toml`) and is slow; later runs are cached.
|
|
|
|
### Run all lints against the whole repo
|
|
|
|
```
|
|
cargo dylint --all -- --workspace
|
|
```
|
|
|
|
### Run all lints against a single crate
|
|
|
|
```
|
|
cargo dylint --all -- -p project_panel
|
|
```
|
|
|
|
### Run a single lint
|
|
|
|
The library loads every lint at once. To run just one, use the `single-lint`
|
|
helper, which silences the rest and force-enables the one you name:
|
|
|
|
```
|
|
tooling/lints/single-lint blocking_io_on_foreground -p project_panel
|
|
```
|
|
|
|
The first argument is the lint name (one of the snake_case identifiers under
|
|
[Current lints](#current-lints)); everything after it is passed to `cargo check`
|
|
and defaults to `--workspace`. Under the hood the script runs:
|
|
|
|
```
|
|
DYLINT_RUSTFLAGS="-A warnings --force-warn <lint>" cargo dylint --all -- <args>
|
|
```
|
|
|
|
It also handles two non-obvious gotchas:
|
|
|
|
- `--force-warn` is required: after `-A warnings` silences the group, a plain
|
|
`-W <lint>` does not reliably re-enable a driver-registered lint.
|
|
- `DYLINT_RUSTFLAGS` is not part of Cargo's fingerprint, so the script cleans the
|
|
targeted package(s) first; otherwise Cargo replays a stale cache and the filter
|
|
appears to do nothing.
|