Skip to content

Commit 2ff8f1f

Browse files
beebbeeb
authored
fix: various validation logic fixes and add a bunch of tests (#33)
* fix(constructor): allow missing natspec if there are no params * fix(constructor): inheritdoc is not a valid NatSpec * fix(enumeration): allow missing natspec if we shouldn't validate params * fix(error): allow missing natspec if there are no params * test(error): add more tests * fix(event): allow missing natpsec if there are no params * fix(function): allow missing natspec if no parameters and no inheridoc needed * test(function): add more tests * fix(modifier): enforce inheritdoc and allow no natspec when there is no param * fix(structure): allow missing natspec if we shouldn't validate members * fix(variable): allow no natspec if we don't enforce inheritdoc
1 parent ee02f55 commit 2ff8f1f

File tree

11 files changed

+1309
-39
lines changed

11 files changed

+1309
-39
lines changed

‎.config/nextest.toml

+1-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
11
[profile.default]
2-
retries = { backoff = "exponential", count = 2, delay = "2s", jitter = true }
3-
slow-timeout = { period = "1m", terminate-after = 3 }
2+
slow-timeout = { period = "20s", terminate-after = 3 }
43
fail-fast = false

‎src/definitions/constructor.rs

+81-11
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use slang_solidity::cst::{NonterminalKind, Query, QueryMatch, TextRange};
33
use crate::{
44
error::Result,
55
lint::{Diagnostic, ItemDiagnostics, ItemType},
6-
natspec::{NatSpec, NatSpecKind},
6+
natspec::NatSpec,
77
};
88

99
use super::{
@@ -71,25 +71,17 @@ impl Validate for ConstructorDefinition {
7171
span: self.span(),
7272
diags: vec![],
7373
};
74-
if !options.constructor {
74+
if !options.constructor || self.params.is_empty() {
7575
return out;
7676
}
77-
// raise error if no NatSpec is available
77+
7878
let Some(natspec) = &self.natspec else {
7979
out.diags.push(Diagnostic {
8080
span: self.span(),
8181
message: "missing NatSpec".to_string(),
8282
});
8383
return out;
8484
};
85-
// if there is `inheritdoc`, no further validation is required
86-
if natspec
87-
.items
88-
.iter()
89-
.any(|n| matches!(n.kind, NatSpecKind::Inheritdoc { .. }))
90-
{
91-
return out;
92-
}
9385
// check params
9486
out.diags.append(&mut check_params(natspec, &self.params));
9587
out
@@ -143,4 +135,82 @@ mod tests {
143135
assert_eq!(res.diags.len(), 1);
144136
assert_eq!(res.diags[0].message, "missing NatSpec");
145137
}
138+
139+
#[test]
140+
fn test_constructor_only_notice() {
141+
let contents = "contract Test {
142+
/// @notice The constructor
143+
constructor(uint256 param1, bytes calldata param2) { }
144+
}";
145+
let res = parse_file(contents).validate(&OPTIONS);
146+
assert_eq!(res.diags.len(), 2);
147+
assert_eq!(res.diags[0].message, "@param param1 is missing");
148+
assert_eq!(res.diags[1].message, "@param param2 is missing");
149+
}
150+
151+
#[test]
152+
fn test_constructor_one_missing() {
153+
let contents = "contract Test {
154+
/// @param param1 The first
155+
constructor(uint256 param1, bytes calldata param2) { }
156+
}";
157+
let res = parse_file(contents).validate(&OPTIONS);
158+
assert_eq!(res.diags.len(), 1);
159+
assert_eq!(res.diags[0].message, "@param param2 is missing");
160+
}
161+
162+
#[test]
163+
fn test_constructor_multiline() {
164+
let contents = "contract Test {
165+
/**
166+
* @param param1 Test
167+
* @param param2 Test2
168+
*/
169+
constructor(uint256 param1, bytes calldata param2) { }
170+
}";
171+
let res = parse_file(contents).validate(&OPTIONS);
172+
assert!(res.diags.is_empty(), "{:#?}", res.diags);
173+
}
174+
175+
#[test]
176+
fn test_constructor_duplicate() {
177+
let contents = "contract Test {
178+
/// @param param1 The first
179+
/// @param param1 The first again
180+
constructor(uint256 param1) { }
181+
}";
182+
let res = parse_file(contents).validate(&OPTIONS);
183+
assert_eq!(res.diags.len(), 1);
184+
assert_eq!(
185+
res.diags[0].message,
186+
"@param param1 is present more than once"
187+
);
188+
}
189+
190+
#[test]
191+
fn test_constructor_no_params() {
192+
let contents = "contract Test {
193+
constructor() { }
194+
}";
195+
let res = parse_file(contents).validate(&OPTIONS);
196+
assert!(res.diags.is_empty(), "{:#?}", res.diags);
197+
}
198+
199+
#[test]
200+
fn test_constructor_inheritdoc() {
201+
let contents = "contract Test {
202+
/// @inheritdoc ITest
203+
constructor(uint256 param1) { }
204+
}";
205+
let res = parse_file(contents).validate(
206+
&ValidationOptions::builder()
207+
.inheritdoc(true) // has no effect on constructor
208+
.constructor(true)
209+
.struct_params(false)
210+
.enum_params(false)
211+
.build(),
212+
);
213+
assert_eq!(res.diags.len(), 1);
214+
assert_eq!(res.diags[0].message, "@param param1 is missing");
215+
}
146216
}

