From cbc62d2ea8af2797168a68c36556cc19d38b69c2 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Tue, 31 Oct 2023 03:44:38 +0900 Subject: [PATCH] Use [lints] in Cargo.toml and apply more lints --- .clippy.toml | 8 ++++ Cargo.toml | 48 +++++++++++++++++++--- futures-async-stream-macro/Cargo.toml | 3 ++ futures-async-stream-macro/src/lib.rs | 1 - src/lib.rs | 15 ------- tests/compiletest.rs | 7 ---- tests/elisions.rs | 1 - tests/for_await.rs | 1 - tests/no-std/Cargo.toml | 3 ++ tests/no-std/lib.rs | 2 +- tests/overwriting_core_crate.rs | 2 +- tests/stream.rs | 8 +++- tests/try_stream.rs | 5 +-- tools/codegen/Cargo.toml | 3 ++ tools/codegen/src/file.rs | 18 ++++++--- tools/codegen/src/main.rs | 33 +++++++--------- tools/tidy.sh | 57 ++++++++++++++++++++------- 17 files changed, 139 insertions(+), 76 deletions(-) diff --git a/.clippy.toml b/.clippy.toml index 8bcd6a7..99fb46d 100644 --- a/.clippy.toml +++ b/.clippy.toml @@ -3,4 +3,12 @@ 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 = [] msrv = "1.74" # nightly-2023-10-21 is 1.75 but not full 1.75 since it is nightly. diff --git a/Cargo.toml b/Cargo.toml index 69bf07b..6d0d1d9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,10 +20,6 @@ allowed_external_types = [ "futures_async_stream_macro::*", ] -[workspace] -resolver = "2" -members = ["futures-async-stream-macro", "tests/no-std", "tools/codegen"] - [lib] doc-scrape-examples = false @@ -37,4 +33,46 @@ pin-project = "1.0.11" futures = { package = "futures-util", version = "0.3", default-features = false } rustversion = "1" static_assertions = "1" -trybuild = "1.0.49" +trybuild = { git = "https://github.com/taiki-e/trybuild.git", branch = "dev" } # adjust overwrite behavior + +[lints] +workspace = true + +[workspace] +resolver = "2" +members = ["futures-async-stream-macro", "tests/no-std", "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/futures-async-stream-macro/Cargo.toml b/futures-async-stream-macro/Cargo.toml index e36a183..1d0ea46 100644 --- a/futures-async-stream-macro/Cargo.toml +++ b/futures-async-stream-macro/Cargo.toml @@ -20,3 +20,6 @@ proc-macro = true proc-macro2 = "1.0.60" quote = "1" syn = { version = "2.0.1", features = ["full", "visit-mut"] } + +[lints] +workspace = true diff --git a/futures-async-stream-macro/src/lib.rs b/futures-async-stream-macro/src/lib.rs index a8ad79a..36cba51 100644 --- a/futures-async-stream-macro/src/lib.rs +++ b/futures-async-stream-macro/src/lib.rs @@ -10,7 +10,6 @@ ) ))] #![forbid(unsafe_code)] -#![warn(rust_2018_idioms, single_use_lifetimes, unreachable_pub, clippy::pedantic)] #![feature(proc_macro_def_site)] #[macro_use] diff --git a/src/lib.rs b/src/lib.rs index 79c298c..58ca669 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -269,10 +269,6 @@ where ) ))] #![warn( - rust_2018_idioms, - single_use_lifetimes, - unreachable_pub, - clippy::pedantic, // Lints that may help when writing public library. missing_debug_implementations, missing_docs, @@ -283,16 +279,6 @@ where clippy::missing_inline_in_public_items, clippy::std_instead_of_alloc, clippy::std_instead_of_core, - // Lints that may help when writing unsafe code. - improper_ctypes, - improper_ctypes_definitions, - unsafe_op_in_unsafe_fn, - clippy::as_ptr_cast_mut, - clippy::default_union_representation, - clippy::inline_asm_x86_att_syntax, - clippy::trailing_empty_array, - clippy::transmute_undefined_repr, - clippy::undocumented_unsafe_blocks, )] #![allow(clippy::must_use_candidate)] #![feature(coroutine_trait)] @@ -547,7 +533,6 @@ pub mod __private { pub use core::future::Future; #[doc(hidden)] - #[allow(unreachable_pub)] // https://github.com/rust-lang/rust/issues/102352 pub use crate::future::{from_coroutine, get_context, ResumeTy}; } diff --git a/tests/compiletest.rs b/tests/compiletest.rs index 926d5e7..ff86f1f 100644 --- a/tests/compiletest.rs +++ b/tests/compiletest.rs @@ -2,17 +2,10 @@ #![cfg(not(miri))] #![cfg(not(careful))] -#![warn(rust_2018_idioms, single_use_lifetimes)] - -use std::env; #[rustversion::attr(not(nightly), ignore)] #[test] fn ui() { - if env::var_os("CI").is_none() { - env::set_var("TRYBUILD", "overwrite"); - } - let t = trybuild::TestCases::new(); t.compile_fail("tests/ui/**/*.rs"); t.pass("tests/run-pass/**/*.rs"); diff --git a/tests/elisions.rs b/tests/elisions.rs index 402d819..4daaff4 100644 --- a/tests/elisions.rs +++ b/tests/elisions.rs @@ -1,6 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![warn(rust_2018_idioms, single_use_lifetimes)] #![allow(clippy::needless_pass_by_value)] #![allow(clippy::needless_lifetimes)] // broken #![feature(coroutines)] diff --git a/tests/for_await.rs b/tests/for_await.rs index a2a9245..ffb1e75 100644 --- a/tests/for_await.rs +++ b/tests/for_await.rs @@ -1,6 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![warn(rust_2018_idioms, single_use_lifetimes)] #![allow(clippy::semicolon_if_nothing_returned)] // broken #![feature(coroutines, proc_macro_hygiene, stmt_expr_attributes)] diff --git a/tests/no-std/Cargo.toml b/tests/no-std/Cargo.toml index 8e025b8..5d6a177 100644 --- a/tests/no-std/Cargo.toml +++ b/tests/no-std/Cargo.toml @@ -10,3 +10,6 @@ path = "lib.rs" [dependencies] futures-async-stream = { path = "../.." } futures-core = { version = "0.3", default-features = false } + +[lints] +workspace = true diff --git a/tests/no-std/lib.rs b/tests/no-std/lib.rs index bc34086..20d9e9a 100644 --- a/tests/no-std/lib.rs +++ b/tests/no-std/lib.rs @@ -1,8 +1,8 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT #![no_std] -#![warn(rust_2018_idioms, single_use_lifetimes)] #![feature(coroutines)] +#![allow(clippy::no_effect_underscore_binding)] // broken use futures_async_stream::{stream, try_stream}; diff --git a/tests/overwriting_core_crate.rs b/tests/overwriting_core_crate.rs index deb6fce..928708f 100644 --- a/tests/overwriting_core_crate.rs +++ b/tests/overwriting_core_crate.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![warn(rust_2018_idioms, single_use_lifetimes)] #![feature(coroutines)] +#![allow(clippy::no_effect_underscore_binding)] // broken // See https://github.com/rust-lang/pin-utils/pull/26#discussion_r344491597 // diff --git a/tests/stream.rs b/tests/stream.rs index 63b9f87..b72605d 100644 --- a/tests/stream.rs +++ b/tests/stream.rs @@ -1,8 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![warn(rust_2018_idioms, single_use_lifetimes)] #![allow(clippy::unused_async)] -#![allow(clippy::needless_lifetimes, clippy::semicolon_if_nothing_returned)] // broken +#![allow( + clippy::must_use_candidate, + clippy::needless_lifetimes, + clippy::no_effect_underscore_binding, + clippy::semicolon_if_nothing_returned +)] // broken #![feature(coroutines, proc_macro_hygiene, stmt_expr_attributes, gen_future)] use std::{pin::Pin, rc::Rc, sync::Arc}; diff --git a/tests/try_stream.rs b/tests/try_stream.rs index e88fbc0..03d45de 100644 --- a/tests/try_stream.rs +++ b/tests/try_stream.rs @@ -1,8 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![warn(rust_2018_idioms, single_use_lifetimes)] -#![allow(clippy::try_err, clippy::unused_async)] -#![allow(clippy::semicolon_if_nothing_returned)] // broken +#![allow(unreachable_pub, clippy::try_err, clippy::unused_async)] +#![allow(clippy::semicolon_if_nothing_returned, clippy::no_effect_underscore_binding)] // broken #![feature(coroutines, proc_macro_hygiene, stmt_expr_attributes)] use futures::{ diff --git a/tools/codegen/Cargo.toml b/tools/codegen/Cargo.toml index 8a9aebf..da530b3 100644 --- a/tools/codegen/Cargo.toml +++ b/tools/codegen/Cargo.toml @@ -12,3 +12,6 @@ prettyplease = "0.2" proc-macro2 = "1" quote = "1" syn = { version = "2", features = ["full", "visit-mut"] } + +[lints] +workspace = true diff --git a/tools/codegen/src/file.rs b/tools/codegen/src/file.rs index d904396..20d6985 100644 --- a/tools/codegen/src/file.rs +++ b/tools/codegen/src/file.rs @@ -23,7 +23,7 @@ macro_rules! function_name { }}; } -pub fn workspace_root() -> PathBuf { +pub(crate) fn workspace_root() -> PathBuf { let mut dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); dir.pop(); // codegen dir.pop(); // tools @@ -31,7 +31,7 @@ pub fn workspace_root() -> PathBuf { } #[track_caller] -pub fn header(function_name: &str) -> String { +pub(crate) fn header(function_name: &str) -> String { // rust-analyzer does not respect outer attribute (#[rustfmt::skip]) on // a module without a body. So use inner attribute under cfg(rustfmt). format!( @@ -47,7 +47,11 @@ pub fn header(function_name: &str) -> String { } #[track_caller] -pub fn write(function_name: &str, path: impl AsRef, contents: TokenStream) -> Result<()> { +pub(crate) fn write( + function_name: &str, + path: impl AsRef, + contents: TokenStream, +) -> Result<()> { write_raw(function_name, path.as_ref(), format_tokens(contents)?) } @@ -119,7 +123,11 @@ fn test_format_macros() { } #[track_caller] -pub fn write_raw(function_name: &str, path: &Path, contents: impl AsRef<[u8]>) -> Result<()> { +pub(crate) fn write_raw( + function_name: &str, + path: &Path, + contents: impl AsRef<[u8]>, +) -> Result<()> { static LINGUIST_GENERATED: OnceLock> = OnceLock::new(); let linguist_generated = LINGUIST_GENERATED.get_or_init(|| { let gitattributes = fs::read_to_string(workspace_root().join(".gitattributes")).unwrap(); @@ -148,7 +156,7 @@ pub fn write_raw(function_name: &str, path: &Path, contents: impl AsRef<[u8]>) - Ok(()) } -pub fn git_ls_files(dir: &Path, filters: &[&str]) -> Result> { +pub(crate) fn git_ls_files(dir: &Path, filters: &[&str]) -> Result> { let mut cmd = Command::new("git"); cmd.arg("ls-files").args(filters).current_dir(dir); let output = cmd.output().with_context(|| format!("could not execute process `{cmd:?}`"))?; diff --git a/tools/codegen/src/main.rs b/tools/codegen/src/main.rs index b788bee..5e410d2 100644 --- a/tools/codegen/src/main.rs +++ b/tools/codegen/src/main.rs @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -#![warn(rust_2018_idioms, single_use_lifetimes)] -#![allow(clippy::match_on_vec_items, clippy::needless_pass_by_value)] +#![allow(clippy::match_on_vec_items, clippy::needless_pass_by_value, clippy::wildcard_imports)] #[macro_use] mod file; @@ -69,13 +68,11 @@ fn gen_assert_impl() -> Result<()> { visited_types.insert(path_string.clone()); let has_generics = generics.type_params().count() != 0; - if generics.const_params().count() != 0 { - panic!( - "gen_assert_impl doesn't support const generics yet; \ - skipped `{}`", - path_string - ); - } + assert_eq!( + generics.const_params().count(), + 0, + "gen_assert_impl doesn't support const generics yet; skipped `{path_string}`" + ); let lt = generics.lifetimes().map(|_| quote! { '_ }).collect::>(); if has_generics { @@ -170,10 +167,10 @@ fn gen_assert_impl() -> Result<()> { }); } } else { - let lt = if !lt.is_empty() { - quote! { <#(#lt),*> } - } else { + let lt = if lt.is_empty() { quote! {} + } else { + quote! { <#(#lt),*> } }; if NOT_SEND.contains(&path_string.as_str()) { use_macros = true; @@ -233,26 +230,24 @@ fn gen_assert_impl() -> Result<()> { } for &t in NOT_SEND { - assert!(visited_types.contains(t), "unknown type `{}` specified in NOT_SEND constant", t); + assert!(visited_types.contains(t), "unknown type `{t}` specified in NOT_SEND constant"); } for &t in NOT_SYNC { - assert!(visited_types.contains(t), "unknown type `{}` specified in NOT_SYNC constant", t); + assert!(visited_types.contains(t), "unknown type `{t}` specified in NOT_SYNC constant"); } for &t in NOT_UNPIN { - assert!(visited_types.contains(t), "unknown type `{}` specified in NOT_UNPIN constant", t); + assert!(visited_types.contains(t), "unknown type `{t}` specified in NOT_UNPIN constant"); } for &t in NOT_UNWIND_SAFE { assert!( visited_types.contains(t), - "unknown type `{}` specified in NOT_UNWIND_SAFE constant", - t + "unknown type `{t}` specified in NOT_UNWIND_SAFE constant" ); } for &t in NOT_REF_UNWIND_SAFE { assert!( visited_types.contains(t), - "unknown type `{}` specified in NOT_REF_UNWIND_SAFE constant", - t + "unknown type `{t}` specified in NOT_REF_UNWIND_SAFE constant" ); } diff --git a/tools/tidy.sh b/tools/tidy.sh index 7e8ce5a..8b6a2aa 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