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

Check for #[repr(i*)]/#[repr(u*)] semver violations. #29

Merged
merged 6 commits into from
Aug 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions scripts/regenerate_test_rustdocs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ mv "$RUSTDOC_OUTPUT" "$TARGET_DIR/baseline.json"
features=(
'enum_missing'
'enum_repr_c_removed'
'enum_repr_int_changed'
'enum_repr_int_removed'
'enum_variant_added'
'enum_variant_missing'
'function_missing'
Expand Down
2 changes: 2 additions & 0 deletions semver_tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ edition = "2021"

[features]
enum_missing = []
enum_repr_int_changed = []
enum_repr_int_removed = []
enum_repr_c_removed = []
enum_variant_added = []
enum_variant_missing = []
Expand Down
71 changes: 71 additions & 0 deletions semver_tests/src/test_cases/enum_repr_int_changed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#[cfg(not(feature = "enum_repr_int_changed"))]
#[repr(u8)]
pub enum U8ToU16Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_changed")]
#[repr(u16)]
pub enum U8ToU16Enum {
Bar,
Baz,
}

#[cfg(not(feature = "enum_repr_int_changed"))]
#[repr(i32)]
pub enum I32ToI8Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_changed")]
#[repr(i8)]
pub enum I32ToI8Enum {
Bar,
Baz,
}

#[cfg(not(feature = "enum_repr_int_changed"))]
#[repr(i32)]
pub enum I32ToU32Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_changed")]
#[repr(u32)]
pub enum I32ToU32Enum {
Bar,
Baz,
}

// The following enums have *removals* of repr(i*) and repr(u*),
// not changes to another repr(i*) or repr(u*).
// They should not be reported by this rule, because they have their own rule.

#[cfg(not(feature = "enum_repr_int_changed"))]
#[repr(u8)]
pub enum U8Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_changed")]
pub enum U8Enum {
Bar,
Baz,
}

#[cfg(not(feature = "enum_repr_int_changed"))]
#[repr(i32)]
pub enum I32Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_changed")]
pub enum I32Enum {
Bar,
Baz,
}
25 changes: 25 additions & 0 deletions semver_tests/src/test_cases/enum_repr_int_removed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#[cfg(not(feature = "enum_repr_int_removed"))]
#[repr(u8)]
pub enum U8Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_removed")]
pub enum U8Enum {
Bar,
Baz,
}

#[cfg(not(feature = "enum_repr_int_removed"))]
#[repr(i32)]
pub enum I32Enum {
Bar,
Baz,
}

#[cfg(feature = "enum_repr_int_removed")]
pub enum I32Enum {
Bar,
Baz,
}
2 changes: 2 additions & 0 deletions semver_tests/src/test_cases/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
pub mod enum_repr_c_removed;
pub mod enum_repr_int_changed;
pub mod enum_repr_int_removed;
pub mod enum_variant_added;
pub mod enum_variant_missing;
pub mod item_missing;
Expand Down
99 changes: 74 additions & 25 deletions src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ impl Origin {
kind: TokenKind::RawType(raw_type),
}
}

fn make_attribute_token<'a>(&self, attr: &'a str) -> Token<'a> {
Token {
origin: *self,
kind: TokenKind::Attribute(attr),
}
}
}

#[derive(Debug, Clone)]
Expand All @@ -69,15 +76,6 @@ pub struct Token<'a> {
kind: TokenKind<'a>,
}

impl<'a> Token<'a> {
fn new_crate(origin: Origin, crate_: &'a Crate) -> Self {
Self {
origin,
kind: TokenKind::Crate(crate_),
}
}
}

