Skip to content

Commit

Permalink
Bug fixes, bring back line numbers (#9)
Browse files Browse the repository at this point in the history 
* fix: handle functions that don't have names

* fix: properly validate script files

* feat: log line numbers for findings

* fix: tweak script rules

* fix: assume .s.sol files are scripts, other extensions are not

* fix: properly validate script interface, and log more info about invalid interface

* chore: clarify script ABI requirement
  • Loading branch information
@mds1
mds1 authored Dec 2, 2022
1 parent 8c44058 commit d3bff51
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 34 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Formatting and checking does the following:
- Uses the `taplo` crate to format TOML.
- Validates test names follow a convention of `test(Fork)?(Fuzz)?_(Revert(If_|When_){1})?\w{1,}`.
- Validates constants and immutables are in `ALL_CAPS`.
- Validates function names and visibility in forge scripts to 1 public `run` method per script.
- Validates function names and visibility in forge scripts to 1 public `run` method per script (executable script files are expected to end with `.s.sol`, whereas non-executable helper contracts in the scripts dir just end with `.sol`).
- Validates internal functions in `src/` start with a leading underscore.

## Usage
Expand Down
140 changes: 107 additions & 33 deletions src/lib.rs
View file
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 @@ -170,24 +170,32 @@ enum Validator {
}

struct InvalidItem {
// TODO Map solang `File` info to line number.
kind: Validator,
file: String, // File name.
text: String, // Incorrectly named item.
text: String, // Details to show about the invalid 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, 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 +222,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 +244,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 +290,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 All @@ -278,8 +304,6 @@ fn validate(paths: [&str; 3]) -> Result<ValidationResults, Box<dyn Error>> {
let mut results = ValidationResults::default();

for path in paths {
let is_script = path == "./script";

for result in WalkDir::new(path) {
let dent = match result {
Ok(dent) => dent,
Expand All @@ -293,32 +317,37 @@ fn validate(paths: [&str; 3]) -> Result<ValidationResults, Box<dyn Error>> {
continue
}

// Executable script files are expected to end with `.s.sol`, whereas non-executable
// helper contracts in the scripts dir just end with `.sol`.
let is_script =
path == "./script" && dent.path().to_str().expect("Bad path").ends_with(".s.sol");

// Get the parse tree (pt) of the file.
let content = fs::read_to_string(dent.path())?;
let (pt, _comments) = solang_parser::parse(&content, 0).expect("Parsing failed");

// Variables used to track status of checks that are file-wide.
let mut num_public_script_methods = 0;
let mut public_methods: Vec<String> = Vec::new();

// Run checks.
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 @@ -328,8 +357,13 @@ fn validate(paths: [&str; 3]) -> Result<ValidationResults, Box<dyn Error>> {
}
_ => false,
});
if is_script && !is_private && name != "setUp" {
num_public_script_methods += 1;

if is_script &&
!is_private &&
name != "setUp" &&
name != "constructor"
{
public_methods.push(name);
}
}
_ => (),
Expand All @@ -340,15 +374,38 @@ 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 {
results.invalid_items.push(InvalidItem {
kind: Validator::Script,
file: dent.path().display().to_string(),
text: String::new(),
});
// Validate scripts only have a single public run method, or no public methods (i.e.
// it's a helper contract not a script).
if is_script {
// If we have no public methods, the `run` method is missing.
match public_methods.len() {
0 => {
results.invalid_items.push(InvalidItem {
kind: Validator::Script,
file: dent.path().display().to_string(),
text: "No `run` method found".to_string(),
line: 0, // This spans multiple lines, so we don't have a line number.
});
}
1 => {
if public_methods[0] != "run" {
results.invalid_items.push(InvalidItem {
kind: Validator::Script,
file: dent.path().display().to_string(),
text: "The only public method must be named `run`".to_string(),
line: 0,
});
}
}
_ => {
results.invalid_items.push(InvalidItem {
kind: Validator::Script,
file: dent.path().display().to_string(),
text: format!("Scripts must have a single public method named `run` (excluding `setUp`), but the following methods were found: {public_methods:?}"),
line: 0,
});
}
}
}
}
}
Expand All @@ -365,3 +422,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")
}

0 comments on commit d3bff51

Please sign in to comment.