diff --git a/cargo-dylint/tests/integration/fix.rs b/cargo-dylint/tests/integration/fix.rs index e7322d6bf..82601dfe5 100644 --- a/cargo-dylint/tests/integration/fix.rs +++ b/cargo-dylint/tests/integration/fix.rs @@ -10,17 +10,6 @@ use tempfile::tempdir; const CATEGORY: &str = "restriction"; const LIB_NAME: &str = "const_path_join"; -fn workspace_metadata(path_spec: &str) -> String { - format!( - r#" -[workspace.metadata.dylint] -libraries = [ - {{ path = "{path_spec}" }}, -] -"#, - ) -} - const MAIN_RS: &str = r#" fn main() { let _ = std::path::Path::new("..").join("target"); @@ -95,3 +84,14 @@ fn append_workspace_metadata(path: &Path) -> Result<()> { Ok(()) } + +fn workspace_metadata(path_spec: &str) -> String { + format!( + r#" +[workspace.metadata.dylint] +libraries = [ + {{ path = "{path_spec}" }}, +] +"#, + ) +} diff --git a/dylint/src/driver_builder.rs b/dylint/src/driver_builder.rs index 8e6d2aa45..313379d4b 100644 --- a/dylint/src/driver_builder.rs +++ b/dylint/src/driver_builder.rs @@ -23,32 +23,6 @@ Deleting this directory will cause Dylint to rebuild the drivers the next time it needs them, but will have no ill effects. "; -fn cargo_toml(toolchain: &str, dylint_driver_spec: &str) -> String { - format!( - r#" -[package] -name = "dylint_driver-{toolchain}" -version = "0.1.0" -edition = "2018" - -[dependencies] -anyhow = "1.0" -env_logger = "0.11" -dylint_driver = {{ {dylint_driver_spec} }} -"#, - ) -} - -fn rust_toolchain(toolchain: &str) -> String { - format!( - r#" -[toolchain] -channel = "{toolchain}" -components = ["llvm-tools-preview", "rustc-dev"] -"#, - ) -} - // smoelius: We need `#![feature(rustc_private)]` as it changes `dylib` linking behavior and allows // us to link to `rustc_driver`. See: https://github.com/rust-lang/rust/pull/122362 const MAIN_RS: &str = r" @@ -238,6 +212,32 @@ fn initialize(toolchain: &str, package: &Path) -> Result<()> { Ok(()) } +fn cargo_toml(toolchain: &str, dylint_driver_spec: &str) -> String { + format!( + r#" +[package] +name = "dylint_driver-{toolchain}" +version = "0.1.0" +edition = "2018" + +[dependencies] +anyhow = "1.0" +env_logger = "0.11" +dylint_driver = {{ {dylint_driver_spec} }} +"#, + ) +} + +fn rust_toolchain(toolchain: &str) -> String { + format!( + r#" +[toolchain] +channel = "{toolchain}" +components = ["llvm-tools-preview", "rustc-dev"] +"#, + ) +} + #[allow(clippy::unwrap_used)] #[cfg(test)] mod test { diff --git a/dylint/src/lib.rs b/dylint/src/lib.rs index 76d23638a..7f8ee4317 100644 --- a/dylint/src/lib.rs +++ b/dylint/src/lib.rs @@ -308,57 +308,6 @@ fn name_as_path(name: &str, as_path_only: bool) -> Result Result<()> { - for (toolchain, paths) in resolved { - for path in paths { - let driver = driver_builder::get(opts, toolchain)?; - let dylint_libs = serde_json::to_string(&[path])?; - let (name, _) = - parse_path_filename(path).ok_or_else(|| anyhow!("Could not parse path"))?; - - print!("{name}"); - if resolved.keys().len() >= 2 { - print!("@{toolchain}"); - } - if paths.len() >= 2 { - let location = display_location(path)?; - print!(" ({location})"); - } - println!(); - - // smoelius: `-W help` is the normal way to list lints, so we can be sure it - // gets the lints loaded. However, we don't actually use it to list the lints. - let mut command = dylint_driver(toolchain, &driver)?; - command - .envs([ - (env::DYLINT_LIBS, dylint_libs.as_str()), - (env::DYLINT_LIST, "1"), - ]) - .args(["rustc", "-W", "help"]) - .success()?; - - println!(); - } - } - - Ok(()) -} - -fn display_location(path: &Path) -> Result { - let current_dir = current_dir().with_context(|| "Could not get current directory")?; - let Ok(path_buf) = path.canonicalize() else { - return Ok("".to_owned()); - }; - let parent = path_buf - .parent() - .ok_or_else(|| anyhow!("Could not get parent directory"))?; - Ok(parent - .strip_prefix(¤t_dir) - .unwrap_or(parent) - .to_string_lossy() - .to_string()) -} - fn check_or_fix( opts: &opts::Dylint, check_opts: &opts::Check, @@ -471,6 +420,62 @@ fn check_or_fix( } } +fn list_lints(opts: &opts::Dylint, resolved: &ToolchainMap) -> Result<()> { + for (toolchain, paths) in resolved { + for path in paths { + let driver = driver_builder::get(opts, toolchain)?; + let dylint_libs = serde_json::to_string(&[path])?; + let (name, _) = + parse_path_filename(path).ok_or_else(|| anyhow!("Could not parse path"))?; + + print!("{name}"); + if resolved.keys().len() >= 2 { + print!("@{toolchain}"); + } + if paths.len() >= 2 { + let location = display_location(path)?; + print!(" ({location})"); + } + println!(); + + // smoelius: `-W help` is the normal way to list lints, so we can be sure it + // gets the lints loaded. However, we don't actually use it to list the lints. + let mut command = dylint_driver(toolchain, &driver)?; + command + .envs([ + (env::DYLINT_LIBS, dylint_libs.as_str()), + (env::DYLINT_LIST, "1"), + ]) + .args(["rustc", "-W", "help"]) + .success()?; + + println!(); + } + } + + Ok(()) +} + +fn display_location(path: &Path) -> Result { + let current_dir = current_dir().with_context(|| "Could not get current directory")?; + let Ok(path_buf) = path.canonicalize() else { + return Ok("".to_owned()); + }; + let parent = path_buf + .parent() + .ok_or_else(|| anyhow!("Could not get parent directory"))?; + Ok(parent + .strip_prefix(¤t_dir) + .unwrap_or(parent) + .to_string_lossy() + .to_string()) +} + +fn clippy_disable_docs_links() -> Result { + let val = env::var(env::CLIPPY_DISABLE_DOCS_LINKS).ok(); + serde_json::to_string(&val).map_err(Into::into) +} + fn target_dir(opts: &opts::Dylint, toolchain: &str) -> Result { let mut command = MetadataCommand::new(); if let Some(path) = &opts.library_selection().manifest_path { @@ -484,11 +489,6 @@ fn target_dir(opts: &opts::Dylint, toolchain: &str) -> Result { .into()) } -fn clippy_disable_docs_links() -> Result { - let val = env::var(env::CLIPPY_DISABLE_DOCS_LINKS).ok(); - serde_json::to_string(&val).map_err(Into::into) -} - #[allow(clippy::unwrap_used)] #[cfg(all(test, __library_packages))] mod test { @@ -511,10 +511,6 @@ mod test { ..Default::default() }); - fn name_toolchain_map() -> NameToolchainMap<'static> { - NameToolchainMap::new(&OPTS) - } - #[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))] #[test] fn multiple_libraries_multiple_toolchains() { @@ -600,4 +596,8 @@ mod test { run_with_name_toolchain_map(&opts, &name_toolchain_map).unwrap(); } + + fn name_toolchain_map() -> NameToolchainMap<'static> { + NameToolchainMap::new(&OPTS) + } } diff --git a/dylint/src/library_packages/cargo_cli/mod.rs b/dylint/src/library_packages/cargo_cli/mod.rs index f5eeee59b..d863bc9c8 100644 --- a/dylint/src/library_packages/cargo_cli/mod.rs +++ b/dylint/src/library_packages/cargo_cli/mod.rs @@ -255,6 +255,25 @@ publish = false )) } +// smoelius: `ident` is based on the function of the same name at: +// https://github.com/rust-lang/cargo/blob/1a498b6c1c119a79d677553862bffae96b97ad7f/src/cargo/sources/git/source.rs#L136-L147 +#[allow(clippy::manual_next_back)] +fn ident(url: &str) -> Result { + let url = Url::parse(url)?; + + let canonical_url = CanonicalUrl::new(&url)?; + + let ident = canonical_url + .raw_canonicalized_url() + .path_segments() + .and_then(|s| s.rev().next()) + .unwrap_or(""); + + let ident = if ident.is_empty() { "_empty" } else { ident }; + + Ok(format!("{}-{}", ident, short_hash(&canonical_url))) +} + fn inject_dummy_dependencies( dep_path: &Path, dep_name: &str, @@ -296,25 +315,6 @@ fn cargo_metadata(path: &Path) -> Result { .map_err(Into::into) } -// smoelius: `ident` is based on the function of the same name at: -// https://github.com/rust-lang/cargo/blob/1a498b6c1c119a79d677553862bffae96b97ad7f/src/cargo/sources/git/source.rs#L136-L147 -#[allow(clippy::manual_next_back)] -fn ident(url: &str) -> Result { - let url = Url::parse(url)?; - - let canonical_url = CanonicalUrl::new(&url)?; - - let ident = canonical_url - .raw_canonicalized_url() - .path_segments() - .and_then(|s| s.rev().next()) - .unwrap_or(""); - - let ident = if ident.is_empty() { "_empty" } else { ident }; - - Ok(format!("{}-{}", ident, short_hash(&canonical_url))) -} - fn find_accessed_subdir<'a>( dep_name: &str, checkout_path: &Path, diff --git a/dylint/src/library_packages/cargo_cli/util/hex.rs b/dylint/src/library_packages/cargo_cli/util/hex.rs index 979036c5b..3dc29c039 100644 --- a/dylint/src/library_packages/cargo_cli/util/hex.rs +++ b/dylint/src/library_packages/cargo_cli/util/hex.rs @@ -11,16 +11,6 @@ use std::fs::File; use std::hash::{Hash, Hasher}; use std::io::Read; -pub fn to_hex(num: u64) -> String { - hex::encode(num.to_le_bytes()) -} - -pub fn hash_u64(hashable: H) -> u64 { - let mut hasher = StableHasher::new(); - hashable.hash(&mut hasher); - Hasher::finish(&hasher) -} - pub fn hash_u64_file(mut file: &File) -> std::io::Result { let mut hasher = StableHasher::new(); let mut buf = [0; 64 * 1024]; @@ -37,3 +27,13 @@ pub fn hash_u64_file(mut file: &File) -> std::io::Result { pub fn short_hash(hashable: &H) -> String { to_hex(hash_u64(hashable)) } + +pub fn to_hex(num: u64) -> String { + hex::encode(num.to_le_bytes()) +} + +pub fn hash_u64(hashable: H) -> u64 { + let mut hasher = StableHasher::new(); + hashable.hash(&mut hasher); + Hasher::finish(&hasher) +} diff --git a/dylint/src/library_packages/mod.rs b/dylint/src/library_packages/mod.rs index 2fff03f28..eed351abd 100644 --- a/dylint/src/library_packages/mod.rs +++ b/dylint/src/library_packages/mod.rs @@ -127,44 +127,31 @@ pub fn from_opts(opts: &opts::Dylint) -> Result> { library_packages(opts, metadata, &[library]) } -fn to_map_entry(key: &str, value: Option<&String>) -> Option<(String, toml::Value)> { - value - .cloned() - .map(|s| (String::from(key), toml::Value::from(s))) -} - -pub fn from_workspace_metadata(opts: &opts::Dylint) -> Result> { +pub fn from_dylint_toml(opts: &opts::Dylint) -> Result> { if_chain! { if let Some(metadata) = cargo_metadata(opts)?; - if let Some(object) = dylint_metadata(opts)?; + let _ = config::try_init_with_metadata(metadata)?; + if let Some(table) = config::get(); then { - library_packages_from_dylint_metadata(opts, metadata, object) + library_packages_from_dylint_toml(opts, metadata, table) } else { Ok(vec![]) } } } -#[allow(clippy::module_name_repetitions)] -pub fn dylint_metadata(opts: &opts::Dylint) -> Result> { +pub fn from_workspace_metadata(opts: &opts::Dylint) -> Result> { if_chain! { if let Some(metadata) = cargo_metadata(opts)?; - if let serde_json::Value::Object(object) = &metadata.workspace_metadata; - if let Some(value) = object.get("dylint"); + if let Some(object) = dylint_metadata(opts)?; then { - if let serde_json::Value::Object(subobject) = value { - Ok(Some(subobject)) - } else { - bail!("`dylint` value must be a map") - } + library_packages_from_dylint_metadata(opts, metadata, object) } else { - Ok(None) + Ok(vec![]) } } } -static CARGO_METADATA: OnceCell> = OnceCell::new(); - fn cargo_metadata(opts: &opts::Dylint) -> Result> { CARGO_METADATA .get_or_try_init(|| { @@ -202,6 +189,32 @@ fn cargo_metadata(opts: &opts::Dylint) -> Result> { .map(Option::as_ref) } +fn to_map_entry(key: &str, value: Option<&String>) -> Option<(String, toml::Value)> { + value + .cloned() + .map(|s| (String::from(key), toml::Value::from(s))) +} + +#[allow(clippy::module_name_repetitions)] +pub fn dylint_metadata(opts: &opts::Dylint) -> Result> { + if_chain! { + if let Some(metadata) = cargo_metadata(opts)?; + if let serde_json::Value::Object(object) = &metadata.workspace_metadata; + if let Some(value) = object.get("dylint"); + then { + if let serde_json::Value::Object(subobject) = value { + Ok(Some(subobject)) + } else { + bail!("`dylint` value must be a map") + } + } else { + Ok(None) + } + } +} + +static CARGO_METADATA: OnceCell> = OnceCell::new(); + fn library_packages_from_dylint_metadata( opts: &opts::Dylint, metadata: &'static Metadata, @@ -221,19 +234,6 @@ fn library_packages_from_dylint_metadata( Ok(libraries.into_iter().flatten().collect()) } -pub fn from_dylint_toml(opts: &opts::Dylint) -> Result> { - if_chain! { - if let Some(metadata) = cargo_metadata(opts)?; - let _ = config::try_init_with_metadata(metadata)?; - if let Some(table) = config::get(); - then { - library_packages_from_dylint_toml(opts, metadata, table) - } else { - Ok(vec![]) - } - } -} - fn library_packages_from_dylint_toml( opts: &opts::Dylint, metadata: &'static Metadata, diff --git a/dylint/src/package_options/auto_correct/rewrite/mod.rs b/dylint/src/package_options/auto_correct/rewrite/mod.rs index 393a53b01..147d14fe1 100644 --- a/dylint/src/package_options/auto_correct/rewrite/mod.rs +++ b/dylint/src/package_options/auto_correct/rewrite/mod.rs @@ -201,6 +201,14 @@ pub fn collect_rewrites( Ok(rewrites) } +fn display_commits(commits: &[Commit]) { + for commit in commits { + let short_id = commit.short_id(); + let summary = commit.summary().unwrap_or_default(); + eprintln!("{short_id}: {summary}"); + } +} + // smoelius: You need a `Patch` to get a `DiffHunk`'s lines. So there would be no easy way to write // a `hunks_from_patch` function. See, for example, `hunk_lines` below. fn rewrites_from_patch( @@ -247,21 +255,6 @@ fn rewrites_from_patch( Ok(rewrites) } -fn hunk_lines<'repo>( - patch: &'repo Patch<'repo>, - hunk_idx: usize, - hunk_lines: Range, -) -> Result> { - hunk_lines - .map(|line_of_hunk_u32| -> Result<_> { - let line_of_hunk = usize::try_from(line_of_hunk_u32)?; - let diff_line = patch.line_in_hunk(hunk_idx, line_of_hunk)?; - let line = std::str::from_utf8(diff_line.content())?; - Ok(line) - }) - .collect() -} - /// Returns true if, for any hunk in any patch in the diff, both the number of old lines and the /// number of new lines is three or more. #[allow(dead_code)] @@ -295,10 +288,17 @@ fn hunk_is_refactor(hunk: &DiffHunk) -> bool { hunk.old_lines() >= REFACTOR_THRESHOLD && hunk.new_lines() >= REFACTOR_THRESHOLD } -fn display_commits(commits: &[Commit]) { - for commit in commits { - let short_id = commit.short_id(); - let summary = commit.summary().unwrap_or_default(); - eprintln!("{short_id}: {summary}"); - } +fn hunk_lines<'repo>( + patch: &'repo Patch<'repo>, + hunk_idx: usize, + hunk_lines: Range, +) -> Result> { + hunk_lines + .map(|line_of_hunk_u32| -> Result<_> { + let line_of_hunk = usize::try_from(line_of_hunk_u32)?; + let diff_line = patch.line_in_hunk(hunk_idx, line_of_hunk)?; + let line = std::str::from_utf8(diff_line.content())?; + Ok(line) + }) + .collect() } diff --git a/examples/README.md b/examples/README.md index d9e23f717..b9193a707 100644 --- a/examples/README.md +++ b/examples/README.md @@ -47,6 +47,7 @@ The example libraries are separated into the following three categories: | [`env_literal`](./restriction/env_literal) | Environment variables referred to with string literals | | [`inconsistent_qualification`](./restriction/inconsistent_qualification) | Inconsistent qualification of module items | | [`misleading_variable_name`](./restriction/misleading_variable_name) | Variables whose names suggest they have types other than the ones they have | +| [`non_topologically_sorted_functions`](./restriction/non_topologically_sorted_functions) | A lint to check the function order | | [`question_mark_in_expression`](./restriction/question_mark_in_expression) | The `?` operator in expressions | | [`ref_aware_redundant_closure_for_method_calls`](./restriction/ref_aware_redundant_closure_for_method_calls) | A ref-aware fork of `redundant_closure_for_method_calls` | | [`register_lints_warn`](./restriction/register_lints_warn) | Calls to `rustc_errors::DiagCtxtHandle::warn` from within a `register_lints` function | diff --git a/examples/general/crate_wide_allow/src/lib.rs b/examples/general/crate_wide_allow/src/lib.rs index 93b30a1ed..c4d7e503d 100644 --- a/examples/general/crate_wide_allow/src/lib.rs +++ b/examples/general/crate_wide_allow/src/lib.rs @@ -80,18 +80,31 @@ mod test { sync::{LazyLock, Mutex, MutexGuard}, }; - fn mutex>>() -> T::Output { - static MUTEX: Mutex<()> = Mutex::new(()); - - let lock = MUTEX.lock().unwrap(); + /// Verify that `allow`ing a lint in the manifest does not silently override `--deny`. + #[test] + fn premise_manifest_deny() { + mutex::(); - // smoelius: Ensure the `clippy` component is installed. - Command::new("rustup") - .args(["component", "add", "clippy"]) + let mut command = Command::new("cargo"); + command.args(["clippy", "--", "--deny=clippy::assertions-on-constants"]); + command.current_dir("ui_manifest"); + command .assert() - .success(); + .failure() + .stderr(predicate::str::contains(ASSERTIONS_ON_CONSTANTS_WARNING)); + } - T::maybe_return(lock) + #[test] + fn premise_manifest_sanity() { + mutex::(); + + let mut command = Command::new("cargo"); + command.args(["clippy"]); + command.current_dir("ui_manifest"); + command + .assert() + .success() + .stderr(predicate::str::contains(ASSERTIONS_ON_CONSTANTS_WARNING).not()); } #[test] @@ -160,19 +173,6 @@ mod test { const ASSERTIONS_ON_CONSTANTS_WARNING: &str = "`assert!(true)` will be optimized out by the compiler"; - #[test] - fn premise_manifest_sanity() { - mutex::(); - - let mut command = Command::new("cargo"); - command.args(["clippy"]); - command.current_dir("ui_manifest"); - command - .assert() - .success() - .stderr(predicate::str::contains(ASSERTIONS_ON_CONSTANTS_WARNING).not()); - } - /// Verify that `allow`ing a lint in the manifest does not silently override `--warn`. #[test] fn premise_manifest_warn() { @@ -187,18 +187,18 @@ mod test { .stderr(predicate::str::contains(ASSERTIONS_ON_CONSTANTS_WARNING)); } - /// Verify that `allow`ing a lint in the manifest does not silently override `--deny`. - #[test] - fn premise_manifest_deny() { - mutex::(); + fn mutex>>() -> T::Output { + static MUTEX: Mutex<()> = Mutex::new(()); - let mut command = Command::new("cargo"); - command.args(["clippy", "--", "--deny=clippy::assertions-on-constants"]); - command.current_dir("ui_manifest"); - command + let lock = MUTEX.lock().unwrap(); + + // smoelius: Ensure the `clippy` component is installed. + Command::new("rustup") + .args(["component", "add", "clippy"]) .assert() - .failure() - .stderr(predicate::str::contains(ASSERTIONS_ON_CONSTANTS_WARNING)); + .success(); + + T::maybe_return(lock) } mod maybe_return { diff --git a/examples/restriction/Cargo.lock b/examples/restriction/Cargo.lock index 57fde2128..cc22125c5 100644 --- a/examples/restriction/Cargo.lock +++ b/examples/restriction/Cargo.lock @@ -854,6 +854,16 @@ dependencies = [ "heck", ] +[[package]] +name = "non_topologically_sorted_functions" +version = "5.0.0" +dependencies = [ + "clippy_utils", + "dylint_internal", + "dylint_linting", + "dylint_testing", +] + [[package]] name = "num_cpus" version = "1.17.0" diff --git a/examples/restriction/const_path_join/src/lib.rs b/examples/restriction/const_path_join/src/lib.rs index 0cad25003..d939a2caa 100644 --- a/examples/restriction/const_path_join/src/lib.rs +++ b/examples/restriction/const_path_join/src/lib.rs @@ -137,6 +137,21 @@ fn collect_components<'tcx>( (components_reversed, ty_or_partial_span) } +fn is_lit_string(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { + if !expr.span.from_expansion() + && let ExprKind::Lit(lit) = &expr.kind + && let LitKind::Str(symbol, _) = lit.node + // smoelius: I don't think the next line should be necessary. But following the upgrade to + // nightly-2023-08-24, `expr.span.from_expansion()` above started returning false for + // `env!(...)`. + && snippet_opt(cx, expr.span) == Some(format!(r#""{}""#, symbol.as_str())) + { + Some(symbol.to_ident_string()) + } else { + None + } +} + fn is_path_buf_from( cx: &LateContext<'_>, callee: &Expr<'_>, @@ -159,21 +174,6 @@ fn is_path_buf_from( } } -fn is_lit_string(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { - if !expr.span.from_expansion() - && let ExprKind::Lit(lit) = &expr.kind - && let LitKind::Str(symbol, _) = lit.node - // smoelius: I don't think the next line should be necessary. But following the upgrade to - // nightly-2023-08-24, `expr.span.from_expansion()` above started returning false for - // `env!(...)`. - && snippet_opt(cx, expr.span) == Some(format!(r#""{}""#, symbol.as_str())) - { - Some(symbol.to_ident_string()) - } else { - None - } -} - #[test] fn ui() { dylint_testing::ui_test_example(env!("CARGO_PKG_NAME"), "ui"); diff --git a/examples/restriction/non_topologically_sorted_functions/Cargo.toml b/examples/restriction/non_topologically_sorted_functions/Cargo.toml new file mode 100644 index 000000000..31316237c --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/Cargo.toml @@ -0,0 +1,25 @@ +[package] +name = "non_topologically_sorted_functions" +version = "5.0.0" +authors = ["J4CK VVH173 "] +description = "A lint to check the function order" +edition = "2024" +publish = false + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } + +dylint_internal = { path = "../../../internal", features = ["match_def_path"] } +dylint_linting = { path = "../../../utils/linting" } + +[dev-dependencies] +dylint_testing = { path = "../../../utils/testing" } + +[lints] +workspace = true + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/examples/restriction/non_topologically_sorted_functions/README.md b/examples/restriction/non_topologically_sorted_functions/README.md new file mode 100644 index 000000000..c6aa42f3e --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/README.md @@ -0,0 +1,29 @@ +# non_topologically_sorted_functions + +### What it does + +It enforces a certain relative order among functions defined within a module. + +### Why is this bad? + +Without a certain order it's really bad to navigate through the modules. + +### Example + +```rust +fn bar() { } + +fn foo() { + bar(); +} +``` + +Use instead: + +```rust +fn foo() { + bar(); +} + +fn bar() { } +``` diff --git a/examples/restriction/non_topologically_sorted_functions/src/lib.rs b/examples/restriction/non_topologically_sorted_functions/src/lib.rs new file mode 100644 index 000000000..9a84ef8e1 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/src/lib.rs @@ -0,0 +1,312 @@ +#![feature(rustc_private)] +#![warn(unused_extern_crates)] + +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_hir::def::Res; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::intravisit::{self, Visitor}; +use rustc_hir::{BodyId, Expr, ExprKind, HirId, Item, ItemKind, Mod}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::Span; +use std::collections::{HashMap, HashSet}; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// It enforces a certain relative order among functions defined within a module. + /// + /// ### Why is this bad? + /// + /// Without a certain order it's really bad to navigate through the modules. + /// + /// ### Example + /// + /// ```rust + /// fn bar() { } + /// + /// fn foo() { + /// bar(); + /// } + /// ``` + /// + /// Use instead: + /// + /// ```rust + /// fn foo() { + /// bar(); + /// } + /// + /// fn bar() { } + /// ``` + pub NON_TOPOLOGICALLY_SORTED_FUNCTIONS, + Warn, + "Enforce callers before callees and consistent order of callees (module-local functions)" +} + +struct Callee { + pub caller_local_def_id: LocalDefId, + pub call_span: Span, +} + +struct Finder<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + seen: HashSet, + order: Vec, +} + +impl<'tcx> Visitor<'tcx> for Finder<'_, 'tcx> { + fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { + if let ExprKind::Call(callee, _args) = &ex.kind + && let ExprKind::Path(ref qpath) = callee.kind + && let res = self.cx.qpath_res(qpath, callee.hir_id) + && let Res::Def(_, def_id) = res + && let Some(local_def_id) = def_id.as_local() + && !self.seen.contains(&local_def_id) + { + self.seen.insert(local_def_id); + self.order.push(Callee { + caller_local_def_id: local_def_id, + call_span: ex.span, + }); // capture call span + } + + // keep traversing + intravisit::walk_expr(self, ex); + } +} + +impl NonTopologicallySortedFunctions { + fn collect_callees_in_body(cx: &LateContext<'_>, body_id: BodyId) -> Vec { + let body = cx.tcx.hir_body(body_id); + let mut finder = Finder { + cx, + seen: HashSet::new(), + order: Vec::new(), + }; + intravisit::walk_body(&mut finder, body); + finder.order + } + + /// Collect all funcs in caller's body and place them like (caller -> (callee, `call_span`)) + fn build_caller_callee_constraint( + caller_id: LocalDefId, + callees: &[Callee], + mut must_come_before: HashSet<(LocalDefId, LocalDefId)>, + call_sites: &mut HashMap<(LocalDefId, LocalDefId), Vec>, + ) -> HashSet<(LocalDefId, LocalDefId)> { + for &Callee { + caller_local_def_id, + call_span, + } in callees + { + // (caller -> callee) constraint + // If the reverse constraint already exists (added by an earlier caller), + // we keep the earlier constraint (because we iterate callers in module order). + if must_come_before.contains(&(caller_local_def_id, caller_id)) { + // reversed constraint exists; skip adding (precedence kept) + } else { + must_come_before.insert((caller_id, caller_local_def_id)); + } + + // record call site (we still keep all call sites, they may be useful) + call_sites + .entry((caller_id, caller_local_def_id)) + .or_default() + .push(call_span); + } + must_come_before + } + + /// Check inner order rule. + /// + /// The earlier order is preferred and is considered the main one. + fn build_multiple_precedence_rule( + callees: &[Callee], + mut must_come_before: HashSet<(LocalDefId, LocalDefId)>, + ) -> HashSet<(LocalDefId, LocalDefId)> { + // We only need the LocalDefId ordering here (ignore spans) + let ids: Vec = callees + .iter() + .map( + |&Callee { + caller_local_def_id, + .. + }| caller_local_def_id, + ) + .collect(); + for i in 0..ids.len() { + for j in (i + 1)..ids.len() { + let a = ids[i]; + let b = ids[j]; + // prefer earlier constraint: if (b,a) already exists, skip + if must_come_before.contains(&(b, a)) { + // earlier caller already set reversed ordering; keep it. + continue; + } + must_come_before.insert((a, b)); + } + } + must_come_before + } + + fn find_violations( + cx: &LateContext<'_>, + must_come_before: &HashSet<(LocalDefId, LocalDefId)>, + functions: &HashMap, + ) -> Vec { + let mut violations: Vec = must_come_before + .iter() + .filter_map(|&(a, b)| { + let span_a = functions.get(&a)?; + let span_b = functions.get(&b)?; + if span_a.lo() > span_b.hi() { + let span = *span_a; + let name_a = cx.tcx.def_path_str(a.to_def_id()); + let name_b = cx.tcx.def_path_str(b.to_def_id()); + let violation = Violation { + span, + id_first_fn: a, + id_second_fn: b, + name_first_fn: name_a, + name_second_fn: name_b, + }; + Some(violation) + } else { + None + } + }) + .collect(); + + // keep the same order: sort deterministically by span.lo, span.hi, name + violations.sort_by( + |Violation { + name_first_fn: name_a, + span: span_a, + .. + }, + Violation { + name_first_fn: name_b, + span: span_b, + .. + }| { + span_a + .lo() + .cmp(&span_b.lo()) + .then(span_a.hi().cmp(&span_b.hi())) + .then(name_a.as_str().cmp(name_b.as_str())) + }, + ); + + violations + } +} + +#[derive(Debug, Clone)] +struct Violation { + name_first_fn: String, + name_second_fn: String, + id_first_fn: LocalDefId, + id_second_fn: LocalDefId, + span: Span, +} + +impl<'tcx> LateLintPass<'tcx> for NonTopologicallySortedFunctions { + fn check_mod(&mut self, cx: &LateContext<'tcx>, module: &'tcx Mod<'tcx>, _module_id: HirId) { + // Collect top-level functions + let mut def_order: Vec = vec![]; + let mut functions: HashMap = HashMap::new(); + + for item_id in module.item_ids { + let item: &Item<'tcx> = cx.tcx.hir_item(*item_id); + if let ItemKind::Fn { .. } = item.kind { + let local_def_id = item.owner_id.def_id; + + def_order.push(local_def_id); + functions.insert(local_def_id, item.span); + } + } + + if def_order.len() < 2 { + return; + } + + let mut must_come_before: HashSet<(LocalDefId, LocalDefId)> = HashSet::new(); + // stores all call sites for (caller, callee) + let mut call_sites: HashMap<(LocalDefId, LocalDefId), Vec> = HashMap::new(); + + for caller_id in def_order { + // use hir_maybe_body_owned_by — works for functions and methods etc. + let caller_body = cx.tcx.hir_maybe_body_owned_by(caller_id); + + if let Some(caller_body) = caller_body { + let caller_body_id = caller_body.id(); + let callees: Vec = Self::collect_callees_in_body(cx, caller_body_id); + + must_come_before = Self::build_caller_callee_constraint( + caller_id, + &callees, + must_come_before, + &mut call_sites, + ); + must_come_before = Self::build_multiple_precedence_rule(&callees, must_come_before); + } + } + + let violations = Self::find_violations(cx, &must_come_before, &functions); + let mut warned: HashSet = HashSet::new(); + + for violation in violations { + let Violation { + name_first_fn, + name_second_fn, + id_first_fn, + id_second_fn, + span, + .. + } = violation; + if warned.insert(id_first_fn) { + cx.span_lint(NON_TOPOLOGICALLY_SORTED_FUNCTIONS, span, |diag| { + // PRIMARY ERROR + diag.span_label( + span, + format!( + "function `{name_first_fn}` should be defined before `{name_second_fn}`" + ), + ); + + diag.help( + "move the function earlier in the module so callers and callee ordering is respected", + ); + + // search call sites + if let Some(sites) = call_sites.get(&(id_first_fn, id_second_fn)) { + let sm = cx.sess().source_map(); + let good_sites: Vec = sites + .iter() + .copied() + .filter(|s| !s.from_expansion() && !s.in_external_macro(sm)) + .collect(); + + if let Some(call_span) = good_sites.first() { + // НОВОЕ: extra block with double info + diag.span_note( + *call_span, + format!("`{name_second_fn}` is called from `{name_first_fn}` here"), + ); + } + } else { + // if the second rule + diag.note("no call site recorded (internal)"); + } + }); + } + } + } +} + +#[test] +fn ui() { + dylint_testing::ui_test(env!("CARGO_PKG_NAME"), "ui"); +} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/external.rs b/examples/restriction/non_topologically_sorted_functions/ui/external.rs new file mode 100644 index 000000000..1724d19f3 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/external.rs @@ -0,0 +1,5 @@ +use std::cmp::max; + +fn main() { + let m = max(1, 2); +} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/main.rs b/examples/restriction/non_topologically_sorted_functions/ui/main.rs new file mode 100644 index 000000000..cb0fb2b98 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/main.rs @@ -0,0 +1,13 @@ +fn main() { + foo(); +} + +fn foo() { + bar(); +} + +fn bar() { + let t_struct = TestStruct; +} + +struct TestStruct; diff --git a/examples/restriction/non_topologically_sorted_functions/ui/multiple_correct.rs b/examples/restriction/non_topologically_sorted_functions/ui/multiple_correct.rs new file mode 100644 index 000000000..0cbc65ed0 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/multiple_correct.rs @@ -0,0 +1,15 @@ +fn main() {} + +fn foo() { + bar(); + baz(); +} + +fn qux() { + baz(); + bar(); +} + +fn bar() {} + +fn baz() {} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/multiple_incorrect.rs b/examples/restriction/non_topologically_sorted_functions/ui/multiple_incorrect.rs new file mode 100644 index 000000000..d4ace433d --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/multiple_incorrect.rs @@ -0,0 +1,15 @@ +fn main() {} + +fn foo() { + bar(); + baz(); +} + +fn qux() { + baz(); + bar(); +} + +fn baz() {} + +fn bar() {} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/multiple_incorrect.stderr b/examples/restriction/non_topologically_sorted_functions/ui/multiple_incorrect.stderr new file mode 100644 index 000000000..e41885772 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/multiple_incorrect.stderr @@ -0,0 +1,12 @@ +warning: + --> $DIR/multiple_incorrect.rs:15:1 + | +LL | fn bar() {} + | ^^^^^^^^^^^ function `bar` should be defined before `baz` + | + = help: move the function earlier in the module so callers and callee ordering is respected + = note: no call site recorded (internal) + = note: `#[warn(non_topologically_sorted_functions)]` on by default + +warning: 1 warning emitted + diff --git a/examples/restriction/non_topologically_sorted_functions/ui/recursion.rs b/examples/restriction/non_topologically_sorted_functions/ui/recursion.rs new file mode 100644 index 000000000..69df911ac --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/recursion.rs @@ -0,0 +1,11 @@ +fn main() { + bar(); +} + +fn bar() { + foo(); +} + +fn foo() { + bar(); +} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/simple_wrong_order.rs b/examples/restriction/non_topologically_sorted_functions/ui/simple_wrong_order.rs new file mode 100644 index 000000000..42fcbc93e --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/simple_wrong_order.rs @@ -0,0 +1,9 @@ +fn main() { + foo(); +} + +fn bar() {} + +fn foo() { + bar(); +} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/simple_wrong_order.stderr b/examples/restriction/non_topologically_sorted_functions/ui/simple_wrong_order.stderr new file mode 100644 index 000000000..4f6ba7069 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/simple_wrong_order.stderr @@ -0,0 +1,18 @@ +warning: + --> $DIR/simple_wrong_order.rs:7:1 + | +LL | / fn foo() { +LL | | bar(); +LL | | } + | |_^ function `foo` should be defined before `bar` + | + = help: move the function earlier in the module so callers and callee ordering is respected +note: `bar` is called from `foo` here + --> $DIR/simple_wrong_order.rs:8:5 + | +LL | bar(); + | ^^^^^ + = note: `#[warn(non_topologically_sorted_functions)]` on by default + +warning: 1 warning emitted + diff --git a/examples/restriction/non_topologically_sorted_functions/ui/wrong_inner_call.rs b/examples/restriction/non_topologically_sorted_functions/ui/wrong_inner_call.rs new file mode 100644 index 000000000..2ef55cfc4 --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/wrong_inner_call.rs @@ -0,0 +1,14 @@ +fn main() { + foo(); +} + +fn baz() {} + +fn foo() { + bar(); + { + baz(); + } +} + +fn bar() {} diff --git a/examples/restriction/non_topologically_sorted_functions/ui/wrong_inner_call.stderr b/examples/restriction/non_topologically_sorted_functions/ui/wrong_inner_call.stderr new file mode 100644 index 000000000..0ed20b2ba --- /dev/null +++ b/examples/restriction/non_topologically_sorted_functions/ui/wrong_inner_call.stderr @@ -0,0 +1,30 @@ +warning: + --> $DIR/wrong_inner_call.rs:7:1 + | +LL | / fn foo() { +LL | | bar(); +LL | | { +LL | | baz(); +LL | | } +LL | | } + | |_^ function `foo` should be defined before `baz` + | + = help: move the function earlier in the module so callers and callee ordering is respected +note: `baz` is called from `foo` here + --> $DIR/wrong_inner_call.rs:10:9 + | +LL | baz(); + | ^^^^^ + = note: `#[warn(non_topologically_sorted_functions)]` on by default + +warning: + --> $DIR/wrong_inner_call.rs:14:1 + | +LL | fn bar() {} + | ^^^^^^^^^^^ function `bar` should be defined before `baz` + | + = help: move the function earlier in the module so callers and callee ordering is respected + = note: no call site recorded (internal) + +warning: 2 warnings emitted +