Skip to content
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

Jump to bottom

lint: add better rust formatting #72

Merged
merged 3 commits into from
Jul 6, 2024
Merged

lint: add better rust formatting #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
425569b
lint: add better rust formatting
alexfertel Jul 6, 2024
9d481c9
Merge branch 'main' into add-rustfmt-rules
alexfertel Jul 6, 2024
e3b32ae
lint: relax clippy checks a bit
alexfertel Jul 6, 2024
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
22 changes: 18 additions & 4 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,23 @@ jobs:
command: build
args: --all-targets

lint:
fmt:
runs-on: ubuntu-latest
name: nightly / fmt
steps:
- uses: actions/checkout@v4
with:
submodules: true
- name: Install stable
# We run in nightly to make use of some features only available there.
# Check out `rustfmt.toml` to see which ones.
uses: dtolnay/rust-toolchain@nightly
with:
components: rustfmt
- name: cargo fmt --all --check
run: cargo fmt --all --check

clippy:
runs-on: ubuntu-latest
permissions:
checks: write
Expand All @@ -46,9 +62,7 @@ jobs:
profile: minimal
toolchain: stable
override: true
components: rustfmt, clippy
- name: Check code formatting
run: cargo fmt -- --check
components: clippy
- name: Run Clippy
uses: actions-rs/clippy-check@v1
with:
Expand Down
13 changes: 8 additions & 5 deletions .rustfmt.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# This file intentionally left almost blank
#
# The empty `rustfmt.toml` makes rustfmt use the default configuration,
# overriding any which may be found in the contributor's home or parent
# folders.
max_width = 80
format_macro_matchers = true
group_imports = "StdExternalCrate"
imports_granularity = "Crate"
reorder_impl_items = true
use_field_init_shorthand = true
use_small_heuristics = "Max"
wrap_comments = true
3 changes: 2 additions & 1 deletion benches/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use bulloak::{self, scaffold};
use criterion::{black_box, criterion_group, criterion_main, Criterion};

