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.

Sign up for GitHub

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 pub_static_mut_now_immutable #768

Merged
merged 8 commits into from
Apr 23, 2024
Merged

Add lint pub_static_mut_now_immutable #768

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb6d449
Add lint pub_static_mut_now_immutable
arpity22 Apr 20, 2024
ff8056c
Add lint pub_static_mut_now_immutable
arpity22 Apr 20, 2024
8662bd1
Modifications in error message and description
arpity22 Apr 20, 2024
2e9783a
Merge branch 'obi1kenobi:main' into main
arpity22 Apr 23, 2024
d4d33e2
Modifications in description and error message
arpity22 Apr 23, 2024
272b496
Modifications in description and error message
arpity22 Apr 23, 2024
98ad49f
Modifications in description and error message
arpity22 Apr 23, 2024
9511ae8
Merge branch 'main' of github.com:arpity22/cargo-semver-checks
arpity22 Apr 23, 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
50 changes: 50 additions & 0 deletions src/lints/pub_static_mut_now_immutable.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
SemverQuery(
id: "pub_static_mut_now_immutable",
human_readable_name: "pub static mut is now immutable",
description: "A mutable static became immutable and thus cannot be imported",
Copy link
Owner

Choose a reason for hiding this comment

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

Is imported here intentional? I admit I haven't looked too closely at what the exact breakage is when a mutable static becomes immutable. I just want to make sure that it is indeed the use import statement that will trigger a compilation error if a static becomes immutable, since that seems a bit surprising to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure either. I just read the discussion on the issue and figured the user would not be able to use the static since they depended on it being mutable.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah okay. Let's step back for a sec, since I think the bigger picture is more important right now.

cargo-semver-checks exists as a tool because the rules for SemVer in Rust are very complex. We humans have a hard time learning them and applying them consistently, so we delegate some of that responsibility to this tool: it can be more consistent, and it also holds the distilled expertise of dozens of humans. Each person there dug into some possible SemVer problem, understood it in detail, figured out how to prevent it from ever happening again, and then wrote a lint that both catches and describes the problem to the end user.

If some of the distilled expertise is wrong, that violates users' trust. Why should they delegate trust and responsibility to a tool that violates their trust? They will stop using it.

This is an existential threat to cargo-semver-checks. This is not exaggeration. Several prior attempts at a SemVer linter for Rust have been abandoned due to reporting incorrect information, such as false-positive lints. We may very well increment that number by one more if we aren't careful.

The real value you are contributing in PRs that add lints is not the lint queries, the test cases, etc. It is your expertise on that particular facet of the SemVer rules. It happens to be distilled into a lint file with its query and error message etc. but that's just the packaging.

The value is realized when users can say "ah, I would have made a terrible mistake here, but cargo-semver-checks caught it and explained it to me in a way that I understand the problem and its consequences." This doesn't happen if the lint query is wrong. It also doesn't happen if the error message or description. Or if any other metadata in the lint is wrong, like if we mis-categorized a lint as Minor instead of Major.

So please dig into the problem space, win that expertise, and contribute it as part of the lint as a whole. Otherwise, our users don't get any value — in fact, if we teach them something plainly incorrect, they may even be worse off overall!

Copy link
Owner

@obi1kenobi obi1kenobi Apr 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Coming back to this specific instance, the user would not be able to use the static in the same way, since it isn't mutable. But I wouldn't expect the manner of "use" they can't do is "import it."

Obtaining the necessary expertise here is figuring out what manner of "use" is denied to the user that depended on a mutable static that became no longer mutable. You'll know you've gained that expertise when you can write a concrete snippet of Rust that compiles with a mutable static, but has a compile error when the static is no longer mutable.

Copy link
Contributor Author

@arpity22 arpity22 Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I looked into the compile error and it seems compile error is only caused by assigning to the immutable static. So, I updated the error message and description accordingly.

P.S: I was looking through issue #767 and wanted to ask if is there any documentation on lint optimization I should go through for future lints? Its nothing urgent and I doubt I would be able to go through them this week.

Copy link
Owner

Choose a reason for hiding this comment

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

Great!

For lint optimization, there's not much one can do within the lints themselves. There's room for minor improvements like "order edges such that filtering happens as early as possible" but that won't make a big difference for cases like #767. Don't worry, you didn't cause that issue and neither did anyone else writing lints. It'll get resolved, and it won't require any changes to the lints themselves to do so.

The cause of that issue is a missing optimization in the Trustfall query engine adapter for rustdoc. If you are curious, you can read more about how I resolved a previous instance of the same issue here: https://predr.ag/blog/speeding-up-rust-semver-checking-by-over-2000x/

required_update: Major,
reference_link: Some("https://doc.rust-lang.org/reference/items/static-items.html"),
query: r#"
{
CrateDiff {
baseline {
item {
... on Static {
visibility_limit @filter(op: "=", value: ["$public"])
mutable @filter(op: "=", value: ["$true"])

importable_path {
path @output @tag
public_api @filter(op: "=", value: ["$true"])
}
}
}
}
current {
item {
... on Static {
visibility_limit @filter(op: "=", value: ["$public"])
mutable @filter(op: "!=", value: ["$true"])
static_name: name @output

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

span_: span @optional {
filename @output
begin_line @output
}
}
}
}
}
}"#,
arguments: {
"public": "public",
"true": true,
},
error_message: "A mutable static is now immutable",
Copy link
Owner

Choose a reason for hiding this comment

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

We may want to expand this error message a bit.

If you just ran cargo-semver-checks and it reported this problem, what information would you like to see printed as part of the error log? You'd like to know what happened, why it's a problem, and where to look for it -- in as few characters as possible. This is tough, but it's a skill like any other and practicing it will help.

per_result_error_template: Some("{{static_name}} in file {{span_filename}}:{{span_begin_line}}"),
)
1 change: 1 addition & 0 deletions src/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,7 @@ add_lints!(
pub_module_level_const_missing,
pub_module_level_const_now_doc_hidden,
pub_static_missing,
pub_static_mut_now_immutable,
pub_static_now_doc_hidden,
repr_c_removed,
repr_packed_added,
Expand Down
7 changes: 7 additions & 0 deletions test_crates/pub_static_mut_now_immutable/new/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "pub_static_mut_now_immutable"
version = "0.1.0"
edition = "2021"

[dependencies]
25 changes: 25 additions & 0 deletions test_crates/pub_static_mut_now_immutable/new/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Basic Test cases
pub static STATIC_A: i32 = 0;
pub static mut STATIC_B: i32 = 0;
static STATIC_C: i32 = 0;

// Test case for #[doc(hidden)] pub static
#[doc(hidden)]
pub static DOC_HIDDEN_STATIC_A: i32 = 0;

// Renaming or making a static #[doc(hidden)] along with making it immutable
// should trigger only one lint
#[doc(hidden)]
pub static DOC_HIDDEN_STATIC_B: i32 = 0;
pub static STATIC_RENAMED: i32 = 0;

// Testing for static defined in private module
mod PRIVATE_MODULE {
pub static STATIC_C: i32 = 0;
}

// Testing for static defined in #[doc(hidden)] module
#[doc(hidden)]
pub mod DOC_HIDDEN_MODULE {
pub static STATIC_C: i32 = 0;
}
7 changes: 7 additions & 0 deletions test_crates/pub_static_mut_now_immutable/old/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
publish = false
name = "pub_static_mut_now_immutable"
version = "0.1.0"
edition = "2021"

[dependencies]
24 changes: 24 additions & 0 deletions test_crates/pub_static_mut_now_immutable/old/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Basic Test cases
pub static mut STATIC_A: i32 = 0;
pub static mut STATIC_B: i32 = 0;
static mut STATIC_C: i32 = 0;

// Test case for #[doc(hidden)] pub static
#[doc(hidden)]
pub static mut DOC_HIDDEN_STATIC_A: i32 = 0;

// Renaming or making a static #[doc(hidden)] along with making it immutable
// should trigger only one lint
pub static mut DOC_HIDDEN_STATIC_B: i32 = 0;
pub static mut STATIC_RENAME: i32 = 0;

// Testing for static defined in private module
mod PRIVATE_MODULE {
pub static mut STATIC_C: i32 = 0;
}

// Testing for static defined in #[doc(hidden)] module
#[doc(hidden)]
pub mod DOC_HIDDEN_MODULE {
pub static mut STATIC_C: i32 = 0;
}
12 changes: 12 additions & 0 deletions test_outputs/pub_static_missing.output.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,16 @@
"visibility_limit": String("public"),
},
],
"./test_crates/pub_static_mut_now_immutable/": [
{
"name": String("STATIC_RENAME"),
"path": List([
String("pub_static_mut_now_immutable"),
String("STATIC_RENAME"),
]),
"span_begin_line": Uint64(13),
"span_filename": String("src/lib.rs"),
"visibility_limit": String("public"),
},
],
}
13 changes: 13 additions & 0 deletions test_outputs/pub_static_mut_now_immutable.output.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"./test_crates/pub_static_mut_now_immutable/": [
{
"path": List([
String("pub_static_mut_now_immutable"),
String("STATIC_A"),
]),
"span_begin_line": Uint64(2),
"span_filename": String("src/lib.rs"),
"static_name": String("STATIC_A"),
},
],
}
11 changes: 11 additions & 0 deletions test_outputs/pub_static_now_doc_hidden.output.ron
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
{
"./test_crates/pub_static_mut_now_immutable/": [
{
"path": List([
String("pub_static_mut_now_immutable"),
String("DOC_HIDDEN_STATIC_B"),
]),
"span_begin_line": Uint64(13),
"span_filename": String("src/lib.rs"),
"static_name": String("DOC_HIDDEN_STATIC_B"),
},
],
"./test_crates/pub_static_now_doc_hidden/": [
{
"path": List([
Expand Down
Loading