‎src/definitions/enumeration.rs

+195-3
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ impl Validate for EnumDefinition {
7575
span: self.span(),
7676
diags: vec![],
7777
};
78+
if !options.enum_params {
79+
return out;
80+
}
7881
// raise error if no NatSpec is available
7982
let Some(natspec) = &self.natspec else {
8083
out.diags.push(Diagnostic {
@@ -83,9 +86,7 @@ impl Validate for EnumDefinition {
8386
});
8487
return out;
8588
};
86-
if options.enum_params {
87-
out.diags = check_params(natspec, &self.members);
88-
}
89+
out.diags = check_params(natspec, &self.members);
8990
out
9091
}
9192
}
@@ -101,3 +102,194 @@ fn extract_enum_members(cursor: Cursor) -> Vec<Identifier> {
101102
}
102103
out
103104
}
105+
106+
#[cfg(test)]
107+
mod tests {
108+
use semver::Version;
109+
use slang_solidity::{cst::NonterminalKind, parser::Parser};
110+
111+
use super::*;
112+
113+
static OPTIONS: ValidationOptions = ValidationOptions {
114+
inheritdoc: false,
115+
constructor: false,
116+
struct_params: false,
117+
enum_params: true,
118+
};
119+
120+
fn parse_file(contents: &str) -> EnumDefinition {
121+
let parser = Parser::create(Version::new(0, 8, 0)).unwrap();
122+
let output = parser.parse(NonterminalKind::SourceUnit, contents);
123+
assert!(output.is_valid(), "{:?}", output.errors());
124+
let cursor = output.create_tree_cursor();
125+
let m = cursor.query(vec![EnumDefinition::query()]).next().unwrap();
126+
let def = EnumDefinition::extract(m).unwrap();
127+
def.as_enum().unwrap()
128+
}
129+
130+
#[test]
131+
fn test_enum() {
132+
let contents = "contract Test {
133+
enum Foobar {
134+
First,
135+
Second
136+
}
137+
}";
138+
let res = parse_file(contents).validate(
139+
&ValidationOptions::builder()
140+
.inheritdoc(false)
141+
.constructor(false)
142+
.enum_params(false)
143+
.struct_params(false)
144+
.build(),
145+
);
146+
assert!(res.diags.is_empty(), "{:#?}", res.diags);
147+
}
148+
149+
#[test]
150+
fn test_enum_missing() {
151+
let contents = "contract Test {
152+
enum Foobar {
153+
First,
154+
Second
155+
}
156+
}";
157+
let res = parse_file(contents).validate(&OPTIONS);
158+
assert_eq!(res.diags.len(), 1);
159+
assert_eq!(res.diags[0].message, "missing NatSpec");
160+
}
161+
162+
#[test]
163+
fn test_enum_params() {
164+
let contents = "contract Test {
165+
/// @param First The first
166+
/// @param Second The second
167+
enum Foobar {
168+
First,
169+
Second
170+
}
171+
}";
172+
let res = parse_file(contents).validate(&OPTIONS);
173+
assert!(res.diags.is_empty(), "{:#?}", res.diags);
174+
}
175+
176+
#[test]
177+
fn test_enum_only_notice() {
178+
let contents = "contract Test {
179+
/// @notice An enum
180+
enum Foobar {
181+
First,
182+
Second
183+
}
184+
}";
185+
let res = parse_file(contents).validate(&OPTIONS);
186+
assert_eq!(res.diags.len(), 2);
187+
assert_eq!(res.diags[0].message, "@param First is missing");
188+
assert_eq!(res.diags[1].message, "@param Second is missing");
189+
}
190+
191+
#[test]
192+
fn test_enum_one_missing() {
193+
let contents = "contract Test {
194+
/// @notice An enum
195+
/// @param First The first
196+
enum Foobar {
197+
First,
198+
Second
199+
}
200+
}";
201+
let res = parse_file(contents).validate(&OPTIONS);
202+
assert_eq!(res.diags.len(), 1);
203+
assert_eq!(res.diags[0].message, "@param Second is missing");
204+
}
205+
206+
#[test]
207+
fn test_enum_multiline() {
208+
let contents = "contract Test {
209+
/**
210+
* @param First The first
211+
* @param Second The second
212+
*/
213+
enum Foobar {
214+
First,
215+
Second
216+
}
217+
}";
218+
let res = parse_file(contents).validate(&OPTIONS);
219+
assert!(res.diags.is_empty(), "{:#?}", res.diags);
220+
}
221+
222+
#[test]
223+
fn test_enum_duplicate() {
224+
let contents = "contract Test {
225+
/// @param First The first
226+
/// @param First The first twice
227+
enum Foobar {
228+
First
229+
}
230+
}";
231+
let res = parse_file(contents).validate(&OPTIONS);
232+
assert_eq!(res.diags.len(), 1);
233+
assert_eq!(
234+
res.diags[0].message,
235+
"@param First is present more than once"
236+
);
237+
}
238+
239+
#[test]
240+
fn test_enum_inheritdoc() {
241+
let contents = "contract Test {
242+
/// @inheritdoc
243+
enum Foobar {
244+
First
245+
}
246+
}";
247+
let res = parse_file(contents).validate(
248+
&ValidationOptions::builder()
249+
.inheritdoc(true) // has no effect for enums
250+
.constructor(false)
251+
.enum_params(true)
252+
.struct_params(false)
253+
.build(),
254+
);
255+
assert_eq!(res.diags.len(), 1);
256+
assert_eq!(res.diags[0].message, "@param First is missing");
257+
}
258+
259+
#[test]
260+
fn test_enum_no_contract() {
261+
let contents = "
262+
/// @param First The first
263+
/// @param Second The second
264+
enum Foobar {
265+
First,
266+
Second
267+
}";
268+
let res = parse_file(contents).validate(&OPTIONS);
269+
assert!(res.diags.is_empty(), "{:#?}", res.diags);
270+
}
271+
272+
#[test]
273+
fn test_enum_no_contract_missing() {
274+
let contents = "enum Foobar {
275+
First,
276+
Second
277+
}";
278+
let res = parse_file(contents).validate(&OPTIONS);
279+
assert_eq!(res.diags.len(), 1);
280+
assert_eq!(res.diags[0].message, "missing NatSpec");
281+
}
282+
283+
#[test]
284+
fn test_enum_no_contract_one_missing() {
285+
let contents = "
286+
/// @param First The first
287+
enum Foobar {
288+
First,
289+
Second
290+
}";
291+
let res = parse_file(contents).validate(&OPTIONS);
292+
assert_eq!(res.diags.len(), 1);
293+
assert_eq!(res.diags[0].message, "@param Second is missing");
294+
}
295+
}

0 commit comments

Comments
 (0)