#[derive(Debug, Clone)]
pub enum TokenKind<'a> {
CrateDiff((&'a Crate, &'a Crate)),
Expand All @@ -86,10 +84,18 @@ pub enum TokenKind<'a> {
Span(&'a Span),
Path(&'a [String]),
RawType(&'a Type),
Attribute(&'a str),
}

#[allow(dead_code)]
impl<'a> Token<'a> {
fn new_crate(origin: Origin, crate_: &'a Crate) -> Self {
Self {
origin,
kind: TokenKind::Crate(crate_),
}
}

/// The name of the actual runtime type of this token,
/// intended to fulfill resolution requests for the __typename property.
#[inline]
Expand All @@ -111,6 +117,7 @@ impl<'a> Token<'a> {
TokenKind::Path(..) => "Path",
TokenKind::Crate(..) => "Crate",
TokenKind::CrateDiff(..) => "CrateDiff",
TokenKind::Attribute(..) => "Attribute",
TokenKind::RawType(ty) => match ty {
rustdoc_types::Type::ResolvedPath { .. } => "ResolvedPathType",
rustdoc_types::Type::Primitive(..) => "PrimitiveType",
Expand Down Expand Up @@ -203,6 +210,13 @@ impl<'a> Token<'a> {
})
}

fn as_attribute(&self) -> Option<&'a str> {
match &self.kind {
TokenKind::Attribute(attr) => Some(*attr),
_ => None,
}
}

fn as_raw_type(&self) -> Option<&'a rustdoc_types::Type> {
match &self.kind {
TokenKind::RawType(ty) => Some(*ty),
Expand Down Expand Up @@ -337,6 +351,14 @@ fn get_impl_property(token: &Token, field_name: &str) -> FieldValue {
}
}

fn get_attribute_property(token: &Token, field_name: &str) -> FieldValue {
let attribute_token = token.as_attribute().expect("token was not an Attribute");
match field_name {
"value" => attribute_token.into(),
_ => unreachable!("Attribute property {field_name}"),
}
}

fn get_resolved_path_type_property(token: &Token, field_name: &str) -> FieldValue {
let type_token = token.as_raw_type().expect("token was not a RawType");
match type_token {
Expand Down Expand Up @@ -467,6 +489,9 @@ impl<'a> Adapter<'a> for RustdocAdapter<'a> {
property_mapper(ctx, field_name.as_ref(), get_impl_property)
}))
}
"Attribute" => Box::new(data_contexts.map(move |ctx| {
property_mapper(ctx, field_name.as_ref(), get_attribute_property)
})),
"ResolvedPathType" => Box::new(data_contexts.map(move |ctx| {
property_mapper(ctx, field_name.as_ref(), get_resolved_path_type_property)
})),
Expand Down Expand Up @@ -607,25 +632,47 @@ impl<'a> Adapter<'a> for RustdocAdapter<'a> {
"Item" | "ImplOwner" | "Struct" | "StructField" | "Enum" | "Variant"
| "PlainVariant" | "TupleVariant" | "StructVariant" | "Function" | "Method"
| "Impl"
if edge_name.as_ref() == "span" =>
if matches!(edge_name.as_ref(), "span" | "attribute") =>
{
Box::new(data_contexts.map(move |ctx| {
let neighbors: Box<dyn Iterator<Item = Self::DataToken> + 'a> =
match &ctx.current_token {
None => Box::new(std::iter::empty()),
Some(token) => {
let origin = token.origin;
let item = token.as_item().expect("token was not an Item");
if let Some(span) = &item.span {
Box::new(std::iter::once(origin.make_span_token(span)))
} else {
Box::new(std::iter::empty())
match edge_name.as_ref() {
"span" => Box::new(data_contexts.map(move |ctx| {
let neighbors: Box<dyn Iterator<Item = Self::DataToken> + 'a> =
match &ctx.current_token {
None => Box::new(std::iter::empty()),
Some(token) => {
let origin = token.origin;
let item = token.as_item().expect("token was not an Item");
if let Some(span) = &item.span {
Box::new(std::iter::once(origin.make_span_token(span)))
} else {
Box::new(std::iter::empty())
}
}
}
};
};

(ctx, neighbors)
}))
(ctx, neighbors)
})),
"attribute" => Box::new(data_contexts.map(move |ctx| {
let neighbors: Box<dyn Iterator<Item = Self::DataToken> + 'a> =
match &ctx.current_token {
None => Box::new(std::iter::empty()),
Some(token) => {
let origin = token.origin;
let item = token.as_item().expect("token was not an Item");
Box::new(
item.attrs
.iter()
.map(move |attr| origin.make_attribute_token(attr)),
)
}
};

(ctx, neighbors)
})),
_ => unreachable!(
"project_neighbors {current_type_name} {edge_name} {parameters:?}"
),
}
}
"ImplOwner" | "Struct" | "Enum"
if matches!(edge_name.as_ref(), "impl" | "inherent_impl") =>
Expand Down Expand Up @@ -959,6 +1006,8 @@ mod tests {
query_execution_tests!(
enum_missing,
enum_repr_c_removed,
enum_repr_int_changed,
enum_repr_int_removed,
enum_variant_added,
enum_variant_missing,
function_missing,
Expand Down
66 changes: 66 additions & 0 deletions src/queries/enum_repr_int_changed.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
SemverQuery(
id: "enum_repr_int_changed",
human_readable_name: "enum repr(u*)/repr(i*) changed",
description: "The repr(u*) or repr(i*) attribute on an enum was changed to another integer type. This can cause its memory representation to change, breaking FFI use cases.",
required_update: Major,

// TODO: Change the reference link to point to the cargo semver reference
// once it has a section on repr(u*)/repr(i*).
reference_link: Some("https://doc.rust-lang.org/nomicon/other-reprs.html#repru-repri"),
query: r#"
{
CrateDiff {
baseline {
item {
... on Enum {
visibility_limit @filter(op: "=", value: ["$public"]) @output
name @tag @output

attribute {
attr_value: value @filter(op: "regex", value: ["$repr_regex"])
@tag @output(name: "old_attr")
}

path {
path @tag @output
}
}
}
}
current {
item {
... on Enum {
visibility_limit @filter(op: "=", value: ["$public"])
name @filter(op: "=", value: ["%name"])

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

# Check that there exists an attribute that:
# - looks like repr(i*) or repr(u*)
# - but is not the same repr(i*) or repr(u*) that we had before.
# This is the breaking change.
attribute @fold @transform(op: "count") @filter(op: ">", value: ["$zero"]) {
value @filter(op: "regex", value: ["$repr_regex"])
@filter(op: "!=", value: ["%attr_value"])
@output(name: "new_attr")
}

span_: span @optional {
filename @output
begin_line @output
}
}
}
}
}
}"#,
arguments: {
"public": "public",
"repr_regex": "#\\[repr\\([ui]\\d+\\)\\]",
"zero": 0,
},
error_message: "The repr(u*) or repr(i*) attribute on an enum was changed to another integer type. This can cause its memory representation to change, breaking FFI use cases.",
per_result_error_template: Some("enum {{name}} {{old_attr}} -> {{new_attr.0}} in {{span_filename}}:{{span_begin_line}}"),
)
Loading