From 7eeec6180421aa846160c70c5c0d75b740d63a0e Mon Sep 17 00:00:00 2001 From: Predrag Gruevski Date: Sat, 21 Dec 2024 22:57:41 +0000 Subject: [PATCH] Add `trait_method_parameter_count_changed` lint. It catches cases where trait methods change arity, i.e. start requiring a different number of arguments than they used to. This breaks calling and/or implementing those methods. --- .../trait_method_parameter_count_changed.ron | 77 +++++++++++++++++++ src/query.rs | 1 + .../new/Cargo.toml | 7 ++ .../new/src/lib.rs | 5 ++ .../old/Cargo.toml | 7 ++ .../old/src/lib.rs | 5 ++ .../trait_method_parameter_count_changed.snap | 65 ++++++++++++++++ 7 files changed, 167 insertions(+) create mode 100644 src/lints/trait_method_parameter_count_changed.ron create mode 100644 test_crates/trait_method_parameter_count_changed/new/Cargo.toml create mode 100644 test_crates/trait_method_parameter_count_changed/new/src/lib.rs create mode 100644 test_crates/trait_method_parameter_count_changed/old/Cargo.toml create mode 100644 test_crates/trait_method_parameter_count_changed/old/src/lib.rs create mode 100644 test_outputs/query_execution/trait_method_parameter_count_changed.snap diff --git a/src/lints/trait_method_parameter_count_changed.ron b/src/lints/trait_method_parameter_count_changed.ron new file mode 100644 index 00000000..bfe38456 --- /dev/null +++ b/src/lints/trait_method_parameter_count_changed.ron @@ -0,0 +1,77 @@ +SemverQuery( + id: "trait_method_parameter_count_changed", + human_readable_name: "pub trait method parameter count changed", + description: "A trait method requires a different number of parameters than it used to.", + required_update: Major, + lint_level: Deny, + reference_link: Some("https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures"), + query: r#" + { + CrateDiff { + baseline { + item { + ... on Trait { + visibility_limit @filter(op: "=", value: ["$public"]) @output + name @output + + importable_path { + path @output @tag + public_api @filter(op: "=", value: ["$true"]) + } + + method { + method_name: name @output @tag + + # TODO: Once we decide to split trait method lints based on whether the + # trait is sealed (e.g. see `trait_method_missing`), split this lint: + # - Sealed traits should ignore ineligible (~approx. `#[doc(hidden)]`) + # methods since they are indeed never public API. + # - Non-sealed traits should only ignore ineligible methods + # if they have a default implementation; otherwise, implementors + # of the trait have to impl the method regardless of `#[doc(hidden)]`. + public_api_eligible @filter(op: "=", value: ["$true"]) + + old_parameter_: parameter @fold @transform(op: "count") @output @tag(name: "parameters") + } + } + } + } + current { + item { + ... on Trait { + visibility_limit @filter(op: "=", value: ["$public"]) + + importable_path { + path @filter(op: "=", value: ["%path"]) + public_api @filter(op: "=", value: ["$true"]) + } + + method { + name @filter(op: "=", value: ["%method_name"]) + + new_parameter_: parameter @fold @transform(op: "count") @filter(op: "!=", value: ["%parameters"]) @output + + span_: span @optional { + filename @output + begin_line @output + } + } + } + } + } + } + }"#, + arguments: { + "public": "public", + "true": true, + }, + // TODO: This has a false-positive edge case, because it assumes the trait method + // was *previously* callable. This might not be the case for partially-sealed traits + // which can make some methods uncallable. In that case, the method signature change + // is not a breaking change, since it could never have been called in the first place. + // If the trait wasn't sealed and the method didn't have a default impl, then + // the change is still breaking even if the method wasn't callable -- it's just + // on the side of implementing the trait, not calling the method. + error_message: "A trait method now takes a different number of parameters.", + per_result_error_template: Some("{{name}}::{{method_name}} now takes {{new_parameter_count}} instead of {{old_parameter_count}} parameters, in file {{span_filename}}:{{span_begin_line}}"), +) diff --git a/src/query.rs b/src/query.rs index 1e18f5bd..e54ccf6b 100644 --- a/src/query.rs +++ b/src/query.rs @@ -1206,6 +1206,7 @@ add_lints!( trait_method_added, trait_method_missing, trait_method_now_doc_hidden, + trait_method_parameter_count_changed, trait_method_unsafe_added, trait_method_unsafe_removed, trait_mismatched_generic_lifetimes, diff --git a/test_crates/trait_method_parameter_count_changed/new/Cargo.toml b/test_crates/trait_method_parameter_count_changed/new/Cargo.toml new file mode 100644 index 00000000..699814e5 --- /dev/null +++ b/test_crates/trait_method_parameter_count_changed/new/Cargo.toml @@ -0,0 +1,7 @@ +[package] +publish = false +name = "trait_method_parameter_count_changed" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/test_crates/trait_method_parameter_count_changed/new/src/lib.rs b/test_crates/trait_method_parameter_count_changed/new/src/lib.rs new file mode 100644 index 00000000..e60f72ad --- /dev/null +++ b/test_crates/trait_method_parameter_count_changed/new/src/lib.rs @@ -0,0 +1,5 @@ +pub trait Example { + fn gain_a_parameter(x: i64); + + fn lose_a_parameter(); +} diff --git a/test_crates/trait_method_parameter_count_changed/old/Cargo.toml b/test_crates/trait_method_parameter_count_changed/old/Cargo.toml new file mode 100644 index 00000000..699814e5 --- /dev/null +++ b/test_crates/trait_method_parameter_count_changed/old/Cargo.toml @@ -0,0 +1,7 @@ +[package] +publish = false +name = "trait_method_parameter_count_changed" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/test_crates/trait_method_parameter_count_changed/old/src/lib.rs b/test_crates/trait_method_parameter_count_changed/old/src/lib.rs new file mode 100644 index 00000000..56745205 --- /dev/null +++ b/test_crates/trait_method_parameter_count_changed/old/src/lib.rs @@ -0,0 +1,5 @@ +pub trait Example { + fn gain_a_parameter(); + + fn lose_a_parameter(a: i64); +} diff --git a/test_outputs/query_execution/trait_method_parameter_count_changed.snap b/test_outputs/query_execution/trait_method_parameter_count_changed.snap new file mode 100644 index 00000000..df76e3bf --- /dev/null +++ b/test_outputs/query_execution/trait_method_parameter_count_changed.snap @@ -0,0 +1,65 @@ +--- +source: src/query.rs +expression: "&query_execution_results" +snapshot_kind: text +--- +{ + "./test_crates/trait_method_default_impl_removed/": [ + { + "method_name": String("method_default_impl_removed_and_becomes_sealed"), + "name": String("TraitE"), + "new_parameter_count": Uint64(2), + "old_parameter_count": Uint64(1), + "path": List([ + String("trait_method_default_impl_removed"), + String("TraitE"), + ]), + "span_begin_line": Uint64(27), + "span_filename": String("src/lib.rs"), + "visibility_limit": String("public"), + }, + ], + "./test_crates/trait_method_parameter_count_changed/": [ + { + "method_name": String("gain_a_parameter"), + "name": String("Example"), + "new_parameter_count": Uint64(1), + "old_parameter_count": Uint64(0), + "path": List([ + String("trait_method_parameter_count_changed"), + String("Example"), + ]), + "span_begin_line": Uint64(2), + "span_filename": String("src/lib.rs"), + "visibility_limit": String("public"), + }, + { + "method_name": String("lose_a_parameter"), + "name": String("Example"), + "new_parameter_count": Uint64(0), + "old_parameter_count": Uint64(1), + "path": List([ + String("trait_method_parameter_count_changed"), + String("Example"), + ]), + "span_begin_line": Uint64(4), + "span_filename": String("src/lib.rs"), + "visibility_limit": String("public"), + }, + ], + "./test_crates/trait_newly_sealed/": [ + { + "method_name": String("method"), + "name": String("TraitMethodBecomesSealed"), + "new_parameter_count": Uint64(2), + "old_parameter_count": Uint64(1), + "path": List([ + String("trait_newly_sealed"), + String("TraitMethodBecomesSealed"), + ]), + "span_begin_line": Uint64(14), + "span_filename": String("src/lib.rs"), + "visibility_limit": String("public"), + }, + ], +}