Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lint inherent_associated_pub_const_missing #714

Merged
merged 25 commits into from
Apr 14, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d1ba5a7
Added lint inherent_associated_pub_const_missing
arpity22 Mar 22, 2024
37702f4
fixes
arpity22 Mar 22, 2024
9ba6760
Modifications
arpity22 Mar 22, 2024
0c677cc
Modifications
arpity22 Mar 23, 2024
79adc6d
Merge branch 'obi1kenobi:main' into main
arpity22 Mar 25, 2024
6ff5d23
Adding new test cases
arpity22 Mar 29, 2024
bd81dc1
editing test output
arpity22 Mar 29, 2024
99b523d
Merge branch 'obi1kenobi:main' into main
arpity22 Mar 29, 2024
3995496
Adding new test cases
arpity22 Mar 29, 2024
8042812
Adding new test cases
arpity22 Mar 29, 2024
938db7d
Update src/lints/inherent_associated_pub_const_missing.ron
arpity22 Mar 30, 2024
ea1f008
adding test case
arpity22 Mar 30, 2024
f11a39f
Merge branch 'main' into main
arpity22 Mar 30, 2024
76b2b57
Merge branch 'obi1kenobi:main' into main
arpity22 Mar 30, 2024
90c3172
adding test case
arpity22 Mar 30, 2024
49eb155
Merge branch 'main' of github.com:arpity22/cargo-semver-checks
arpity22 Mar 30, 2024
4cf6944
Update src/lints/inherent_associated_pub_const_missing.ron
arpity22 Mar 31, 2024
4081a40
Merge branch 'main' into main
arpity22 Mar 31, 2024
c4062cf
Merge branch 'main' into main
obi1kenobi Apr 1, 2024
11c7098
Merge branch 'main' into main
obi1kenobi Apr 3, 2024
afb330a
Query Modifications
arpity22 Apr 4, 2024
60b2ed3
Formatting
arpity22 Apr 4, 2024
353d8a8
Merge branch 'main' into main
arpity22 Apr 14, 2024
c42b06c
Apply suggestions from code review
obi1kenobi Apr 14, 2024
7da4ce5
Merge branch 'main' into main
obi1kenobi Apr 14, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions src/lints/inherent_associated_pub_const_missing.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
SemverQuery(
id: "inherent_associated_pub_const_missing",
human_readable_name: "inherent impl's associated pub const removed",
description: "An inherent impl's associated public const removed or renamed",
required_update: Major,
reference_link: Some("https://doc.rust-lang.org/cargo/reference/semver.html#item-remove"),
query: r#"
{
CrateDiff {
baseline {
item {
... on ImplOwner {
visibility_limit @filter(op: "=", value: ["$public"]) @output

importable_path {
path @output @tag
public_api @filter(op: "=", value: ["$true"])
}

inherent_impl {
public_api_eligible @filter(op: "=", value: ["$true"])

associated_constant {
associated_constant: name @output @tag
public_api_eligible @filter(op: "=", value: ["$true"])
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved

span_: span @optional {
filename @output
begin_line @output
}
}
}
}
}
}
current {
item {
... on ImplOwner {
visibility_limit @filter(op: "=", value: ["$public"])
name @output

importable_path {
path @filter(op: "=", value: ["%path"])
public_api @filter(op: "=", value: ["$true"])
}

inherent_impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah one last test case is needed here.

The following might not always be a breaking change, and should be covered by a separate lint:

pub trait Trait {}

pub struct Example;

impl Example {
    pub const N: i64 = 0;
}

impl Trait for Example {}

to

pub trait Trait {
    const N: i64 = 0;
}

pub struct Example;

impl Trait for Example {}

The reason this might not be breaking is that if both Example and Trait are in a prelude, or if the Trait trait is already otherwise in scope, Example::N will continue to work. In the original code that refers to an inherent const, but in the new code it's a trait's const, equivalent to <Example as Trait>::N in explicit form.

To fix this, we allow the query to look at all impl blocks (including impl Trait for Example ones) and not just inherent impl (only impl Example) blocks:

Suggested change
inherent_impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){
impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]){

It would be great if you could add the above code as a test case here.

associated_constant {
name @filter(op: "=", value: ["%associated_constant"])
arpity22 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
}
}
}"#,
arguments: {
"public": "public",
"true": true,
"zero": 0,
},
error_message: "An inherent impl's associated public constant is removed or renamed",
per_result_error_template: Some("{{name}}::{{associated_constant}}, previously at {{span_filename}}:{{span_begin_line}}"),
)
1 change: 1 addition & 0 deletions src/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ macro_rules! add_lints {
}

add_lints!(
inherent_associated_pub_const_missing,
pub_static_now_doc_hidden,
function_abi_no_longer_unwind,
pub_module_level_const_now_doc_hidden,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "inherent_associated_pub_const_missing"
version = "0.1.0"
edition = "2021"

[dependencies]
56 changes: 56 additions & 0 deletions test_crates/inherent_associated_pub_const_missing/new/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Test Cases where #[doc(hidden)] is neither added or removed
pub struct StructA {}

impl StructA {
pub const PublicConstantB: i32 = 0;
// Should Be caught on renaming
pub const PublicConstantRenamedA: i32 = 0;
}

struct StructB {}

impl StructB {
// Should not be caught since StructB is not pub
pub const PublicConstantRenamedB: i32 = 0;
}

#[doc(hidden)]
pub struct StructC {}

impl StructC {
// Should not be caught on removing or renaming since the struct is #[doc(hidden)]
pub const PublicConstantRenamedC: i32 = 0;
}

pub struct StructD {}

#[doc(hidden)]
impl StructD {
pub const PublicConstantE: i32 = 0;
}

// Test Cases where #[doc(hidden)] is added
pub struct StructE {}

#[doc(hidden)]
impl StructE {
pub const PublicConstantH: i32 = 0;
}

#[doc(hidden)]
pub struct StructF {}

impl StructF {
pub const PublicConstantJ: i32 = 0;
}

// Test cases where #[doc(hidden)] is removed
pub struct DocHiddenStruct {}

impl DocHiddenStruct {
pub const PublicConstantL: i32 = 0;
}

impl DocHiddenStruct {
pub const PublicConstantO: i32 = 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "inherent_associated_pub_const_missing"
version = "0.1.0"
edition = "2021"

[dependencies]
80 changes: 80 additions & 0 deletions test_crates/inherent_associated_pub_const_missing/old/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Test Cases where #[doc(hidden)] is neither added or removed
pub struct StructA {}

impl StructA {
// Basic Test case should be caught
pub const PublicConstantA: i32 = 0;
pub const PublicConstantB: i32 = 0;
// Const that was #[doc(hidden)] will be removed
#[doc(hidden)]
pub const DocHiddenConstantA: i32 = 0;
// Should Be caught on renaming
pub const PublicConstantRenameA: i32 = 0;
// Should not be caught on removing since its not pub
const ConstantA: i32 = 0;
}

struct StructB {}

impl StructB {
// Should not be caught since StructB is not pub
pub const PublicConstantC: i32 = 0;
pub const PublicConstantRenameB: i32 = 0;
const ConstantB: i32 = 0;
}

#[doc(hidden)]
pub struct StructC {}

impl StructC {
// Should not be caught on removing or renaming since the struct #[doc(hidden)]
pub const PublicConstantD: i32 = 0;
pub const PublicConstantRenameC: i32 = 0;
}

pub struct StructD {}
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved

#[doc(hidden)]
impl StructD {
pub const PublicConstantE: i32 = 0;
pub const PublicConstantF: i32 = 0;
}
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved

// Test Cases where #[doc(hidden)] is added
pub struct StructE {}

// This impl block will be #[doc(hidden)]
impl StructE {
pub const PublicConstantH: i32 = 0;
// This will be removed
pub const PublicConstantI: i32 = 0;
}

// This struct will be #[doc(hidden)]
pub struct StructF {}

impl StructF {
pub const PublicConstantJ: i32 = 0;
// This const will be removed
pub const PublicConstantK: i32 = 0;
}

// Test cases where #[doc(hidden)] is removed
#[doc(hidden)]
pub struct DocHiddenStruct {}

impl DocHiddenStruct {
// This const will be removed
#[doc(hidden)]
pub const DocHiddenConstantB: i32 = 0;
pub const PublicConstantL: i32 = 0;
// This const will be removed
pub const PublicConstantM: i32 = 0;
}

#[doc(hidden)]
impl DocHiddenStruct {
// This const will be removed
pub const PublicConstantN: i32 = 0;
pub const PublicConstantO: i32 = 0;
}
37 changes: 37 additions & 0 deletions test_outputs/inherent_associated_pub_const_missing.output.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
{
"./test_crates/inherent_associated_pub_const_missing/": [
{
"associated_constant": String("PublicConstantA"),
"name": String("StructA"),
"path": List([
String("inherent_associated_pub_const_missing"),
String("StructA"),
]),
"span_begin_line": Uint64(6),
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
{
"associated_constant": String("PublicConstantRenameA"),
"name": String("StructA"),
"path": List([
String("inherent_associated_pub_const_missing"),
String("StructA"),
]),
"span_begin_line": Uint64(12),
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
{
"associated_constant": String("PublicConstantI"),
"name": String("StructE"),
"path": List([
String("inherent_associated_pub_const_missing"),
String("StructE"),
]),
"span_begin_line": Uint64(50),
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
],
}
11 changes: 11 additions & 0 deletions test_outputs/struct_now_doc_hidden.output.ron
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
{
"./test_crates/inherent_associated_pub_const_missing/": [
{
"path": List([
String("inherent_associated_pub_const_missing"),
String("StructF"),
]),
"span_begin_line": Uint64(41),
"span_filename": String("src/lib.rs"),
"struct_name": String("StructF"),
},
],
"./test_crates/struct_now_doc_hidden/": [
{
"path": List([
Expand Down
Loading