Skip to content

Commit 2f7e57a

Browse files
0xrusowsky0xrusowsky
authored
fix(forge-lint): [inline-config] use relative span positions (#11022)
1 parent d0eca3d commit 2f7e57a

File tree

5 files changed

+80
-52
lines changed

5 files changed

+80
-52
lines changed

‎crates/forge/tests/cli/lint.rs

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,10 @@ const OTHER_CONTRACT: &str = r#"
2929
// SPDX-License-Identifier: MIT
3030
pragma solidity ^0.8.0;
3131
32-
contract ContractWithLints {
32+
// forge-lint: disable-next-line
33+
import { ContractWithLints } from "ContractWithLints.sol";
34+
35+
contract OtherContractWithLints {
3336
uint256 VARIABLE_MIXED_CASE_INFO;
3437
}
3538
"#;
@@ -77,9 +80,9 @@ forgetest!(can_use_config_ignore, |prj, cmd| {
7780
});
7881
cmd.arg("lint").assert_success().stderr_eq(str![[r#"
7982
note[mixed-case-variable]: mutable variables should use mixedCase
80-
[FILE]:6:17
83+
[FILE]:9:17
8184
|
82-
6 | uint256 VARIABLE_MIXED_CASE_INFO;
85+
9 | uint256 VARIABLE_MIXED_CASE_INFO;
8386
| ------------------------
8487
|
8588
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable
@@ -115,9 +118,9 @@ forgetest!(can_override_config_severity, |prj, cmd| {
115118
});
116119
cmd.arg("lint").args(["--severity", "info"]).assert_success().stderr_eq(str![[r#"
117120
note[mixed-case-variable]: mutable variables should use mixedCase
118-
[FILE]:6:17
121+
[FILE]:9:17
119122
|
120-
6 | uint256 VARIABLE_MIXED_CASE_INFO;
123+
9 | uint256 VARIABLE_MIXED_CASE_INFO;
121124
| ------------------------
122125
|
123126
= help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable
@@ -344,6 +347,18 @@ Warning (2018): Function state mutability can be restricted to pure
344347
"#]]);
345348
});
346349

350+
forgetest!(can_process_inline_config_regardless_of_input_order, |prj, cmd| {
351+
prj.wipe_contracts();
352+
prj.add_source("ContractWithLints", CONTRACT).unwrap();
353+
prj.add_source("OtherContractWithLints", OTHER_CONTRACT).unwrap();
354+
cmd.arg("lint").assert_success();
355+
356+
prj.wipe_contracts();
357+
prj.add_source("OtherContractWithLints", OTHER_CONTRACT).unwrap();
358+
prj.add_source("ContractWithLints", CONTRACT).unwrap();
359+
cmd.arg("lint").assert_success();
360+
});
361+
347362
#[tokio::test]
348363
async fn ensure_lint_rule_docs() {
349364
const FOUNDRY_BOOK_LINT_PAGE_URL: &str =

‎crates/lint/src/inline_config.rs

Lines changed: 33 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use solar_ast::{Item, SourceUnit, visit::Visit};
2+
use solar_interface::SourceMap;
23
use solar_parse::ast::Span;
34
use std::{collections::HashMap, fmt, marker::PhantomData, ops::ControlFlow};
45

@@ -110,10 +111,10 @@ impl InlineConfig {
110111
pub fn new<'ast>(
111112
items: impl IntoIterator<Item = (Span, InlineConfigItem)>,
112113
ast: &'ast SourceUnit<'ast>,
113-
src: &str,
114+
source_map: &SourceMap,
114115
) -> Self {
115116
let mut disabled_ranges: HashMap<String, Vec<DisabledRange>> = HashMap::new();
116-
let mut disabled_blocks: HashMap<String, (usize, usize)> = HashMap::new();
117+
let mut disabled_blocks: HashMap<String, (usize, usize, usize)> = HashMap::new();
117118

118119
let mut prev_sp = Span::DUMMY;
119120
for (sp, item) in items {
@@ -122,11 +123,11 @@ impl InlineConfig {
122123
prev_sp = sp;
123124
}
124125

126+
let Ok((file, comment_range)) = source_map.span_to_source(sp) else { continue };
127+
let src = file.src.as_str();
125128
match item {
126129
InlineConfigItem::DisableNextItem(lints) => {
127-
let comment_end = sp.hi().to_usize();
128-
129-
if let Some(next_item) = NextItemFinder::new(comment_end).find(ast) {
130+
if let Some(next_item) = NextItemFinder::new(sp.hi().to_usize()).find(ast) {
130131
for lint in lints {
131132
disabled_ranges.entry(lint).or_default().push(DisabledRange {
132133
start: next_item.lo().to_usize(),
@@ -137,55 +138,50 @@ impl InlineConfig {
137138
};
138139
}
139140
InlineConfigItem::DisableLine(lints) => {
140-
let mut prev_newline = src[..sp.lo().to_usize()]
141-
.char_indices()
142-
.rev()
143-
.skip_while(|(_, ch)| *ch != '\n');
144-
let start = prev_newline.next().map(|(idx, _)| idx).unwrap_or_default();
141+
let start = src[..comment_range.start].rfind('\n').map_or(0, |i| i);
142+
let end = src[comment_range.end..]
143+
.find('\n')
144+
.map_or(src.len(), |i| comment_range.end + i);
145145

146-
let end_offset = sp.hi().to_usize();
147-
let mut next_newline =
148-
src[end_offset..].char_indices().skip_while(|(_, ch)| *ch != '\n');
149-
let end =
150-
end_offset + next_newline.next().map(|(idx, _)| idx).unwrap_or_default();
151146
for lint in lints {
152147
disabled_ranges.entry(lint).or_default().push(DisabledRange {
153-
start,
154-
end,
148+
start: start + file.start_pos.to_usize(),
149+
end: end + file.start_pos.to_usize(),
155150
loose: false,
156151
})
157152
}
158153
}
159154
InlineConfigItem::DisableNextLine(lints) => {
160-
let offset = sp.hi().to_usize();
161-
let mut char_indices =
162-
src[offset..].char_indices().skip_while(|(_, ch)| *ch != '\n').skip(1);
163-
if let Some((mut start, _)) = char_indices.next() {
164-
start += offset;
165-
let end = char_indices
166-
.find(|(_, ch)| *ch == '\n')
167-
.map(|(idx, _)| offset + idx + 1)
168-
.unwrap_or(src.len());
169-
for lint in lints {
170-
disabled_ranges.entry(lint).or_default().push(DisabledRange {
171-
start,
172-
end,
173-
loose: false,
174-
})
155+
if let Some(offset) = src[comment_range.end..].find('\n') {
156+
let start = comment_range.end + offset + 1;
157+
if start < src.len() {
158+
let end = src[start..].find('\n').map_or(src.len(), |i| start + i);
159+
for lint in lints {
160+
disabled_ranges.entry(lint).or_default().push(DisabledRange {
161+
start: start + file.start_pos.to_usize(),
162+
end: end + file.start_pos.to_usize(),
163+
loose: false,
164+
})
165+
}
175166
}
176167
}
177168
}
178169
InlineConfigItem::DisableStart(lints) => {
179170
for lint in lints {
180171
disabled_blocks
181172
.entry(lint)
182-
.and_modify(|(_, depth)| *depth += 1)
183-
.or_insert((sp.hi().to_usize(), 1));
173+
.and_modify(|(_, depth, _)| *depth += 1)
174+
.or_insert((
175+
sp.hi().to_usize(),
176+
1,
177+
// Use file end as fallback for unclosed blocks
178+
file.start_pos.to_usize() + src.len(),
179+
));
184180
}
185181
}
186182
InlineConfigItem::DisableEnd(lints) => {
187183
for lint in lints {
188-
if let Some((start, depth)) = disabled_blocks.get_mut(&lint) {
184+
if let Some((start, depth, _)) = disabled_blocks.get_mut(&lint) {
189185
*depth = depth.saturating_sub(1);
190186

191187
if *depth == 0 {
@@ -204,10 +200,10 @@ impl InlineConfig {
204200
}
205201
}
206202

207-
for (lint, (start, _)) in disabled_blocks {
203+
for (lint, (start, _, file_end)) in disabled_blocks {
208204
disabled_ranges.entry(lint).or_default().push(DisabledRange {
209205
start,
210-
end: src.len(),
206+
end: file_end,
211207
loose: false,
212208
});
213209
}

‎crates/lint/src/sol/mod.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,8 @@ impl SolidityLinter {
124124
.unzip();
125125

126126
// Process the inline-config
127-
let source = file.src.as_str();
128127
let comments = Comments::new(&file);
129-
let inline_config = parse_inline_config(sess, &comments, &lints, &ast, source);
128+
let inline_config = parse_inline_config(sess, &comments, &lints, &ast);
130129

131130
// Initialize and run the visitor
132131
let ctx = LintContext::new(sess, self.with_description, inline_config);
@@ -176,7 +175,6 @@ fn parse_inline_config<'ast>(
176175
comments: &Comments,
177176
lints: &[&'static str],
178177
ast: &'ast SourceUnit<'ast>,
179-
src: &str,
180178
) -> InlineConfig {
181179
let items = comments.iter().filter_map(|comment| {
182180
let mut item = comment.lines.first()?.as_str();
@@ -197,7 +195,7 @@ fn parse_inline_config<'ast>(
197195
}
198196
});
199197

200-
InlineConfig::new(items, ast, src)
198+
InlineConfig::new(items, ast, sess.source_map())
201199
}
202200

203201
#[derive(Error, Debug)]

‎crates/lint/testdata/Imports.sol

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,17 @@ import {
1515
IContractNotUsed //~NOTE: unused imports should be removed
1616
} from "File.sol";
1717

18+
// forge-lint: disable-next-item
19+
import {
20+
symbolNotUsed2
21+
} from "File.sol";
22+
23+
// in this case, disabling the following line doesn't do anything
24+
// forge-lint: disable-next-line
25+
import {
26+
symbolNotUsed3 //~NOTE: unused imports should be removed
27+
} from "File.sol";
28+
1829
import {
1930
CONSTANT_0,
2031
CONSTANT_1 //~NOTE: unused imports should be removed

‎crates/lint/testdata/Imports.stderr

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
note[unaliased-plain-import]: use named imports '{A, B}' or alias 'import ".." as X'
22
--> ROOT/testdata/Imports.sol:LL:CC
33
|
4-
29 | import "SomeFile.sol";
4+
40 | import "SomeFile.sol";
55
| --------------
66
|
77
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unaliased-plain-import
88

99
note[unaliased-plain-import]: use named imports '{A, B}' or alias 'import ".." as X'
1010
--> ROOT/testdata/Imports.sol:LL:CC
1111
|
12-
30 | import "AnotherFile.sol";
12+
41 | import "AnotherFile.sol";
1313
| -----------------
1414
|
1515
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unaliased-plain-import
@@ -49,31 +49,39 @@ note[unused-import]: unused imports should be removed
4949
note[unused-import]: unused imports should be removed
5050
--> ROOT/testdata/Imports.sol:LL:CC
5151
|
52-
20 | CONSTANT_1
52+
26 | symbolNotUsed3
53+
| --------------
54+
|
55+
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import
56+
57+
note[unused-import]: unused imports should be removed
58+
--> ROOT/testdata/Imports.sol:LL:CC
59+
|
60+
31 | CONSTANT_1
5361
| ----------
5462
|
5563
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import
5664

5765
note[unused-import]: unused imports should be removed
5866
--> ROOT/testdata/Imports.sol:LL:CC
5967
|
60-
26 | YetAnotherType
68+
37 | YetAnotherType
6169
| --------------
6270
|
6371
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import
6472

6573
note[unused-import]: unused imports should be removed
6674
--> ROOT/testdata/Imports.sol:LL:CC
6775
|
68-
33 | import "another_file_2.sol" as AnotherFile2;
76+
44 | import "another_file_2.sol" as AnotherFile2;
6977
| --------------------------------------------
7078
|
7179
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import
7280

7381
note[unused-import]: unused imports should be removed
7482
--> ROOT/testdata/Imports.sol:LL:CC
7583
|
76-
36 | import * as OtherUtils from "utils2.sol";
84+
47 | import * as OtherUtils from "utils2.sol";
7785
| -----------------------------------------
7886
|
7987
= help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import

0 commit comments

Comments
 (0)