From 3ba41e50ff785a76fd33c0aff624e172a2101180 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Tue, 31 Oct 2023 03:08:57 +0900 Subject: [PATCH] Use [lints] in Cargo.toml and apply more lints --- .clippy.toml | 8 +++++ Cargo.toml | 35 ++++++++++++++++++++ tools/codegen/Cargo.toml | 3 ++ tools/codegen/src/main.rs | 68 +++++++++++++++++++-------------------- tools/tidy.sh | 57 +++++++++++++++++++++++--------- 5 files changed, 121 insertions(+), 50 deletions(-) diff --git a/.clippy.toml b/.clippy.toml index 85a79aaf8..27a1e0911 100644 --- a/.clippy.toml +++ b/.clippy.toml @@ -3,3 +3,11 @@ avoid-breaking-exported-api = false disallowed-names = [] +disallowed-macros = [ + { path = "std::dbg", reason = "it is okay to use during development, but please do not include it in main branch" }, +] +disallowed-methods = [ + { path = "std::env::remove_var", reason = "this is not thread-safe and inherently unsafe; see for more" }, + { path = "std::env::set_var", reason = "this is not thread-safe and inherently unsafe; see for more" }, +] +disallowed-types = [] diff --git a/Cargo.toml b/Cargo.toml index 3467e5fd5..7b9f6f5dd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,3 +1,38 @@ [workspace] resolver = "2" members = ["tools/codegen"] + +# This table is shared by projects under https://github.com/taiki-e. +# It is not intended for manual editing. +[workspace.lints.rust] +improper_ctypes = "warn" +improper_ctypes_definitions = "warn" +non_ascii_idents = "warn" +rust_2018_idioms = "warn" +single_use_lifetimes = "warn" +unreachable_pub = "warn" +unsafe_op_in_unsafe_fn = "warn" +[workspace.lints.clippy] +all = "warn" # Downgrade deny-by-default lints +pedantic = "warn" +as_ptr_cast_mut = "warn" +default_union_representation = "warn" +inline_asm_x86_att_syntax = "warn" +trailing_empty_array = "warn" +transmute_undefined_repr = "warn" +undocumented_unsafe_blocks = "warn" +# Suppress buggy or noisy clippy lints +borrow_as_ptr = { level = "allow", priority = 1 } # https://github.com/rust-lang/rust-clippy/issues/8286 +doc_markdown = { level = "allow", priority = 1 } +float_cmp = { level = "allow", priority = 1 } # https://github.com/rust-lang/rust-clippy/issues/7725 +manual_assert = { level = "allow", priority = 1 } +manual_range_contains = { level = "allow", priority = 1 } # https://github.com/rust-lang/rust-clippy/issues/6455#issuecomment-1225966395 +missing_errors_doc = { level = "allow", priority = 1 } +module_name_repetitions = { level = "allow", priority = 1 } +similar_names = { level = "allow", priority = 1 } +single_match = { level = "allow", priority = 1 } +single_match_else = { level = "allow", priority = 1 } +struct_excessive_bools = { level = "allow", priority = 1 } +too_many_arguments = { level = "allow", priority = 1 } +too_many_lines = { level = "allow", priority = 1 } +type_complexity = { level = "allow", priority = 1 } diff --git a/tools/codegen/Cargo.toml b/tools/codegen/Cargo.toml index a63ff0bce..07be3db2d 100644 --- a/tools/codegen/Cargo.toml +++ b/tools/codegen/Cargo.toml @@ -15,3 +15,6 @@ sha2 = "0.10" tar = "0.4" toml_edit = { version = "0.20", features = ["serde"] } ureq = { version = "2", features = ["json"] } + +[lints] +workspace = true diff --git a/tools/codegen/src/main.rs b/tools/codegen/src/main.rs index 3d177d6e5..96920f799 100644 --- a/tools/codegen/src/main.rs +++ b/tools/codegen/src/main.rs @@ -1,7 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![allow(clippy::single_match)] - use std::{ cmp::Reverse, collections::{BTreeMap, BTreeSet}, @@ -517,8 +515,8 @@ impl Version { major: Some(major), minor, patch: None, - pre: Default::default(), - build: Default::default(), + pre: semver::Prerelease::default(), + build: semver::BuildMetadata::default(), } } fn latest() -> Self { @@ -526,8 +524,8 @@ impl Version { major: None, minor: None, patch: None, - pre: Default::default(), - build: Default::default(), + pre: semver::Prerelease::default(), + build: semver::BuildMetadata::default(), } } fn to_semver(&self) -> Option { @@ -611,8 +609,8 @@ impl FromStr for Version { major: Some(v.major), minor: v.minor, patch: v.patch, - pre: Default::default(), - build: Default::default(), + pre: semver::Prerelease::default(), + build: semver::BuildMetadata::default(), }), Err(_e) => Err(e), }, @@ -806,21 +804,21 @@ mod github { use serde_derive::Deserialize; // https://api.github.com/repos//releases - pub type Releases = Vec; + pub(crate) type Releases = Vec; // https://api.github.com/repos//releases/ #[derive(Debug, Deserialize)] - pub struct Release { - pub tag_name: String, - pub prerelease: bool, - pub assets: Vec, + pub(crate) struct Release { + pub(crate) tag_name: String, + pub(crate) prerelease: bool, + pub(crate) assets: Vec, } #[derive(Debug, Deserialize)] - pub struct ReleaseAsset { - pub name: String, - pub content_type: String, - pub browser_download_url: String, + pub(crate) struct ReleaseAsset { + pub(crate) name: String, + // pub(crate) content_type: String, + pub(crate) browser_download_url: String, } } @@ -829,16 +827,16 @@ mod crates_io { // https://crates.io/api/v1/crates/ #[derive(Debug, Deserialize)] - pub struct Crate { - pub versions: Vec, + pub(crate) struct Crate { + pub(crate) versions: Vec, } #[derive(Debug, Deserialize)] - pub struct Version { - pub checksum: String, - pub dl_path: String, - pub num: semver::Version, - pub yanked: bool, + pub(crate) struct Version { + pub(crate) checksum: String, + pub(crate) dl_path: String, + pub(crate) num: semver::Version, + pub(crate) yanked: bool, } } @@ -846,28 +844,28 @@ mod cargo_manifest { use serde_derive::Deserialize; #[derive(Debug, Deserialize)] - pub struct Manifest { - pub package: Package, + pub(crate) struct Manifest { + pub(crate) package: Package, } #[derive(Debug, Deserialize)] - pub struct Package { - pub metadata: Metadata, + pub(crate) struct Package { + pub(crate) metadata: Metadata, } #[derive(Debug, Deserialize)] - pub struct Metadata { - pub binstall: Binstall, + pub(crate) struct Metadata { + pub(crate) binstall: Binstall, } #[derive(Debug, Deserialize)] - pub struct Binstall { - pub signing: BinstallSigning, + pub(crate) struct Binstall { + pub(crate) signing: BinstallSigning, } #[derive(Debug, Deserialize)] - pub struct BinstallSigning { - pub algorithm: String, - pub pubkey: String, + pub(crate) struct BinstallSigning { + pub(crate) algorithm: String, + pub(crate) pubkey: String, } } diff --git a/tools/tidy.sh b/tools/tidy.sh index 7e8ce5ae1..8b6a2aaaa 100755 --- a/tools/tidy.sh +++ b/tools/tidy.sh @@ -65,6 +65,9 @@ fi # Rust (if exists) if [[ -n "$(git ls-files '*.rs')" ]]; then info "checking Rust code style" + if [[ ! -e .rustfmt.toml ]]; then + warn "could not found .rustfmt.toml in the repository root" + fi if type -P rustup &>/dev/null; then # `cargo fmt` cannot recognize files not included in the current workspace and modules # defined inside macros, so run rustfmt directly. @@ -119,6 +122,10 @@ if [[ -n "$(git ls-files '*.rs')" ]]; then for id in $(jq <<<"${metadata}" '.workspace_members[]'); do pkg=$(jq <<<"${metadata}" ".packages[] | select(.id == ${id})") publish=$(jq <<<"${pkg}" -r '.publish') + manifest_path=$(jq <<<"${pkg}" -r '.manifest_path') + if ! grep -q '^\[lints\]' "${manifest_path}"; then + warn "no [lints] table in ${manifest_path} please add '[lints]' with 'workspace = true'" + fi # Publishing is unrestricted if null, and forbidden if an empty array. if [[ "${publish}" == "[]" ]]; then continue @@ -127,11 +134,19 @@ if [[ -n "$(git ls-files '*.rs')" ]]; then done if [[ -n "${has_public_crate}" ]]; then info "checking file permissions" - if [[ -f Cargo.toml ]] && grep -Eq '^\[package\]' Cargo.toml && ! grep -Eq '^publish = false' Cargo.toml; then - if ! grep -Eq '^exclude = \[.*\.\*.*\]' Cargo.toml; then - error "top-level Cargo.toml of real manifest should have exclude field with \"/.*\" and \"/tools\"" - elif ! grep -Eq '^exclude = \[.*/tools.*\]' Cargo.toml; then - error "top-level Cargo.toml of real manifest should have exclude field with \"/.*\" and \"/tools\"" + if [[ -f Cargo.toml ]]; then + root_manifest=$(cargo locate-project --message-format=plain --manifest-path Cargo.toml) + root_pkg=$(jq <<<"${metadata}" ".packages[] | select(.manifest_path == \"${root_manifest}\")") + if [[ -n "${root_pkg}" ]]; then + publish=$(jq <<<"${root_pkg}" -r '.publish') + # Publishing is unrestricted if null, and forbidden if an empty array. + if [[ "${publish}" != "[]" ]]; then + if ! grep -Eq '^exclude = \[.*\.\*.*\]' Cargo.toml; then + error "top-level Cargo.toml of real manifest should have exclude field with \"/.*\" and \"/tools\"" + elif ! grep -Eq '^exclude = \[.*/tools.*\]' Cargo.toml; then + error "top-level Cargo.toml of real manifest should have exclude field with \"/.*\" and \"/tools\"" + fi + fi fi fi for p in $(git ls-files); do @@ -158,27 +173,30 @@ if [[ -n "$(git ls-files '*.rs')" ]]; then fi # C/C++ (if exists) -if [[ -n "$(git ls-files '*.c')$(git ls-files '*.cpp')" ]]; then +if [[ -n "$(git ls-files '*.c' '*.h' '*.cpp' '*.hpp')" ]]; then info "checking C/C++ code style" if [[ ! -e .clang-format ]]; then - warn "could not fount .clang-format in the repository root" + warn "could not found .clang-format in the repository root" fi if type -P clang-format &>/dev/null; then - echo "+ clang-format -i \$(git ls-files '*.c') \$(git ls-files '*.cpp')" - clang-format -i $(git ls-files '*.c') $(git ls-files '*.cpp') - check_diff $(git ls-files '*.c') $(git ls-files '*.cpp') + echo "+ clang-format -i \$(git ls-files '*.c' '*.h' '*.cpp' '*.hpp')" + clang-format -i $(git ls-files '*.c' '*.h' '*.cpp' '*.hpp') + check_diff $(git ls-files '*.c' '*.h' '*.cpp' '*.hpp') else warn "'clang-format' is not installed; skipped C/C++ code style check" fi fi # YAML/JavaScript/JSON (if exists) -if [[ -n "$(git ls-files '*.yml')$(git ls-files '*.js')$(git ls-files '*.json')" ]]; then +if [[ -n "$(git ls-files '*.yml' '*.js' '*.json')" ]]; then info "checking YAML/JavaScript/JSON code style" + if [[ ! -e .editorconfig ]]; then + warn "could not found .editorconfig in the repository root" + fi if type -P npm &>/dev/null; then - echo "+ npx -y prettier -l -w \$(git ls-files '*.yml') \$(git ls-files '*.js') \$(git ls-files '*.json')" - npx -y prettier -l -w $(git ls-files '*.yml') $(git ls-files '*.js') $(git ls-files '*.json') - check_diff $(git ls-files '*.yml') $(git ls-files '*.js') $(git ls-files '*.json') + echo "+ npx -y prettier -l -w \$(git ls-files '*.yml' '*.js' '*.json')" + npx -y prettier -l -w $(git ls-files '*.yml' '*.js' '*.json') + check_diff $(git ls-files '*.yml' '*.js' '*.json') else warn "'npm' is not installed; skipped YAML/JavaScript/JSON code style check" fi @@ -188,7 +206,7 @@ if [[ -n "$(git ls-files '*.yml')$(git ls-files '*.js')$(git ls-files '*.json')" if type -P jq &>/dev/null && type -P yq &>/dev/null; then for workflow in .github/workflows/*.yml; do # The top-level permissions must be weak as they are referenced by all jobs. - permissions=$(yq '.permissions' "${workflow}" | jq -c) + permissions=$(yq -c '.permissions' "${workflow}") case "${permissions}" in '{"contents":"read"}' | '{"contents":"none"}') ;; null) error "${workflow}: top level permissions not found; it must be 'contents: read' or weaker permissions" ;; @@ -222,6 +240,9 @@ fi # Markdown (if exists) if [[ -n "$(git ls-files '*.md')" ]]; then info "checking Markdown style" + if [[ ! -e .markdownlint.yml ]]; then + warn "could not found .markdownlint.yml in the repository root" + fi if type -P npm &>/dev/null; then echo "+ npx -y markdownlint-cli2 \$(git ls-files '*.md')" npx -y markdownlint-cli2 $(git ls-files '*.md') @@ -237,6 +258,9 @@ fi # Shell scripts info "checking Shell scripts" if type -P shfmt &>/dev/null; then + if [[ ! -e .editorconfig ]]; then + warn "could not found .editorconfig in the repository root" + fi echo "+ shfmt -l -w \$(git ls-files '*.sh')" shfmt -l -w $(git ls-files '*.sh') check_diff $(git ls-files '*.sh') @@ -244,6 +268,9 @@ else warn "'shfmt' is not installed; skipped Shell scripts style check" fi if type -P shellcheck &>/dev/null; then + if [[ ! -e .shellcheckrc ]]; then + warn "could not found .shellcheckrc in the repository root" + fi echo "+ shellcheck \$(git ls-files '*.sh')" if ! shellcheck $(git ls-files '*.sh'); then should_fail=1