From 56e82108afa1e54a4bb1996251d3d79016a092e0 Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Fri, 10 Jan 2025 19:01:24 -0800 Subject: [PATCH 1/3] port style.rs to syn and add tests for the style checker --- .gitignore | 1 - libc-test/Cargo.toml | 16 ++ libc-test/test/check_style.rs | 50 ++++ libc-test/test/style/mod.rs | 490 ++++++++++++++++++++++++++++++++++ libc-test/test/style_tests.rs | 260 ++++++++++++++++++ 5 files changed, 816 insertions(+), 1 deletion(-) create mode 100644 libc-test/test/check_style.rs create mode 100644 libc-test/test/style/mod.rs create mode 100644 libc-test/test/style_tests.rs diff --git a/.gitignore b/.gitignore index bbbad4bc51532..f0ff2599d09b5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,3 @@ target Cargo.lock *~ -style diff --git a/libc-test/Cargo.toml b/libc-test/Cargo.toml index 721ccc90932dc..857886711dfa3 100644 --- a/libc-test/Cargo.toml +++ b/libc-test/Cargo.toml @@ -15,6 +15,12 @@ A test crate for the libc crate. [dependencies] libc = { path = "..", version = "1.0.0-alpha.1", default-features = false } +[dev-dependencies] +syn = { version = "2.0.91", features = ["full", "visit"] } +proc-macro2 = { version = "1.0.92", features = ["span-locations"] } +glob = "0.3.2" +annotate-snippets = { version = "0.11.5", features = ["testing-colors"] } + [build-dependencies] cc = "1.0.83" # FIXME: Use fork ctest until the maintainer gets back. @@ -90,3 +96,13 @@ harness = false name = "primitive_types" path = "test/primitive_types.rs" harness = true + +[[test]] +name = "style" +path = "test/check_style.rs" +harness = true + +[[test]] +name = "style_tests" +path = "test/style_tests.rs" +harness = true diff --git a/libc-test/test/check_style.rs b/libc-test/test/check_style.rs new file mode 100644 index 0000000000000..ee5e134891104 --- /dev/null +++ b/libc-test/test/check_style.rs @@ -0,0 +1,50 @@ +//! Simple script to verify the coding style of this library. +//! +//! ## How to run +//! +//! The first argument to this script is the directory to run on, so running +//! this script should be as simple as: +//! +//! ```notrust +//! cargo test --test style +//! ``` + +pub mod style; + +use std::env; +use std::path::Path; + +use style::{Result, StyleChecker}; + +#[test] +fn check_style() { + let root_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../src"); + walk(&root_dir).unwrap(); + eprintln!("good style!"); +} + +fn walk(root_dir: &Path) -> Result<()> { + let mut style_checker = StyleChecker::new(); + + for entry in glob::glob(&format!( + "{}/**/*.rs", + root_dir.to_str().expect("dir should be valid UTF-8") + ))? { + let entry = entry?; + + let name = entry + .file_name() + .expect("file name should not end in ..") + .to_str() + .expect("file name should be valid UTF-8"); + if let "lib.rs" | "macros.rs" = &name[..] { + continue; + } + + let path = entry.as_path(); + style_checker.check_file(path)?; + style_checker.reset_state(); + } + + style_checker.finalize() +} diff --git a/libc-test/test/style/mod.rs b/libc-test/test/style/mod.rs new file mode 100644 index 0000000000000..cc953d32c3aed --- /dev/null +++ b/libc-test/test/style/mod.rs @@ -0,0 +1,490 @@ +//! Provides the [StyleChecker] visitor to verify the coding style of +//! this library. +//! +//! This is split out so that the implementation itself can be tested +//! separately, see test/check_style.rs for how it's used and +//! test/style_tests.rs for the implementation tests. +//! +//! ## Guidelines +//! +//! The current style is: +//! +//! * Specific module layout: +//! 1. use directives +//! 2. typedefs +//! 3. structs +//! 4. constants +//! 5. f! { ... } functions +//! 6. extern functions +//! 7. modules + pub use +//! * No manual deriving Copy/Clone +//! * Only one f! per module +//! * Multiple s! macros are allowed as long as there isn't a duplicate cfg, +//! whether as a standalone attribute (#[cfg]) or in a cfg_if! +//! * s! macros should not just have a positive cfg since they should +//! just go into the relevant file but combined cfgs with all(...) and +//! any(...) are allowed + +use std::collections::HashMap; +use std::fs; +use std::ops::Deref; +use std::path::{Path, PathBuf}; + +use annotate_snippets::{Level, Renderer, Snippet}; +use proc_macro2::Span; +use syn::parse::{Parse, ParseStream}; +use syn::spanned::Spanned; +use syn::visit::{self, Visit}; +use syn::Token; + +const ALLOWED_REPEATED_MACROS: &[&str] = &["s", "s_no_extra_traits", "s_paren"]; + +pub type Error = Box; +pub type Result = std::result::Result; + +#[derive(Default)] +pub struct StyleChecker { + /// The state the style checker is in, used to enforce the module layout. + state: State, + /// Span of the first item encountered in this state to use in help + /// diagnostic text. + state_span: Option, + /// The s! macro cfgs we have seen, whether through #[cfg] attributes + /// or within the branches of cfg_if! blocks so that we can check for duplicates. + seen_s_macro_cfgs: HashMap, + /// Span of the first f! macro seen, used to enforce only one f! macro + /// per module. + first_f_macro: Option, + /// The errors that the style checker has seen. + errors: Vec, + /// Path of the currently active file. + path: PathBuf, + /// Whether the style checker is currently in an `impl` block. + in_impl: bool, +} + +/// The part of the module layout we are currently checking. +#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum State { + #[default] + Start, + Imports, + Typedefs, + Structs, + Constants, + FunctionDefinitions, + Functions, + Modules, +} + +/// Similar to [syn::ExprIf] except with [syn::Attribute] +/// as the condition instead of [syn::Expr]. +struct ExprCfgIf { + _cond: syn::Attribute, + /// A `cfg_if!` branch can only contain items. + then_branch: Vec, + else_branch: Option>, +} + +enum ExprCfgElse { + /// Final block with no condition `else { /* ... */ }`. + Block(Vec), + /// `else if { /* ... */ }` block. + If(ExprCfgIf), +} + +/// Describes an that occurred error when checking the file +/// at the given `path`. Besides the error message, it contains +/// additional span information so that we can print nice error messages. +#[derive(Debug)] +struct FileError { + path: PathBuf, + span: Span, + title: String, + msg: String, + help: Option, +} + +/// Help message with an optional span where the help should point to. +type HelpMsg = (Option, String); + +impl StyleChecker { + pub fn new() -> Self { + Self::default() + } + + /// Reads and parses the file at the given path and checks + /// for any style violations. + pub fn check_file(&mut self, path: &Path) -> Result<()> { + let contents = fs::read_to_string(path)?; + + self.path = PathBuf::from(path); + self.check_string(contents) + } + + pub fn check_string(&mut self, contents: String) -> Result<()> { + let file = syn::parse_file(&contents)?; + self.visit_file(&file); + Ok(()) + } + + /// Resets the state of the [StyleChecker]. + pub fn reset_state(&mut self) { + *self = Self { + errors: std::mem::take(&mut self.errors), + ..Self::default() + }; + } + + /// Collect all errors into a single error, reporting them if any. + pub fn finalize(self) -> Result<()> { + if self.errors.is_empty() { + return Ok(()); + } + + let renderer = Renderer::styled(); + for error in self.errors { + let source = fs::read_to_string(&error.path)?; + + let mut snippet = Snippet::source(&source) + .origin(error.path.to_str().expect("path to be UTF-8")) + .fold(true) + .annotation(Level::Error.span(error.span.byte_range()).label(&error.msg)); + if let Some((help_span, help_msg)) = &error.help { + if let Some(help_span) = help_span { + snippet = snippet + .annotation(Level::Help.span(help_span.byte_range()).label(help_msg)); + } + } + + let mut msg = Level::Error.title(&error.title).snippet(snippet); + if let Some((help_span, help_msg)) = &error.help { + if help_span.is_none() { + msg = msg.footer(Level::Help.title(help_msg)) + } + } + + eprintln!("{}", renderer.render(msg)); + } + + Err("some tests failed".into()) + } + + fn set_state(&mut self, new_state: State, span: Span) { + if self.state > new_state && !self.in_impl { + let help_span = self + .state_span + .expect("state_span should be set since we are on a second state"); + self.error( + "incorrect module layout".to_string(), + span, + format!( + "{} found after {} when it belongs before", + new_state.desc(), + self.state.desc() + ), + ( + Some(help_span), + format!( + "move the {} to before this {}", + new_state.desc(), + self.state.desc() + ), + ), + ); + } + + if self.state != new_state { + self.state = new_state; + self.state_span = Some(span); + } + } + + /// Visit the items inside the [ExprCfgIf], restoring the state after + /// each branch. + fn visit_expr_cfg_if(&mut self, expr_cfg_if: &ExprCfgIf) { + let initial_state = self.state; + + for item in &expr_cfg_if.then_branch { + self.visit_item(item); + } + self.state = initial_state; + + if let Some(else_branch) = &expr_cfg_if.else_branch { + match else_branch.deref() { + ExprCfgElse::Block(items) => { + for item in items { + self.visit_item(item); + } + } + ExprCfgElse::If(expr_cfg_if) => self.visit_expr_cfg_if(&expr_cfg_if), + } + } + self.state = initial_state; + } + + /// If we see a normal s! macro without any attributes we just need + /// to check if there are any duplicates. + fn handle_s_macro_no_attrs(&mut self, item_macro: &syn::ItemMacro) { + let span = item_macro.span(); + match self.seen_s_macro_cfgs.get("") { + Some(seen_span) => { + self.error( + "duplicate s! macro".to_string(), + span, + format!("other s! macro"), + (Some(*seen_span), "combine the two".to_string()), + ); + } + None => { + self.seen_s_macro_cfgs.insert(String::new(), span); + } + } + } + + /// If an s! macro has attributes we check for any duplicates as well + /// as if they are standalone positive cfgs that would be better + /// in a separate file. + fn handle_s_macro_with_attrs(&mut self, item_macro: &syn::ItemMacro) { + for attr in &item_macro.attrs { + let Ok(meta_list) = attr.meta.require_list() else { + continue; + }; + + if meta_list.path.is_ident("cfg") { + let span = meta_list.span(); + let meta_str = meta_list.tokens.to_string(); + + match self.seen_s_macro_cfgs.get(&meta_str) { + Some(seen_span) => { + self.error( + "duplicate #[cfg] for s! macro".to_string(), + span, + "duplicated #[cfg]".to_string(), + (Some(*seen_span), "combine the two".to_string()), + ); + } + None => { + self.seen_s_macro_cfgs.insert(meta_str.clone(), span); + } + } + + if !meta_str.starts_with("not") + && !meta_str.starts_with("any") + && !meta_str.starts_with("all") + { + self.error( + "positive #[cfg] for s! macro".to_string(), + span, + String::new(), + (None, "move it to the relevant file".to_string()), + ); + } + } + } + } + + fn push_error(&mut self, title: String, span: Span, msg: String, help: Option) { + self.errors.push(FileError { + path: self.path.clone(), + title, + span, + msg, + help, + }); + } + + fn error(&mut self, title: String, span: Span, msg: String, help: HelpMsg) { + self.push_error(title, span, msg, Some(help)); + } +} + +impl<'ast> Visit<'ast> for StyleChecker { + /// Visit all items; most just update our current state but some also + /// perform additional checks like for the s! macro. + fn visit_item_use(&mut self, item_use: &'ast syn::ItemUse) { + let span = item_use.span(); + let new_state = if matches!(item_use.vis, syn::Visibility::Public(_)) { + State::Modules + } else { + State::Imports + }; + self.set_state(new_state, span); + + visit::visit_item_use(self, item_use); + } + + fn visit_item_const(&mut self, item_const: &'ast syn::ItemConst) { + let span = item_const.span(); + self.set_state(State::Constants, span); + + visit::visit_item_const(self, item_const); + } + + fn visit_item_impl(&mut self, item_impl: &'ast syn::ItemImpl) { + self.in_impl = true; + visit::visit_item_impl(self, item_impl); + self.in_impl = false; + } + + fn visit_item_struct(&mut self, item_struct: &'ast syn::ItemStruct) { + let span = item_struct.span(); + self.set_state(State::Structs, span); + + visit::visit_item_struct(self, item_struct); + } + + fn visit_item_type(&mut self, item_type: &'ast syn::ItemType) { + let span = item_type.span(); + self.set_state(State::Typedefs, span); + + visit::visit_item_type(self, item_type); + } + + /// Checks s! macros for any duplicate cfgs and whether they are + /// just positive #[cfg(...)] attributes. We need [syn::ItemMacro] + /// instead of [syn::Macro] because it contains the attributes. + fn visit_item_macro(&mut self, item_macro: &'ast syn::ItemMacro) { + if item_macro.mac.path.is_ident("s") { + if item_macro.attrs.is_empty() { + self.handle_s_macro_no_attrs(item_macro); + } else { + self.handle_s_macro_with_attrs(item_macro); + } + } + + visit::visit_item_macro(self, item_macro); + } + + fn visit_macro(&mut self, mac: &'ast syn::Macro) { + let span = mac.span(); + if mac.path.is_ident("cfg_if") { + let expr_cfg_if: ExprCfgIf = mac + .parse_body() + .expect("cfg_if! should be parsed since it compiled"); + + self.visit_expr_cfg_if(&expr_cfg_if); + } else { + let new_state = + if mac.path.get_ident().is_some_and(|ident| { + ALLOWED_REPEATED_MACROS.contains(&ident.to_string().as_str()) + }) { + // multiple macros of this type are allowed + State::Structs + } else if mac.path.is_ident("f") { + match self.first_f_macro { + Some(f_macro_span) => { + self.error( + "multiple f! macros in one module".to_string(), + span, + "other f! macro".to_string(), + ( + Some(f_macro_span), + "combine it with this f! macro".to_string(), + ), + ); + } + None => { + self.first_f_macro = Some(span); + } + } + State::FunctionDefinitions + } else { + self.state + }; + self.set_state(new_state, span); + } + + visit::visit_macro(self, mac); + } + + fn visit_item_foreign_mod(&mut self, item_foreign_mod: &'ast syn::ItemForeignMod) { + let span = item_foreign_mod.span(); + self.set_state(State::Functions, span); + + visit::visit_item_foreign_mod(self, item_foreign_mod); + } + + fn visit_item_mod(&mut self, item_mod: &'ast syn::ItemMod) { + let span = item_mod.span(); + self.set_state(State::Modules, span); + + visit::visit_item_mod(self, item_mod); + } + + fn visit_meta_list(&mut self, meta_list: &'ast syn::MetaList) { + let span = meta_list.span(); + let meta_str = meta_list.tokens.to_string(); + if meta_list.path.is_ident("derive") + && (meta_str.contains("Copy") || meta_str.contains("Clone")) + { + self.error( + "impl Copy and Clone manually".to_string(), + span, + "found manual implementation of Copy and/or Clone".to_string(), + (None, "use one of the s! macros instead".to_string()), + ); + } + + visit::visit_meta_list(self, meta_list); + } +} + +impl Parse for ExprCfgIf { + fn parse(input: ParseStream) -> syn::Result { + input.parse::()?; + let cond = input + .call(syn::Attribute::parse_outer)? + .into_iter() + .next() + .expect("an attribute should be present since it compiled"); + + let content; + syn::braced!(content in input); + let mut then_branch = Vec::new(); + while !content.is_empty() { + let mut value = content.parse()?; + if let syn::Item::Macro(item_macro) = &mut value { + item_macro.attrs.push(cond.clone()); + } + then_branch.push(value); + } + + let mut else_branch = None; + if input.peek(Token![else]) { + input.parse::()?; + + if input.peek(Token![if]) { + else_branch = Some(Box::new(ExprCfgElse::If(input.parse()?))); + } else { + let content; + syn::braced!(content in input); + let mut items = Vec::new(); + while !content.is_empty() { + items.push(content.parse()?); + } + else_branch = Some(Box::new(ExprCfgElse::Block(items))); + } + } + Ok(Self { + _cond: cond, + then_branch, + else_branch, + }) + } +} + +impl State { + fn desc(&self) -> &str { + match *self { + State::Start => "start", + State::Imports => "import", + State::Typedefs => "typedef", + State::Structs => "struct", + State::Constants => "constant", + State::FunctionDefinitions => "function definition", + State::Functions => "extern function", + State::Modules => "module", + } + } +} diff --git a/libc-test/test/style_tests.rs b/libc-test/test/style_tests.rs new file mode 100644 index 0000000000000..be8fddbccf644 --- /dev/null +++ b/libc-test/test/style_tests.rs @@ -0,0 +1,260 @@ +//! Verifies the implementation of the style checker in [style]. + +use style::StyleChecker; + +pub mod style; + +#[test] +fn check_style_accept_correct_module_layout() { + let contents = r#" +use core::mem::size_of; +pub type foo_t = u32; +struct Foo {} +pub const FOO: u32 = 0x20000; +f! {} +extern "C" {} +mod foolib; +pub use self::foolib::*; +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_incorrect_module_layout() { + let contents = r#" +use core::mem::size_of; +pub type foo_t = u32; +struct Foo {} +pub const FOO: u32 = 0x20000; +extern "C" {} +f! {} +mod foolib; +pub use self::foolib::*; +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_incorrect_cfg_if_layout() { + let contents = r#" +cfg_if! { + if #[cfg(foo)] { + pub type foo_t = u32; + use core::mem::size_of; + } +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_cfg_if_branch_resets_state() { + let contents = r#" +cfg_if! { + if #[cfg(foo)] { + use core::mem::size_of; + pub type foo_t = u32; + } else { + use core::mem::align_of; + } +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_multiple_f_macros() { + let contents = r#" +f! {} +f! {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_cfg_ignore_target_endian_nested() { + let contents = r#" +pub struct Foo { + #[cfg(target_endian = "little")] + pub id: __u16, +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_manual_copy() { + let contents = r#" +#[derive(Copy)] +pub struct Foo {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_manual_clone() { + let contents = r#" +#[derive(Clone)] +pub struct Foo {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_multiple_s_macros_with_disjoint_cfg() { + let contents = r#" +// Main `s!` +s! {} + +// These are not supported on a single arch. It doesn't make sense to +// duplicate `foo` into every single file except one, so allow this here. +#[cfg(not(target_arch = "foo"))] +s! { pub struct foo { /* ... */ } } + +// Similar to the above, no problems here +#[cfg(not(target_os = "illumos"))] +s! { pub struct bar { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_duplicated_s_macro() { + let contents = r#" +s! {} +s! {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_duplicated_s_macro_cfg() { + let contents = r#" +#[cfg(not(target_arch = "foo"))] +s! {} + +#[cfg(not(target_arch = "foo"))] +s! {} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_single_positive_s_macro_cfg() { + let contents = r#" +// A positive (no `not`) config: reject because this should go into +// the relevant file. +#[cfg(target_arch = "foo")] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_reject_single_positive_s_macro_cfg_target_os() { + let contents = r#" +// A positive (no `not`) config: reject because this should go into +// the relevant file. +#[cfg(target_os = "foo")] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} + +#[test] +fn check_style_accept_positive_s_macro_any() { + let contents = r#" +// It's nicer to accept this so that we don't have to duplicate the same struct 3 times. +#[cfg(any(target_arch = "foo", target_arch = "bar", target_arch = "baz"))] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_accept_positive_s_macro_all() { + let contents = r#" +#[cfg(all(target_arch = "foo", target_arch = "bar", target_arch = "baz"))] +s! { pub struct foo { /* ... */ } } +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + checker.finalize().unwrap(); +} + +#[test] +fn check_style_reject_duplicated_cfg_and_cfg_if() { + let contents = r#" +#[cfg(not(target_arch = "foo"))] +s! { pub struct foo { /* ... */ } } + +cfg_if! { + if #[cfg(not(target_arch = "foo"))] { + s!{ pub struct bar {} } + } +} +"# + .to_string(); + + let mut checker = StyleChecker::new(); + checker.check_string(contents).unwrap(); + assert!(checker.finalize().is_err()); +} From bd1b83864184b0587d666bb1b3f1b563a487bde2 Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Fri, 10 Jan 2025 19:02:21 -0800 Subject: [PATCH 2/3] update style script and ci to run new style checker --- ci/run.sh | 19 ++-- ci/runtest-android.rs | 20 ++-- ci/style.rs | 213 ------------------------------------------ ci/style.sh | 2 +- 4 files changed, 25 insertions(+), 229 deletions(-) delete mode 100644 ci/style.rs diff --git a/ci/run.sh b/ci/run.sh index 9754118f742b8..8889cda5a21e5 100755 --- a/ci/run.sh +++ b/ci/run.sh @@ -81,6 +81,7 @@ if [ -n "${QEMU:-}" ]; then fi cmd="cargo test --target $target ${LIBC_CI_ZBUILD_STD+"-Zbuild-std"}" +test_flags="--skip check_style" # Run tests in the `libc` crate case "$target" in @@ -101,17 +102,20 @@ if [ "$target" = "s390x-unknown-linux-gnu" ]; then passed=0 until [ $n -ge $N ]; do if [ "$passed" = "0" ]; then - if $cmd --no-default-features; then + # shellcheck disable=SC2086 + if $cmd --no-default-features -- $test_flags; then passed=$((passed+1)) continue fi elif [ "$passed" = "1" ]; then - if $cmd; then + # shellcheck disable=SC2086 + if $cmd -- $test_flags; then passed=$((passed+1)) continue fi elif [ "$passed" = "2" ]; then - if $cmd --features extra_traits; then + # shellcheck disable=SC2086 + if $cmd --features extra_traits -- $test_flags; then break fi fi @@ -119,7 +123,10 @@ if [ "$target" = "s390x-unknown-linux-gnu" ]; then sleep 1 done else - $cmd --no-default-features - $cmd - $cmd --features extra_traits + # shellcheck disable=SC2086 + $cmd --no-default-features -- $test_flags + # shellcheck disable=SC2086 + $cmd -- $test_flags + # shellcheck disable=SC2086 + $cmd --features extra_traits -- $test_flags fi diff --git a/ci/runtest-android.rs b/ci/runtest-android.rs index 92bce79b0d714..d422f9c2e8a7e 100644 --- a/ci/runtest-android.rs +++ b/ci/runtest-android.rs @@ -1,11 +1,11 @@ use std::env; -use std::process::Command; use std::path::{Path, PathBuf}; +use std::process::Command; fn main() { let args = env::args_os() .skip(1) - .filter(|arg| arg != "--quiet") + .filter(|arg| arg != "--quiet" && arg != "--skip" && arg != "check_style") .collect::>(); assert_eq!(args.len(), 1); let test = PathBuf::from(&args[0]); @@ -36,14 +36,16 @@ fn main() { let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); - println!("status: {}\nstdout ---\n{}\nstderr ---\n{}", - output.status, - stdout, - stderr); + println!( + "status: {}\nstdout ---\n{}\nstderr ---\n{}", + output.status, stdout, stderr + ); - if !stderr.lines().any(|l| (l.starts_with("PASSED ") && l.contains(" tests")) || l.starts_with("test result: ok")) - && !stdout.lines().any(|l| (l.starts_with("PASSED ") && l.contains(" tests")) || l.starts_with("test result: ok")) - { + if !stderr.lines().any(|l| { + (l.starts_with("PASSED ") && l.contains(" tests")) || l.starts_with("test result: ok") + }) && !stdout.lines().any(|l| { + (l.starts_with("PASSED ") && l.contains(" tests")) || l.starts_with("test result: ok") + }) { panic!("failed to find successful test run"); }; } diff --git a/ci/style.rs b/ci/style.rs deleted file mode 100644 index cad08f2eecc88..0000000000000 --- a/ci/style.rs +++ /dev/null @@ -1,213 +0,0 @@ -//! Simple script to verify the coding style of this library -//! -//! ## How to run -//! -//! The first argument to this script is the directory to run on, so running -//! this script should be as simple as: -//! -//! ```notrust -//! rustc ci/style.rs -//! ./style src -//! ``` -//! -//! ## Guidelines -//! -//! The current style is: -//! -//! * Specific module layout: -//! 1. use directives -//! 2. typedefs -//! 3. structs -//! 4. constants -//! 5. f! { ... } functions -//! 6. extern functions -//! 7. modules + pub use -//! -//! Things not verified: -//! -//! * alignment -//! * leading colons on paths - -use std::io::prelude::*; -use std::path::Path; -use std::{env, fs}; - -macro_rules! t { - ($e:expr) => { - match $e { - Ok(e) => e, - Err(e) => panic!("{} failed with {}", stringify!($e), e), - } - }; -} - -fn main() { - let arg = env::args().skip(1).next().unwrap_or(".".to_string()); - - let mut errors = Errors { errs: false }; - walk(Path::new(&arg), &mut errors); - - if errors.errs { - panic!("found some lint errors"); - } else { - println!("good style!"); - } -} - -fn walk(path: &Path, err: &mut Errors) { - for entry in t!(path.read_dir()).map(|e| t!(e)) { - let path = entry.path(); - if t!(entry.file_type()).is_dir() { - walk(&path, err); - continue; - } - - let name = entry.file_name().into_string().unwrap(); - match &name[..] { - n if !n.ends_with(".rs") => continue, - - "lib.rs" | "macros.rs" => continue, - - _ => {} - } - - let mut contents = String::new(); - t!(t!(fs::File::open(&path)).read_to_string(&mut contents)); - - check_style(&contents, &path, err); - } -} - -struct Errors { - errs: bool, -} - -#[derive(Clone, Copy, PartialEq)] -enum State { - Start, - Imports, - Typedefs, - Structs, - Constants, - FunctionDefinitions, - Functions, - Modules, -} - -fn check_style(file: &str, path: &Path, err: &mut Errors) { - let mut state = State::Start; - - // FIXME: see below - // let mut s_macros = 0; - - let mut f_macros = 0; - let mut in_impl = false; - - for (i, line) in file.lines().enumerate() { - if line.contains("#[cfg(") - && line.contains(']') - && !line.contains(" if ") - && !(line.contains("target_endian") || line.contains("target_arch")) - { - if state != State::Structs { - err.error(path, i, "use cfg_if! and submodules instead of #[cfg]"); - } - } - if line.contains("#[derive(") && (line.contains("Copy") || line.contains("Clone")) { - err.error(path, i, "impl Copy and Clone manually"); - } - if line.contains("impl") { - in_impl = true; - } - if in_impl && line.starts_with('}') { - in_impl = false; - } - - let orig_line = line; - let line = line.trim_start(); - let is_pub = line.starts_with("pub "); - let line = if is_pub { &line[4..] } else { line }; - - let line_state = if line.starts_with("use ") { - if line.contains("c_void") || line.contains("c_char") { - continue; - } - if is_pub { - State::Modules - } else { - State::Imports - } - } else if line.starts_with("const ") { - State::Constants - } else if line.starts_with("type ") && !in_impl { - State::Typedefs - } else if line.starts_with("s! {") { - // FIXME: see below - // s_macros += 1; - State::Structs - } else if line.starts_with("s_no_extra_traits! {") { - // multiple macros of this type are allowed - State::Structs - } else if line.starts_with("s_paren! {") { - // multiple macros of this type are allowed - State::Structs - } else if line.starts_with("f! {") { - f_macros += 1; - State::FunctionDefinitions - } else if line.starts_with("extern ") && !orig_line.starts_with(" ") { - State::Functions - } else if line.starts_with("mod ") { - State::Modules - } else { - continue; - }; - - if state as usize > line_state as usize { - err.error( - path, - i, - &format!( - "{} found after {} when it belongs before", - line_state.desc(), - state.desc() - ), - ); - } - - if f_macros == 2 { - f_macros += 1; - err.error(path, i, "multiple f! macros in one module"); - } - - // FIXME(#4109): multiple should be allowed if at least one is `cfg(not) within `cfg_if`. - // For now just disable this and check by hand. - // if s_macros == 2 { - // s_macros += 1; - // err.error(path, i, "multiple s! macros in one module"); - // } - - state = line_state; - } -} - -impl State { - fn desc(&self) -> &str { - match *self { - State::Start => "start", - State::Imports => "import", - State::Typedefs => "typedef", - State::Structs => "struct", - State::Constants => "constant", - State::FunctionDefinitions => "function definition", - State::Functions => "extern function", - State::Modules => "module", - } - } -} - -impl Errors { - fn error(&mut self, path: &Path, line: usize, msg: &str) { - self.errs = true; - println!("{}:{}: {}", path.display(), line + 1, msg); - } -} diff --git a/ci/style.sh b/ci/style.sh index c758712012e16..da16bf4fe9baf 100755 --- a/ci/style.sh +++ b/ci/style.sh @@ -9,7 +9,7 @@ if [ -n "${CI:-}" ]; then check="--check" fi -rustc ci/style.rs && ./style src +cargo test --manifest-path libc-test/Cargo.toml --test style -- --nocapture command -v rustfmt rustfmt -V From 94c2b1424af9b69e7dd15b4f3de0f5af3427433d Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Fri, 10 Jan 2025 19:02:42 -0800 Subject: [PATCH 3/3] fix some lints that were detected by the new style checker --- src/teeos/mod.rs | 6 ++--- src/unix/linux_like/linux/mod.rs | 24 +++++++++---------- .../linux_like/linux/uclibc/x86_64/l4re.rs | 1 - src/wasi/mod.rs | 3 +-- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/teeos/mod.rs b/src/teeos/mod.rs index b055d2aca8c7e..1ec2706cfdf76 100644 --- a/src/teeos/mod.rs +++ b/src/teeos/mod.rs @@ -51,9 +51,6 @@ pub type c_long = i64; pub type c_ulong = u64; -#[repr(align(16))] -pub struct _CLongDouble(pub u128); - // long double in C means A float point value, which has 128bit length. // but some bit maybe not used, so the real length of long double could be 80(x86) or 128(power pc/IEEE) // this is different from f128(not stable and not included default) in Rust, so we use u128 for FFI(Rust to C). @@ -88,6 +85,9 @@ pub type wctype_t = c_ulong; pub type cmpfunc = extern "C" fn(x: *const c_void, y: *const c_void) -> c_int; +#[repr(align(16))] +pub struct _CLongDouble(pub u128); + #[repr(align(8))] #[repr(C)] pub struct pthread_cond_t { diff --git a/src/unix/linux_like/linux/mod.rs b/src/unix/linux_like/linux/mod.rs index f17245dfaf43e..5af61377b023f 100644 --- a/src/unix/linux_like/linux/mod.rs +++ b/src/unix/linux_like/linux/mod.rs @@ -2214,18 +2214,6 @@ cfg_if! { } } -cfg_if! { - if #[cfg(all( - any(target_env = "gnu", target_env = "musl", target_env = "ohos"), - any(target_arch = "x86_64", target_arch = "x86") - ))] { - extern "C" { - pub fn iopl(level: c_int) -> c_int; - pub fn ioperm(from: c_ulong, num: c_ulong, turn_on: c_int) -> c_int; - } - } -} - cfg_if! { if #[cfg(any( target_env = "gnu", @@ -6088,6 +6076,18 @@ safe_f! { } } +cfg_if! { + if #[cfg(all( + any(target_env = "gnu", target_env = "musl", target_env = "ohos"), + any(target_arch = "x86_64", target_arch = "x86") + ))] { + extern "C" { + pub fn iopl(level: c_int) -> c_int; + pub fn ioperm(from: c_ulong, num: c_ulong, turn_on: c_int) -> c_int; + } + } +} + cfg_if! { if #[cfg(all(not(target_env = "uclibc"), not(target_env = "ohos")))] { extern "C" { diff --git a/src/unix/linux_like/linux/uclibc/x86_64/l4re.rs b/src/unix/linux_like/linux/uclibc/x86_64/l4re.rs index 7e1499a1fd8bd..b108e77c7cd32 100644 --- a/src/unix/linux_like/linux/uclibc/x86_64/l4re.rs +++ b/src/unix/linux_like/linux/uclibc/x86_64/l4re.rs @@ -28,7 +28,6 @@ s! { } } -#[cfg(target_os = "l4re")] #[allow(missing_debug_implementations)] pub struct pthread_attr_t { pub __detachstate: c_int, diff --git a/src/wasi/mod.rs b/src/wasi/mod.rs index 027f443217a76..750fdfb55fe5d 100644 --- a/src/wasi/mod.rs +++ b/src/wasi/mod.rs @@ -44,6 +44,7 @@ pub type nfds_t = c_ulong; pub type wchar_t = i32; pub type nl_item = c_int; pub type __wasi_rights_t = u64; +pub type locale_t = *mut __locale_struct; s_no_extra_traits! { #[repr(align(16))] @@ -63,8 +64,6 @@ pub enum DIR {} #[cfg_attr(feature = "extra_traits", derive(Debug))] pub enum __locale_struct {} -pub type locale_t = *mut __locale_struct; - s_paren! { // in wasi-libc clockid_t is const struct __clockid* (where __clockid is an opaque struct), // but that's an implementation detail that we don't want to have to deal with