fn big_tree(c: &mut Criterion) {
let tree = std::fs::read_to_string("benches/bench_data/cancel.tree").unwrap();
let tree =
std::fs::read_to_string("benches/bench_data/cancel.tree").unwrap();

let cfg = Default::default();
let mut group = c.benchmark_group("sample-size-10");
Expand Down
69 changes: 39 additions & 30 deletions src/check/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
//! Defines the context in which rule-checking occurs.

use forge_fmt::{format, Comments, FormatterConfig, FormatterError, InlineConfig, Parsed};
use solang_parser::pt::SourceUnit;
use std::{
ffi::OsStr,
fs,
path::{Path, PathBuf},
};

use crate::{check::Violation, scaffold::emitter::Emitter};
use forge_fmt::{
format, Comments, FormatterConfig, FormatterError, InlineConfig, Parsed,
};
use solang_parser::pt::SourceUnit;

use super::{location::Location, violation::ViolationKind};
use crate::{
check::Violation,
config::Config,
hir::{self, Hir},
scaffold::emitter::Emitter,
};

use super::{location::Location, violation::ViolationKind};

/// The context in which rule-checking happens.
///
/// This is a utility struct that abstracts away the requirements for a `check`
Expand Down Expand Up @@ -47,42 +50,37 @@
///
/// This structure contains everything necessary to perform checks between
/// trees and Solidity files.
pub(crate) fn new(tree: PathBuf, cfg: Config) -> Result<Self, Violation> {

Check warning on line 53 in src/check/context.rs

View workflow job for this annotation

GitHub Actions / clippy

this argument is passed by value, but not consumed in the function body 

warning: this argument is passed by value, but not consumed in the function body
  --> src/check/context.rs:53:43
   |
53 |     pub(crate) fn new(tree: PathBuf, cfg: Config) -> Result<Self, Violation> {
   |                                           ^^^^^^ help: consider taking a reference instead: `&Config`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
   = note: `#[warn(clippy::needless_pass_by_value)]` implied by `#[warn(clippy::pedantic)]`
let tree_path_cow = tree.to_string_lossy();
let tree_contents = try_read_to_string(&tree)?;
let hir = crate::hir::translate(&tree_contents, &Default::default()).map_err(|e| {
Violation::new(
ViolationKind::ParsingFailed(e),
Location::File(tree_path_cow.into_owned()),
)
})?;
let hir = crate::hir::translate(&tree_contents, &Config::default())
.map_err(|e| {
Violation::new(
ViolationKind::ParsingFailed(e),
Location::File(tree_path_cow.into_owned()),
)
})?;

let sol = get_path_with_ext(&tree, "t.sol")?;
let src = try_read_to_string(&sol)?;
let parsed = forge_fmt::parse(&src).map_err(|_| {
let sol_filename = sol.to_string_lossy().into_owned();
Violation::new(
ViolationKind::ParsingFailed(anyhow::anyhow!("Failed to parse {sol_filename}")),
ViolationKind::ParsingFailed(anyhow::anyhow!(
"Failed to parse {sol_filename}"
)),
Location::File(sol_filename),
)
})?;

let pt = parsed.pt.clone();
let comments = parsed.comments;
Ok(Context {
tree,
hir,
sol,
src,
pt,
comments,
cfg: cfg.clone(),
})
Ok(Context { tree, hir, sol, src, pt, comments, cfg: cfg.clone() })
}

/// Updates this `Context` with the result of parsing a Solidity file.
#[inline]
pub(crate) fn from_parsed(mut self, parsed: Parsed) -> Self {

Check warning on line 83 in src/check/context.rs

View workflow job for this annotation

GitHub Actions / clippy

methods called `from_*` usually take no `self` 

warning: methods called `from_*` usually take no `self`
  --> src/check/context.rs:83:31
   |
83 |     pub(crate) fn from_parsed(mut self, parsed: Parsed) -> Self {
   |                               ^^^^^^^^
   |
   = help: consider choosing a less ambiguous name
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
note: the lint level is defined here
  --> src/lib.rs:3:9
   |
3  | #![warn(clippy::all, clippy::pedantic, clippy::cargo)]
   |         ^^^^^^^^^^^
   = note: `#[warn(clippy::wrong_self_convention)]` implied by `#[warn(clippy::all)]`
parsed.src.clone_into(&mut self.src);
self.pt = parsed.pt;
self.comments = parsed.comments;
Expand All @@ -108,17 +106,25 @@
Ok(formatted)
}

/// Inserts a function definition into the source string at a specified offset.
/// Inserts a function definition into the source string at a specified
/// offset.
///
/// This function takes a `FunctionDefinition` from the High-Level Intermediate Representation (HIR),
/// converts it into a Solidity function definition string using an `Emitter`, and then inserts
/// this string into the specified source code at a given offset.
/// This function takes a `FunctionDefinition` from the High-Level
/// Intermediate Representation (HIR), converts it into a Solidity
/// function definition string using an `Emitter`, and then inserts this
/// string into the specified source code at a given offset.
///
/// # Arguments
/// * `function` - A reference to the HIR `FunctionDefinition` to be inserted.
/// * `offset` - The character position in the source string where the function definition should be inserted.
pub(crate) fn insert_function_at(&mut self, function: &hir::FunctionDefinition, offset: usize) {
let cfg = &Default::default();
/// * `function` - A reference to the HIR `FunctionDefinition` to be
/// inserted.
/// * `offset` - The character position in the source string where the
/// function definition should be inserted.
pub(crate) fn insert_function_at(
&mut self,
function: &hir::FunctionDefinition,
offset: usize,
) {
let cfg = &Config::default();
let f = &Hir::FunctionDefinition(function.clone());
let function = Emitter::new(cfg).emit(f);
self.src = format!(
Expand All @@ -130,7 +136,10 @@
}
}

fn get_path_with_ext(path: impl AsRef<Path>, ext: impl AsRef<OsStr>) -> Result<PathBuf, Violation> {
fn get_path_with_ext(
path: impl AsRef<Path>,
ext: impl AsRef<OsStr>,
) -> Result<PathBuf, Violation> {
let path = path.as_ref();
let mut sol = path.to_path_buf();
sol.set_extension(ext);
Expand Down
55 changes: 35 additions & 20 deletions src/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,20 @@
//! Defines the `bulloak check` command.
//!
//! This command performs checks on the relationship between a bulloak tree and a
//! Solidity file.
//! This command performs checks on the relationship between a bulloak tree and
//! a Solidity file.

use std::fs;
use std::path::PathBuf;
use std::{fs, path::PathBuf};

use clap::Parser;
use owo_colors::OwoColorize;
use serde::{Deserialize, Serialize};
use violation::{Violation, ViolationKind};

use crate::check::violation::fix_order;
use crate::config::Config;
use crate::sol::find_contract;
use crate::utils::pluralize;

use self::context::Context;
use self::rules::Checker;
use self::{context::Context, rules::Checker};
use crate::{
check::violation::fix_order, config::Config, sol::find_contract,
utils::pluralize,
};

mod context;
mod location;
Expand Down Expand Up @@ -55,70 +52,87 @@
/// Entrypoint for `bulloak check`.
///
/// Note that we don't deal with `solang_parser` errors at all.
pub(crate) fn run(&self, cfg: &Config) -> anyhow::Result<()> {
let mut violations = Vec::new();
let ctxs: Vec<Context> = self
.files
.iter()
.filter_map(|tree_path| {
Context::new(tree_path.clone(), cfg.clone())
.map_err(|violation| violations.push(violation))
.ok()
})
.collect();

if self.fix {
let mut fixed_count = 0;
for mut ctx in ctxs {
let violations = rules::StructuralMatcher::check(&ctx);
let fixable_count = violations.iter().filter(|v| v.is_fixable()).count();
let fixable_count =
violations.iter().filter(|v| v.is_fixable()).count();

// Process violations that affect function order first.
let violations = violations
.into_iter()
.filter(|v| !matches!(v.kind, ViolationKind::FunctionOrderMismatch(_, _, _)));
let violations = violations.into_iter().filter(|v| {
!matches!(
v.kind,
ViolationKind::FunctionOrderMismatch(_, _, _)
)
});
for violation in violations {
ctx = violation.kind.fix(ctx);
}

// Second pass fixing order violations.
let violations = rules::StructuralMatcher::check(&ctx);
let violations: Vec<Violation> = violations
.into_iter()
.filter(|v| matches!(v.kind, ViolationKind::FunctionOrderMismatch(_, _, _)))
.filter(|v| {
matches!(
v.kind,
ViolationKind::FunctionOrderMismatch(_, _, _)
)
})
.collect();
if !violations.is_empty() {
if let Some(contract_sol) = find_contract(&ctx.pt) {
if let Some(contract_hir) = ctx.hir.clone().find_contract() {
ctx = fix_order(&violations, &contract_sol, contract_hir, ctx);
if let Some(contract_hir) =
ctx.hir.clone().find_contract()
{
ctx = fix_order(
&violations,
&contract_sol,
contract_hir,
ctx,
);
}
}
}

let sol = ctx.sol.clone();
let formatted = ctx.fmt().expect("should format the emitted solidity code");
let formatted =
ctx.fmt().expect("should format the emitted solidity code");
self.write(&formatted, sol);

fixed_count += fixable_count;
}

let issue_literal = pluralize(fixed_count, "issue", "issues");
println!(
"\n{}: {} {} fixed.",
"success".bold().green(),
fixed_count,
issue_literal
);
} else {
for ctx in ctxs {
violations.append(&mut rules::StructuralMatcher::check(&ctx));
}

exit(&violations);
}

Ok(())
}

Check warning on line 135 in src/check/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

this function's return value is unnecessary 

warning: this function's return value is unnecessary
   --> src/check/mod.rs:55:5
    |
55  | /     pub(crate) fn run(&self, cfg: &Config) -> anyhow::Result<()> {
56  | |         let mut violations = Vec::new();
57  | |         let ctxs: Vec<Context> = self
58  | |             .files
...   |
134 | |         Ok(())
135 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
    = note: `#[warn(clippy::unnecessary_wraps)]` implied by `#[warn(clippy::pedantic)]`
help: remove the return type...
    |
55  |     pub(crate) fn run(&self, cfg: &Config) -> anyhow::Result<()> {
    |                                               ~~~~~~~~~~~~~~~~~~
help: ...and then remove returned values
    |
134 -         Ok(())
134 +         
    |

/// Handles writing the output of the `check` command.
///
Expand Down Expand Up @@ -153,7 +167,8 @@
violations.len(),
check_literal
);
let fixable_count = violations.iter().filter(|v| v.is_fixable()).count();
let fixable_count =
violations.iter().filter(|v| v.is_fixable()).count();
if fixable_count > 0 {
let fix_literal = pluralize(fixable_count, "fix", "fixes");
eprintln!(
Expand Down
49 changes: 36 additions & 13 deletions src/check/rules/structural_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
//!
//! This rule enforces the following:
//! - All spec-generated functions & modifiers are present in the output file.
//! - The order of the spec-generated functions & modifiers matches the output file.
//! - The order of the spec-generated functions & modifiers matches the output
//! file.
//!
//! Matching is name-based, which means that two functions are considered the
//! same if:
Expand All @@ -14,6 +15,7 @@

use solang_parser::pt;

use super::{Checker, Context};
use crate::{
check::{
location::Location,
Expand All @@ -25,8 +27,6 @@
utils::sanitize,
};

use super::{Checker, Context};

/// An implementation of a structural matching rule.
///
/// Read more at the [module-level documentation].
Expand Down Expand Up @@ -72,8 +72,16 @@
let contract_hir = contract_hir.unwrap();
let contract_sol = contract_sol.unwrap();
if let Hir::ContractDefinition(contract_hir) = contract_hir {
violations.append(&mut check_contract_names(contract_hir, &contract_sol, ctx));
violations.append(&mut check_fns_structure(contract_hir, &contract_sol, ctx));
violations.append(&mut check_contract_names(
contract_hir,
&contract_sol,
ctx,
));
violations.append(&mut check_fns_structure(
contract_hir,
&contract_sol,
ctx,
));
};

violations
Expand All @@ -93,7 +101,10 @@
let contract_name = sanitize(&contract_hir.identifier);
if identifier.name != contract_name {
let violation = Violation::new(
ViolationKind::ContractNameNotMatches(contract_name, identifier.name.clone()),
ViolationKind::ContractNameNotMatches(
contract_name,
identifier.name.clone(),
),
Location::Code(
ctx.sol.as_path().to_string_lossy().into_owned(),
offset_to_line(&ctx.src, contract_sol.loc.start()),
Expand All @@ -107,8 +118,8 @@
}

/// Checks that function structures match between the HIR and the Solidity AST.
/// i.e. that all the functions are present in the output file in the right order.
/// This could be better, currently it is O(N^2).
/// i.e. that all the functions are present in the output file in the right
/// order. This could be better, currently it is O(N^2).
fn check_fns_structure(
contract_hir: &hir::ContractDefinition,
contract_sol: &pt::ContractDefinition,
Expand All @@ -118,26 +129,32 @@

// Check that hir functions are present in the solidity contract. Store
// their indices for later processing.
let mut present_fn_indices = Vec::with_capacity(contract_hir.children.len());
let mut present_fn_indices =
Vec::with_capacity(contract_hir.children.len());
for (hir_idx, fn_hir) in contract_hir.children.iter().enumerate() {
if let Hir::FunctionDefinition(fn_hir) = fn_hir {
let fn_sol = find_matching_fn(contract_sol, fn_hir);

match fn_sol {
// Store the matched function to check it is at the
// appropriate place later.
Some((sol_idx, _)) => present_fn_indices.push((hir_idx, sol_idx)),
Some((sol_idx, _)) => {
present_fn_indices.push((hir_idx, sol_idx))

Check warning on line 142 in src/check/rules/structural_match.rs

View workflow job for this annotation

GitHub Actions / clippy

consider adding a `;` to the last statement for consistent formatting 

warning: consider adding a `;` to the last statement for consistent formatting
   --> src/check/rules/structural_match.rs:142:21
    |
142 |                     present_fn_indices.push((hir_idx, sol_idx))
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add a `;` here: `present_fn_indices.push((hir_idx, sol_idx));`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned
    = note: `#[warn(clippy::semicolon_if_nothing_returned)]` implied by `#[warn(clippy::pedantic)]`
}
// We didn't find a matching function, so this is a
// violation.
None => {
if !ctx.cfg.check().skip_modifiers {
violations.push(Violation::new(
ViolationKind::MatchingFunctionMissing(fn_hir.clone(), hir_idx),
ViolationKind::MatchingFunctionMissing(
fn_hir.clone(),
hir_idx,
),
Location::Code(
ctx.tree.to_string_lossy().into_owned(),
fn_hir.span.start.line,
),
))

Check warning on line 157 in src/check/rules/structural_match.rs

View workflow job for this annotation

GitHub Actions / clippy

consider adding a `;` to the last statement for consistent formatting 

warning: consider adding a `;` to the last statement for consistent formatting
   --> src/check/rules/structural_match.rs:148:25
    |
148 | /                         violations.push(Violation::new(
149 | |                             ViolationKind::MatchingFunctionMissing(
150 | |                                 fn_hir.clone(),
151 | |                                 hir_idx,
...   |
156 | |                             ),
157 | |                         ))
    | |__________________________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned
help: add a `;` here
    |
148 ~                         violations.push(Violation::new(
149 +                             ViolationKind::MatchingFunctionMissing(
150 +                                 fn_hir.clone(),
151 +                                 hir_idx,
152 +                             ),
153 +                             Location::Code(
154 +                                 ctx.tree.to_string_lossy().into_owned(),
155 +                                 fn_hir.span.start.line,
156 +                             ),
157 +                         ));
    |
}
}
}
Expand All @@ -157,7 +174,7 @@
// Everything that's less than the ith item is unsorted.
// If there is at least one element that is less than the
// ith item, then, this element is also unsorted.
for j in i + 1..present_fn_indices.len() {

Check warning on line 177 in src/check/rules/structural_match.rs

View workflow job for this annotation

GitHub Actions / clippy

the loop variable `j` is only used to index `present_fn_indices` 

warning: the loop variable `j` is only used to index `present_fn_indices`
   --> src/check/rules/structural_match.rs:177:18
    |
177 |         for j in i + 1..present_fn_indices.len() {
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
    = note: `#[warn(clippy::needless_range_loop)]` implied by `#[warn(clippy::all)]`
help: consider using an iterator
    |
177 |         for <item> in present_fn_indices.iter().skip(i + 1) {
    |             ~~~~~~    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
let (_, j_sol_idx) = present_fn_indices[j];
// We found an inversion.
if i_sol_idx > j_sol_idx {
Expand All @@ -169,9 +186,15 @@
// Emit a violation per unsorted item.
for (hir_idx, sol_idx) in unsorted_set {
if let Hir::FunctionDefinition(_) = contract_hir.children[hir_idx] {
if let pt::ContractPart::FunctionDefinition(ref fn_sol) = contract_sol.parts[sol_idx] {
if let pt::ContractPart::FunctionDefinition(ref fn_sol) =
contract_sol.parts[sol_idx]
{
violations.push(Violation::new(
ViolationKind::FunctionOrderMismatch(*fn_sol.clone(), sol_idx, hir_idx),
ViolationKind::FunctionOrderMismatch(
*fn_sol.clone(),
sol_idx,
hir_idx,
),
Location::Code(
ctx.sol.clone().to_string_lossy().into_owned(),
offset_to_line(&ctx.src, fn_sol.loc.start()),
Expand Down
Loading
Loading