Skip to content

Commit 395db3d

Browse files
robertoaloimeta-codesync[bot]
authored andcommitted
Add lists_reverse_append linter
Summary: Introduce a new linter which identifies usages of the following pattern: ``` lists:reverse(List) ++ Tail ``` Suggesting to replace them with the more performing: ``` lists:reverse(List, Tail) ``` Reviewed By: michalmuskala, alanz Differential Revision: D86967410 fbshipit-source-id: cc5142aa9548a69792bb5ba9ff6698afbd38227c
1 parent 4486d66 commit 395db3d

File tree

4 files changed

+238
-0
lines changed

4 files changed

+238
-0
lines changed

crates/ide/src/diagnostics.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ mod head_mismatch;
108108
mod inefficient_enumerate;
109109
mod inefficient_flatlength;
110110
mod inefficient_last;
111+
mod lists_reverse_append;
111112
mod macro_precedence_suprise;
112113
mod map_find_to_syntax;
113114
mod map_insertion_to_syntax;
@@ -1558,6 +1559,7 @@ const SSR_PATTERN_LINTERS: &[&dyn SsrPatternsDiagnostics] = &[
15581559
&binary_string_to_sigil::LINTER,
15591560
&unnecessary_map_to_list_in_comprehension::LINTER,
15601561
&could_be_a_string_literal::LINTER,
1562+
&lists_reverse_append::LINTER,
15611563
];
15621564

