Skip to content

Bug fixes, bring back line numbers #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mds1 merged 7 commits into from
Dec 2, 2022
Merged

Bug fixes, bring back line numbers #9

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3a5a3f8
fix: handle functions that don't have names
mds1 Nov 16, 2022
1a8d478
fix: properly validate script files
mds1 Nov 16, 2022
ba9fd20
feat: log line numbers for findings
mds1 Nov 16, 2022
910998b
fix: tweak script rules
mds1 Nov 16, 2022
dafa93e
fix: assume .s.sol files are scripts, other extensions are not
mds1 Nov 23, 2022
ae52540
fix: properly validate script interface, and log more info about inva…
mds1 Nov 24, 2022
ad20dae
chore: clarify script ABI requirement
mds1 Dec 2, 2022
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
77 changes: 60 additions & 17 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use once_cell::sync::Lazy;

use regex::Regex;
use solang_parser::pt::{
ContractPart, FunctionAttribute, FunctionDefinition, SourceUnitPart, VariableAttribute,
VariableDefinition, Visibility,
ContractPart, FunctionAttribute, FunctionDefinition, FunctionTy, SourceUnitPart,
VariableAttribute, VariableDefinition, Visibility,
};
use std::{error::Error, ffi::OsStr, fmt, fs, process};
use walkdir::{DirEntry, WalkDir};
Expand Down Expand Up @@ -174,20 +174,27 @@ struct InvalidItem {
kind: Validator,
file: String, // File name.
text: String, // Incorrectly named item.
line: usize, // Line number.
}

impl InvalidItem {
fn description(&self) -> String {
match self.kind {
Validator::Test => {
format!("Invalid test name in {}: {}", self.file, self.text)
format!("Invalid test name in {} on line {}: {}", self.file, self.line, self.text)
}
Validator::Constant => {
format!("Invalid constant or immutable name in {}: {}", self.file, self.text)
format!(
"Invalid constant or immutable name in {} on line {}: {}",
self.file, self.line, self.text
)
}
Validator::Script => format!("Invalid script interface in {}", self.file),
Validator::Src => {
format!("Invalid src method name in {}: {}", self.file, self.text)
format!(
"Invalid src method name in {} on line {}: {}",
self.file, self.line, self.text
)
}
}
}
Expand All @@ -214,11 +221,15 @@ impl ValidationResults {
}

trait Validate {
fn validate(&self, dent: &DirEntry) -> Vec<InvalidItem>;
fn validate(&self, content: &str, dent: &DirEntry) -> Vec<InvalidItem>;
}

trait Name {
fn name(&self) -> String;
}

impl Validate for VariableDefinition {
fn validate(&self, dent: &DirEntry) -> Vec<InvalidItem> {
fn validate(&self, content: &str, dent: &DirEntry) -> Vec<InvalidItem> {
let mut invalid_items = Vec::new();
let name = &self.name.name;

Expand All @@ -232,24 +243,37 @@ impl Validate for VariableDefinition {
kind: Validator::Constant,
file: dent.path().display().to_string(),
text: name.clone(),
line: offset_to_line(content, self.loc.start()),
});
}

invalid_items
}
}

impl Name for FunctionDefinition {
fn name(&self) -> String {
match self.ty {
FunctionTy::Constructor => "constructor".to_string(),
FunctionTy::Fallback => "fallback".to_string(),
FunctionTy::Receive => "receive".to_string(),
FunctionTy::Function | FunctionTy::Modifier => self.name.as_ref().unwrap().name.clone(),
}
}
}

impl Validate for FunctionDefinition {
fn validate(&self, dent: &DirEntry) -> Vec<InvalidItem> {
fn validate(&self, content: &str, dent: &DirEntry) -> Vec<InvalidItem> {
let mut invalid_items = Vec::new();
let name = &self.name.as_ref().unwrap().name;
let name = &self.name();

// Validate test names match the required pattern.
if dent.path().starts_with("./test") && !is_valid_test_name(name) {
invalid_items.push(InvalidItem {
kind: Validator::Test,
file: dent.path().display().to_string(),
text: name.clone(),
text: name.to_string(),
line: offset_to_line(content, self.loc.start()),
});
}

Expand All @@ -265,7 +289,8 @@ impl Validate for FunctionDefinition {
invalid_items.push(InvalidItem {
kind: Validator::Src,
file: dent.path().display().to_string(),
text: name.clone(),
text: name.to_string(),
line: offset_to_line(content, self.loc.start()),
});
}

Expand Down Expand Up @@ -304,21 +329,21 @@ fn validate(paths: [&str; 3]) -> Result<ValidationResults, Box<dyn Error>> {
for element in pt.0 {
match element {
SourceUnitPart::FunctionDefinition(f) => {
results.invalid_items.extend(f.validate(&dent));
results.invalid_items.extend(f.validate(&content, &dent));
}
SourceUnitPart::VariableDefinition(v) => {
results.invalid_items.extend(v.validate(&dent));
results.invalid_items.extend(v.validate(&content, &dent));
}
SourceUnitPart::ContractDefinition(c) => {
for el in c.parts {
match el {
ContractPart::VariableDefinition(v) => {
results.invalid_items.extend(v.validate(&dent));
results.invalid_items.extend(v.validate(&content, &dent));
}
ContractPart::FunctionDefinition(f) => {
results.invalid_items.extend(f.validate(&dent));
results.invalid_items.extend(f.validate(&content, &dent));

let name = f.name.unwrap().name;
let name = f.name();
let is_private = f.attributes.iter().any(|a| match a {
FunctionAttribute::Visibility(v) => {
matches!(
Expand All @@ -343,11 +368,12 @@ fn validate(paths: [&str; 3]) -> Result<ValidationResults, Box<dyn Error>> {
// Validate scripts only have a single run method.
// TODO Script checks don't really fit nicely into InvalidItem, refactor needed to log
// more details about the invalid script's ABI.
if num_public_script_methods > 1 {
if is_script && num_public_script_methods != 1 {
results.invalid_items.push(InvalidItem {
kind: Validator::Script,
file: dent.path().display().to_string(),
text: String::new(),
line: 0, // This spans multiple lines, so we don't have a line number.
});
}
}
Expand All @@ -365,3 +391,20 @@ fn is_valid_test_name(name: &str) -> bool {
fn is_valid_constant_name(name: &str) -> bool {
RE_VALID_CONSTANT_NAME.is_match(name)
}

// Converts the start offset of a `Loc` to `(line, col)`. Modified from https://github.com/foundry-rs/foundry/blob/45b9dccdc8584fb5fbf55eb190a880d4e3b0753f/fmt/src/helpers.rs#L54-L70
fn offset_to_line(content: &str, start: usize) -> usize {
debug_assert!(content.len() > start);

let mut line_counter = 1; // First line is `1`.
for (offset, c) in content.chars().enumerate() {
if c == '\n' {
line_counter += 1;
}
if offset > start {
return line_counter
}
}

unreachable!("content.len() > start")
}