Add two lints looking for new fields in repr(C) unions.

If a union is `repr(C)`, downstream users may rely on bit-validity invariants that may or may not be intended by the union type. As described in #950, we differentiate between two cases here based on whether the bit-validity invariant is reasonably implied by the public API, or whether it might be the product of Hyrum's Law. This PR also discovered and fixes a small bug in the `union_pub_field_now_doc_hidden` lint, which could have a false-positive on fields that are no longer `pub` but never became `#[doc(hidden)]`. Resolves #950.
obi1kenobi · Dec 16, 2024 · f9a09d8 · f9a09d8
1 parent 2fcfccd
commit f9a09d8
Show file tree

Hide file tree

Showing 14 changed files with 842 additions and 0 deletions.
diff --git a/src/lints/union_field_added_with_all_pub_fields.ron b/src/lints/union_field_added_with_all_pub_fields.ron
@@ -0,0 +1,93 @@
+SemverQuery(
+    id: "union_field_added_with_all_pub_fields",
+    human_readable_name: "union with only public API fields added a new field",
+    description: "A union with only public API fields added a new field, possibly changing the union's bit-compatibility.",
+    required_update: Major,
+    lint_level: Deny,
+    reference_link: Some("https://github.com/obi1kenobi/cargo-semver-checks/issues/950"),
+    query: r#"
+    {
+        CrateDiff {
+            baseline {
+                item {
+                    ... on Union {
+                        visibility_limit @filter(op: "=", value: ["$public"])
+
+                        attribute {
+                            content {
+                                base @filter(op: "=", value: ["$repr"])
+                                argument {
+                                    base @filter(op: "=", value: ["$c"])
+                                }
+                            }
+                        }
+
+                        importable_path {
+                            path @output @tag
+                            public_api @filter(op: "=", value: ["$true"])
+                        }
+
+                        # None of the union's fields are non-public-API.
+                        field @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
+                            public_api_eligible @filter(op: "!=", value: ["$true"])
+                        }
+                    }
+                }
+            }
+            current {
+                item {
+                    ... on Union {
+                        visibility_limit @filter(op: "=", value: ["$public"])
+                        union_name: name @output
+
+                        attribute {
+                            content {
+                                base @filter(op: "=", value: ["$repr"])
+                                argument {
+                                    base @filter(op: "=", value: ["$c"])
+                                }
+                            }
+                        }
+
+                        importable_path {
+                            path @filter(op: "=", value: ["%path"])
+                            public_api @filter(op: "=", value: ["$true"])
+                        }
+
+                        field {
+                            field_name: name @output @tag
+
+                            span_: span @optional {
+                                filename @output
+                                begin_line @output
+                            }
+                        }
+                    }
+                }
+            }
+            baseline {
+                item {
+                    ... on Union {
+                        importable_path {
+                            path @filter(op: "=", value: ["%path"])
+                        }
+
+                        # The original union's definition didn't include the field we're looking at.
+                        field @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
+                            name @filter(op: "=", value: ["%field_name"])
+                        }
+                    }
+                }
+            }
+        }
+    }"#,
+    arguments: {
+        "public": "public",
+        "true": true,
+        "zero": 0,
+        "repr": "repr",
+        "c": "C",
+    },
+    error_message: "A public repr(C) union with only public fields has added a new field, which may change the union's bit-compatibility rules. This may invalidate downstream safety invariants and cause those programs to become unsound.",
+    per_result_error_template: Some("field {{union_name}}.{{field_name}} in file {{span_filename}}:{{span_begin_line}}"),
+)
diff --git a/src/lints/union_field_added_with_non_pub_fields.ron b/src/lints/union_field_added_with_non_pub_fields.ron
@@ -0,0 +1,95 @@
+SemverQuery(
+    id: "union_field_added_with_non_pub_fields",
+    human_readable_name: "union with some non-public API fields added a new field",
+    description: "A union with some non-public API fields added a new field, possibly changing the union's bit-compatibility.",
+    required_update: Major,
+    lint_level: Warn,
+    reference_link: Some("https://github.com/obi1kenobi/cargo-semver-checks/issues/950"),
+    query: r#"
+    {
+        CrateDiff {
+            baseline {
+                item {
+                    ... on Union {
+                        visibility_limit @filter(op: "=", value: ["$public"])
+
+                        attribute {
+                            content {
+                                base @filter(op: "=", value: ["$repr"])
+                                argument {
+                                    base @filter(op: "=", value: ["$c"])
+                                }
+                            }
+                        }
+
+                        importable_path {
+                            path @output @tag
+                            public_api @filter(op: "=", value: ["$true"])
+                        }
+
+                        # Some of the union's fields are non-public-API.
+                        # The case where all the union's fields are public API is handled
+                        # in the `union_field_added_with_all_pub_fields` lint.
+                        field @fold @transform(op: "count") @filter(op: ">", value: ["$zero"]) {
+                            public_api_eligible @filter(op: "!=", value: ["$true"])
+                        }
+                    }
+                }
+            }
+            current {
+                item {
+                    ... on Union {
+                        visibility_limit @filter(op: "=", value: ["$public"])
+                        union_name: name @output
+
+                        attribute {
+                            content {
+                                base @filter(op: "=", value: ["$repr"])
+                                argument {
+                                    base @filter(op: "=", value: ["$c"])
+                                }
+                            }
+                        }
+
+                        importable_path {
+                            path @filter(op: "=", value: ["%path"])
+                            public_api @filter(op: "=", value: ["$true"])
+                        }
+
+                        field {
+                            field_name: name @output @tag
+
+                            span_: span @optional {
+                                filename @output
+                                begin_line @output
+                            }
+                        }
+                    }
+                }
+            }
+            baseline {
+                item {
+                    ... on Union {
+                        importable_path {
+                            path @filter(op: "=", value: ["%path"])
+                        }
+
+                        # The original union's definition didn't include the field we're looking at.
+                        field @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) {
+                            name @filter(op: "=", value: ["%field_name"])
+                        }
+                    }
+                }
+            }
+        }
+    }"#,
+    arguments: {
+        "public": "public",
+        "true": true,
+        "zero": 0,
+        "repr": "repr",
+        "c": "C",
+    },
+    error_message: "A public repr(C) union with some non-public fields has added a new field, which may change the union's bit-compatibility rules. While the non-public fields didn't promise any specific bit-compatibility, Hyrum's Law says that downstream users may still have been relying on bit-compatibility assumptions that may now be broken. This may invalidate those users' safety invariants and cause those programs to become unsound.",
+    per_result_error_template: Some("field {{union_name}}.{{field_name}} in file {{span_filename}}:{{span_begin_line}}"),
+)
diff --git a/src/lints/union_pub_field_now_doc_hidden.ron b/src/lints/union_pub_field_now_doc_hidden.ron
@@ -46,7 +46,12 @@ SemverQuery(
 
                         field {
                             name @filter(op: "=", value: ["%field_name"])
+
+                            # We have to ensure that the field is non-public-API
+                            # *by reason of* becoming `#[doc(hidden)]`. Otherwise,
+                            # this lint will trigger on fields becoming non-`pub` as well.
                             public_api_eligible @filter(op: "!=", value: ["$true"])
+                            doc_hidden @filter(op: "=", value: ["$true"])
                         }
 
                         span_: span @optional {

diff --git a/src/query.rs b/src/query.rs
@@ -1218,6 +1218,8 @@ add_lints!(
     tuple_struct_to_plain_struct,
     type_marked_deprecated,
     type_mismatched_generic_lifetimes,
+    union_field_added_with_all_pub_fields,
+    union_field_added_with_non_pub_fields,
     union_field_missing,
     union_missing,
     union_must_use_added,

diff --git a/test_crates/union_field_added/new/Cargo.toml b/test_crates/union_field_added/new/Cargo.toml
@@ -0,0 +1,7 @@
+[package]
+publish = false
+name = "union_field_added"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
diff --git a/test_crates/union_field_added/new/src/lib.rs b/test_crates/union_field_added/new/src/lib.rs
@@ -0,0 +1,90 @@
+// Should trigger `union_field_added_with_all_pub_fields`.
+#[repr(C)]
+pub union AllPubFields {
+    pub a: [i32; 2],
+    pub b: i64,
+    pub c: *const i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`.
+#[repr(C)]
+pub union SomeHiddenPubFields {
+    pub a: [i32; 2],
+
+    #[doc(hidden)]
+    pub b: i64,
+
+    #[doc(hidden)]
+    pub c: *const i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`.
+#[repr(C)]
+pub union SomePrivateFields {
+    pub a: [i32; 2],
+    pub(crate) b: i64,
+    c: *const i64,
+}
+
+// Shouldn't trigger the "union field added" lints,
+// but will trigger the "repr(C) removed" lint for unions.
+pub union ReprCRemovedAllPublicFields {
+    pub a: [i32; 2],
+    pub b: i64,
+    pub c: *const i64,
+}
+
+// Shouldn't trigger the "union field added" lints,
+// but will trigger the "repr(C) removed" lint for unions.
+pub union ReprCRemovedNonPublicFields {
+    pub a: [i32; 2],
+    b: i64,
+    c: *const i64,
+}
+
+// Shouldn't trigger any of the lints.
+#[repr(C)]
+pub union BecameReprC {
+    pub a: [i32; 2],
+    pub b: i64,
+    pub c: *const i64,
+}
+
+// Should trigger `union_field_added_with_all_pub_fields`
+// even though a field also became non-public-API.
+#[repr(C)]
+pub union FieldBecameNonPublic {
+    pub a: [i32; 2],
+    b: i64,
+    c: *const i64,
+}
+
+// Should trigger `union_field_added_with_all_pub_fields`
+// even though a field also became non-public-API.
+#[repr(C)]
+pub union FieldBecameNonPublicAPI {
+    pub a: [i32; 2],
+
+    #[doc(hidden)]
+    pub b: i64,
+
+    pub c: *const i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`
+// even though the non-public-API field also became public API.
+#[repr(C)]
+pub union HiddenFieldBecamePublicAPI {
+    pub a: [i32; 2],
+    pub b: i64,
+    pub c: *const i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`
+// even though the non-public field also became public API.
+#[repr(C)]
+pub union PrivateFieldBecamePublicAPI {
+    pub a: [i32; 2],
+    pub b: i64,
+    pub c: *const i64,
+}
diff --git a/test_crates/union_field_added/old/Cargo.toml b/test_crates/union_field_added/old/Cargo.toml
@@ -0,0 +1,7 @@
+[package]
+publish = false
+name = "union_field_added"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
diff --git a/test_crates/union_field_added/old/src/lib.rs b/test_crates/union_field_added/old/src/lib.rs
@@ -0,0 +1,79 @@
+// Should trigger `union_field_added_with_all_pub_fields`.
+#[repr(C)]
+pub union AllPubFields {
+    pub a: [i32; 2],
+    pub b: i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`.
+#[repr(C)]
+pub union SomeHiddenPubFields {
+    pub a: [i32; 2],
+
+    #[doc(hidden)]
+    pub b: i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`.
+#[repr(C)]
+pub union SomePrivateFields {
+    pub a: [i32; 2],
+    pub(crate) b: i64,
+}
+
+// Shouldn't trigger the "union field added" lints,
+// but will trigger the "repr(C) removed" lint for unions.
+#[repr(C)]
+pub union ReprCRemovedAllPublicFields {
+    pub a: [i32; 2],
+    pub b: i64,
+}
+
+// Shouldn't trigger the "union field added" lints,
+// but will trigger the "repr(C) removed" lint for unions.
+#[repr(C)]
+pub union ReprCRemovedNonPublicFields {
+    pub a: [i32; 2],
+    b: i64,
+}
+
+// Shouldn't trigger any of the lints.
+pub union BecameReprC {
+    pub a: [i32; 2],
+    pub b: i64,
+}
+
+
+// Should trigger `union_field_added_with_all_pub_fields`
+// even though a field also became non-public-API.
+#[repr(C)]
+pub union FieldBecameNonPublic {
+    pub a: [i32; 2],
+    pub b: i64,
+}
+
+// Should trigger `union_field_added_with_all_pub_fields`
+// even though a field also became non-public-API.
+#[repr(C)]
+pub union FieldBecameNonPublicAPI {
+    pub a: [i32; 2],
+    pub b: i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`
+// even though the non-public-API field also became public API.
+#[repr(C)]
+pub union HiddenFieldBecamePublicAPI {
+    pub a: [i32; 2],
+
+    #[doc(hidden)]
+    pub b: i64,
+}
+
+// Should trigger `union_field_added_with_non_pub_fields`
+// even though the non-public field also became public API.
+#[repr(C)]
+pub union PrivateFieldBecamePublicAPI {
+    pub a: [i32; 2],
+    b: i64,
+}