15631565
/// Generic linters
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is dual-licensed under either the MIT license found in the
5+
* LICENSE-MIT file in the root directory of this source tree or the Apache
6+
* License, Version 2.0 found in the LICENSE-APACHE file in the root directory
7+
* of this source tree. You may select, at your option, one of the
8+
* above-listed licenses.
9+
*/
10+
11+
//! Lint: lists_reverse_append
12+
//!
13+
//! Detect patterns of the form `lists:reverse(_@L) ++ _@T` and suggest `lists:reverse(_@L, _@T)`
14+
15+
use elp_ide_assists::Assist;
16+
use elp_ide_db::DiagnosticCode;
17+
use elp_ide_db::elp_base_db::FileId;
18+
use elp_ide_db::source_change::SourceChangeBuilder;
19+
use hir::Semantic;
20+
21+
use crate::diagnostics::Linter;
22+
use crate::diagnostics::SsrPatternsLinter;
23+
use crate::fix;
24+
25+
pub(crate) struct ListsReverseAppendLinter;
26+
27+
impl Linter for ListsReverseAppendLinter {
28+
fn id(&self) -> DiagnosticCode {
29+
DiagnosticCode::ListsReverseAppend
30+
}
31+
32+
fn description(&self) -> &'static str {
33+
"Use `lists:reverse/2` instead of `lists:reverse/1 ++ Tail` for better performance."
34+
}
35+
}
36+
37+
impl SsrPatternsLinter for ListsReverseAppendLinter {
38+
type Context = ();
39+
40+
fn patterns(&self) -> Vec<(String, Self::Context)> {
41+
vec![(format!("ssr: lists:reverse({LIST_VAR}) ++ {TAIL_VAR}."), ())]
42+
}
43+
44+
fn fixes(
45+
&self,
46+
_context: &Self::Context,
47+
matched: &elp_ide_ssr::Match,
48+
sema: &Semantic,
49+
_file_id: FileId,
50+
) -> Option<Vec<Assist>> {
51+
let list_match = matched.placeholder_text(sema, LIST_VAR)?;
52+
let tail_match = matched.placeholder_text(sema, TAIL_VAR)?;
53+
let mut builder = SourceChangeBuilder::new(matched.range.file_id);
54+
let replacement = format!("lists:reverse({list_match}, {tail_match})");
55+
let range = matched.range.range;
56+
builder.replace(range, replacement);
57+
let fixes = vec![fix(
58+
"lists_reverse_append",
59+
"Use lists:reverse/2",
60+
builder.finish(),
61+
range,
62+
)];
63+
Some(fixes)
64+
}
65+
}
66+
67+
pub(crate) static LINTER: ListsReverseAppendLinter = ListsReverseAppendLinter;
68+
69+
static LIST_VAR: &str = "_@L";
70+
static TAIL_VAR: &str = "_@T";
71+
72+
#[cfg(test)]
73+
mod tests {
74+
75+
use expect_test::Expect;
76+
use expect_test::expect;
77+
78+
use crate::diagnostics::Diagnostic;
79+
use crate::diagnostics::DiagnosticCode;
80+
use crate::tests;
81+
82+
fn filter(d: &Diagnostic) -> bool {
83+
d.code == DiagnosticCode::ListsReverseAppend
84+
}
85+
86+
#[track_caller]
87+
fn check_diagnostics(fixture: &str) {
88+
tests::check_filtered_diagnostics(fixture, &filter)
89+
}
90+
91+
#[track_caller]
92+
fn check_fix(fixture_before: &str, fixture_after: Expect) {
93+
tests::check_fix(fixture_before, fixture_after)
94+
}
95+
96+
#[test]
97+
fn detects_lists_reverse_append() {
98+
check_diagnostics(
99+
r#"
100+
//- /src/lists_reverse_append.erl
101+
-module(lists_reverse_append).
102+
103+
reverse_and_append(List, Tail) ->
104+
lists:reverse(List) ++ Tail.
105+
%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 warning: W0056: Use `lists:reverse/2` instead of `lists:reverse/1 ++ Tail` for better performance.
106+
"#,
107+
)
108+
}
109+
110+
#[test]
111+
fn detects_lists_reverse_append_with_variables() {
112+
check_diagnostics(
113+
r#"
114+
//- /src/lists_reverse_append.erl
115+
-module(lists_reverse_append).
116+
117+
process(Items, Acc) ->
118+
lists:reverse(Items) ++ Acc.
119+
%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 warning: W0056: Use `lists:reverse/2` instead of `lists:reverse/1 ++ Tail` for better performance.
120+
"#,
121+
)
122+
}
123+
124+
#[test]
125+
fn ignores_regular_reverse() {
126+
check_diagnostics(
127+
r#"
128+
//- /src/lists_reverse_append.erl
129+
-module(lists_reverse_append).
130+
131+
reverse_only(List) ->
132+
lists:reverse(List).
133+
"#,
134+
)
135+
}
136+
137+
#[test]
138+
fn ignores_regular_append() {
139+
check_diagnostics(
140+
r#"
141+
//- /src/lists_reverse_append.erl
142+
-module(lists_reverse_append).
143+
144+
append_only(A, B) ->
145+
A ++ B.
146+
"#,
147+
)
148+
}
149+
150+
#[test]
151+
fn fixes_lists_reverse_append() {
152+
check_fix(
153+
r#"
154+
//- /src/lists_reverse_append.erl
155+
-module(lists_reverse_append).
156+
157+
reverse_and_append(List, Tail) ->
158+
lists:re~verse(List) ++ Tail.
159+
"#,
160+
expect![[r#"
161+
-module(lists_reverse_append).
162+
163+
reverse_and_append(List, Tail) ->
164+
lists:reverse(List, Tail).
165+
"#]],
166+
)
167+
}
168+
169+
#[test]
170+
fn fixes_lists_reverse_append_with_complex_expressions() {
171+
check_fix(
172+
r#"
173+
//- /src/lists_reverse_append.erl
174+
-module(lists_reverse_append).
175+
176+
process([H|T], Acc) ->
177+
lists:rev~erse([H|T]) ++ [1, 2, 3].
178+
"#,
179+
expect![[r#"
180+
-module(lists_reverse_append).
181+
182+
process([H|T], Acc) ->
183+
lists:reverse([H|T], [1, 2, 3]).
184+
"#]],
185+
)
186+
}
187+
}

crates/ide_db/src/diagnostic_code.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ pub enum DiagnosticCode {
8989
NoErrorLogger,
9090
NoNoWarnSuppressions,
9191
CouldBeAStringLiteral,
92+
ListsReverseAppend,
9293

9394
// Wrapper for erlang service diagnostic codes
9495
ErlangService(String),
@@ -248,6 +249,7 @@ impl DiagnosticCode {
248249
DiagnosticCode::NoErrorLogger => "W0053".to_string(),
249250
DiagnosticCode::NoNoWarnSuppressions => "W0054".to_string(),
250251
DiagnosticCode::CouldBeAStringLiteral => "W0055".to_string(),
252+
DiagnosticCode::ListsReverseAppend => "W0056".to_string(),
251253
DiagnosticCode::ErlangService(c) => c.to_string(),
252254
DiagnosticCode::Eqwalizer(c) => format!("eqwalizer: {c}"),
253255
DiagnosticCode::AdHoc(c) => format!("ad-hoc: {c}"),
@@ -344,6 +346,7 @@ impl DiagnosticCode {
344346
DiagnosticCode::NoErrorLogger => "no_error_logger".to_string(),
345347
DiagnosticCode::NoNoWarnSuppressions => "no_nowarn_suppressions".to_string(),
346348
DiagnosticCode::CouldBeAStringLiteral => "could_be_a_binary_string_literal".to_string(),
349+
DiagnosticCode::ListsReverseAppend => "lists_reverse_append".to_string(),
347350

348351
DiagnosticCode::ErlangService(c) => c.to_string(),
349352
DiagnosticCode::Eqwalizer(c) => c.to_string(),
@@ -520,6 +523,7 @@ impl DiagnosticCode {
520523
DiagnosticCode::NoCatch => false,
521524
DiagnosticCode::NoErrorLogger => false,
522525
DiagnosticCode::NoNoWarnSuppressions => false,
526+
DiagnosticCode::ListsReverseAppend => false,
523527

524528
DiagnosticCode::BinaryStringToSigil => false,
525529
DiagnosticCode::ErlangService(_) => false,
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
---
2+
sidebar_position: 56
3+
---
4+
5+
# W0056 - Use `lists:reverse/2` instead of `lists:reverse/1 ++ Tail`
6+
7+
## Warning
8+
9+
```erlang
10+
reverse_and_append(List, Tail) ->
11+
lists:reverse(List) ++ Tail.
12+
%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 warning: W0056: Use `lists:reverse/2` instead of `lists:reverse/1 ++ Tail` for better performance.
13+
```
14+
15+
## Explanation
16+
17+
The warning indicates that you are using the pattern `lists:reverse(List) ++ Tail`, which is inefficient. This pattern first reverses the list, creating an intermediate reversed list, and then appends the tail to it using the `++` operator.
18+
19+
The Erlang standard library provides a more efficient function `lists:reverse/2` that performs both operations in a single pass without creating the intermediate reversed list.
20+
21+
### Why This Matters
22+
23+
- **Performance**: Using `lists:reverse/1` followed by `++` creates an unnecessary intermediate data structure
24+
- **Memory**: The intermediate reversed list consumes additional memory that is immediately discarded
25+
- **Idiomatic Erlang**: `lists:reverse/2` is the idiomatic way to reverse and prepend in Erlang
26+
27+
### Fix
28+
29+
Replace the pattern with `lists:reverse/2`:
30+
31+
```erlang
32+
reverse_and_append(List, Tail) ->
33+
lists:reverse(List, Tail).
34+
```
35+
36+
### How lists:reverse/2 Works
37+
38+
The second argument to `lists:reverse/2` acts as an accumulator that gets prepended to the reversed list:
39+
40+
```erlang
41+
lists:reverse([1, 2, 3], [4, 5, 6]).
42+
% Returns: [3, 2, 1, 4, 5, 6]
43+
```
44+
45+
This is equivalent to `lists:reverse([1, 2, 3]) ++ [4, 5, 6]` but significantly more efficient.

0 commit comments

Comments
 (0)